From f22bdf3debfd2ef011f94de09027635094ca936e Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Tue, 3 Nov 2026 09:23:55 -0800 Subject: [PATCH] test(e2e): let a codex row name its second model once and add the no-comments rule --- tests/e2e/AGENTS.md | 2 + tests/e2e/codex/setups.py | 18 ++++++-- tests/e2e/codex/test_switch_model_e2e.py | 44 +++++++------------- tests/e2e_harness/codex/test_codex_setups.py | 8 ++-- 4 files changed, 34 insertions(+), 38 deletions(-) diff --git a/tests/e2e/AGENTS.md b/tests/e2e/AGENTS.md index 72f9b5e39b1..b654d2dc4ef 100644 --- a/tests/e2e/AGENTS.md +++ b/tests/e2e/AGENTS.md @@ -315,6 +315,8 @@ other... ``` ## Hard Rules +- no comments in code unless they are truly necessary + - no unit tests of a product feature under `tests/e2e`, and no mock tests or monkeypatching of code anywhere in it: a product feature is proven end to end against a live proxy, never with a unit test. if a contributor asks you to write an end to end test, do NOT stage a unit test with it. If you find a product gap, call it out in the PR description. the harness's own plumbing is tested outside the suite, in `tests/e2e_harness/` (mirroring this folder's layout), because the Buildkite e2e run copies `tests/e2e/` into the runner image and runs every test in it, so a harness test in here would count as a product test in the nightly numbers. those tests run without a proxy and take their inputs as arguments or env vars (setting an env var through pytest's `monkeypatch` fixture is fine, patching a function, class, or module is not), and no coverage-registry or compat-matrix cell rests on them. judge a change inside one of them by that standard, not as a misplaced product test - use model management endpoints to create new models for a test. this could be in a conftest / inline for each test. ask the user what they want. diff --git a/tests/e2e/codex/setups.py b/tests/e2e/codex/setups.py index 523f94febb1..19c92fae51b 100644 --- a/tests/e2e/codex/setups.py +++ b/tests/e2e/codex/setups.py @@ -97,21 +97,31 @@ def with_expected_failure(setup_id: SetupId, *, reason: str) -> tuple[ParameterS ) -def also_driving(setups: tuple[ParameterSet, ...], extra: Mapping[str, Subject]) -> tuple[ParameterSet, ...]: +@dataclass(frozen=True, slots=True) +class ExtraModel: + provider: Provider + litellm_params: LiteLLMParamsBody + + +def also_driving(setups: tuple[ParameterSet, ...], extra: Mapping[str, ExtraModel]) -> tuple[ParameterSet, ...]: return tuple(_also_driving(setup, extra) for setup in setups) -def _also_driving(setup: ParameterSet, extra: Mapping[str, Subject]) -> ParameterSet: +def _also_driving(setup: ParameterSet, extra: Mapping[str, ExtraModel]) -> ParameterSet: deployment: Final = setup.values[0] assert isinstance(deployment, Deployment) added: Final = extra[deployment.litellm_params.model] return pytest.param(*setup.values, id=setup.id, marks=tuple(_widened(mark, added) for mark in setup.marks)) -def _widened(mark: pytest.MarkDecorator | pytest.Mark, added: Subject) -> pytest.MarkDecorator | pytest.Mark: +def _widened(mark: pytest.MarkDecorator | pytest.Mark, added: ExtraModel) -> pytest.MarkDecorator | pytest.Mark: subject: Final = mark.args[0] if mark.name == "meta" and mark.args else None if not isinstance(subject, Subject): return mark return meta( - replace(subject, providers=(*subject.providers, *added.providers), models=(*subject.models, *added.models)) + replace( + subject, + providers=(*subject.providers, added.provider), + models=(*subject.models, added.litellm_params.model), + ) ) diff --git a/tests/e2e/codex/test_switch_model_e2e.py b/tests/e2e/codex/test_switch_model_e2e.py index 5ff375e3730..ea8225ce281 100644 --- a/tests/e2e/codex/test_switch_model_e2e.py +++ b/tests/e2e/codex/test_switch_model_e2e.py @@ -3,7 +3,6 @@ from __future__ import annotations from collections.abc import Callable -from dataclasses import dataclass from types import MappingProxyType from typing import Final @@ -17,44 +16,34 @@ from codex.setups import ( GPT_ON_BEDROCK_RUNTIME, GPT_ON_OPENAI, Deployment, + ExtraModel, ModelSetup, also_driving, with_expected_failure, ) from e2e_config import unique_marker -from e2e_metadata import Provider, Subject +from e2e_metadata import Provider from models import LiteLLMParamsBody pytestmark = pytest.mark.e2e -OTHER_GPT_ON_BEDROCK_MANTLE: Final = "bedrock_mantle/openai.gpt-6-luna" -OTHER_GPT_ON_BEDROCK_RUNTIME: Final = "bedrock/us.openai.gpt-6-luna" -OTHER_GPT_ON_OPENAI: Final = "openai/gpt-6.1-terra" -OTHER_CLAUDE_ON_BEDROCK: Final = "bedrock/us.anthropic.claude-haiku-5-5" - - -@dataclass(frozen=True, slots=True) -class SwitchTo: - provider: Provider - litellm_params: LiteLLMParamsBody - - -TO_OTHER_GPT_ON_OPENAI: Final = SwitchTo( - Provider.OPENAI, LiteLLMParamsBody(model=OTHER_GPT_ON_OPENAI, api_key="os.environ/OPENAI_API_KEY") +TERRA_ON_OPENAI: Final = ExtraModel( + Provider.OPENAI, LiteLLMParamsBody(model="openai/gpt-6.1-terra", api_key="os.environ/OPENAI_API_KEY") ) SWITCH_TO: Final = MappingProxyType( { - GPT_ON_BEDROCK_MANTLE: SwitchTo( + GPT_ON_BEDROCK_MANTLE: ExtraModel( Provider.BEDROCK_MANTLE, - LiteLLMParamsBody(model=OTHER_GPT_ON_BEDROCK_MANTLE, aws_region_name=BEDROCK_REGION), + LiteLLMParamsBody(model="bedrock_mantle/openai.gpt-6-luna", aws_region_name=BEDROCK_REGION), ), - GPT_ON_BEDROCK_RUNTIME: SwitchTo( - Provider.BEDROCK, LiteLLMParamsBody(model=OTHER_GPT_ON_BEDROCK_RUNTIME, aws_region_name=BEDROCK_REGION) + GPT_ON_BEDROCK_RUNTIME: ExtraModel( + Provider.BEDROCK, LiteLLMParamsBody(model="bedrock/us.openai.gpt-6-luna", aws_region_name=BEDROCK_REGION) ), - GPT_ON_AZURE_OPENAI: TO_OTHER_GPT_ON_OPENAI, - GPT_ON_OPENAI: TO_OTHER_GPT_ON_OPENAI, - CLAUDE_ON_BEDROCK: SwitchTo( - Provider.BEDROCK, LiteLLMParamsBody(model=OTHER_CLAUDE_ON_BEDROCK, aws_region_name=BEDROCK_REGION) + GPT_ON_AZURE_OPENAI: TERRA_ON_OPENAI, + GPT_ON_OPENAI: TERRA_ON_OPENAI, + CLAUDE_ON_BEDROCK: ExtraModel( + Provider.BEDROCK, + LiteLLMParamsBody(model="bedrock/us.anthropic.claude-haiku-5-5", aws_region_name=BEDROCK_REGION), ), } ) @@ -78,12 +67,7 @@ def key_models(setup: ModelSetup, second_model: str) -> tuple[str, ...]: reason="LiteLLM forwards the reasoning Azure encrypted to OpenAI, which cannot decrypt another account's" " reasoning and rejects the turn with invalid_encrypted_content (#41792)", ), - MappingProxyType( - { - model: Subject(providers=(to.provider,), models=(to.litellm_params.model,)) - for model, to in SWITCH_TO.items() - } - ), + SWITCH_TO, ), ) class TestCodexSwitchModel: diff --git a/tests/e2e_harness/codex/test_codex_setups.py b/tests/e2e_harness/codex/test_codex_setups.py index cae8aa5e49a..40f4522635d 100644 --- a/tests/e2e_harness/codex/test_codex_setups.py +++ b/tests/e2e_harness/codex/test_codex_setups.py @@ -4,8 +4,9 @@ from types import MappingProxyType from typing import Final from _pytest.mark.structures import ParameterSet -from codex.setups import SETUPS, Deployment, also_driving, with_expected_failure +from codex.setups import SETUPS, Deployment, ExtraModel, also_driving, with_expected_failure from e2e_metadata import Provider, Subject +from models import LiteLLMParamsBody SECOND_MODEL: Final = "openai/second-model" @@ -28,11 +29,10 @@ def _model(setup: ParameterSet) -> str: def test_also_driving_declares_the_extra_model_on_every_setup_and_keeps_the_xfail() -> None: + second: Final = ExtraModel(Provider.OPENAI, LiteLLMParamsBody(model=SECOND_MODEL)) setups: Final = also_driving( with_expected_failure("azure_openai", reason="known bug"), - MappingProxyType( - {_model(setup): Subject(providers=(Provider.OPENAI,), models=(SECOND_MODEL,)) for setup in SETUPS} - ), + MappingProxyType({_model(setup): second for setup in SETUPS}), ) assert tuple(setup.id for setup in setups) == tuple(setup.id for setup in SETUPS)