From 116f88b023996a52a46f02b2dd8bffc8c15c9246 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Sat, 5 Sep 2026 21:03:50 -0700 Subject: [PATCH] fix(e2e-changed): keep the gate off suites the stack cannot run The selector picked up two suites that can never pass in this stack, so editing either one turned the check permanently red: the presidio masking suite calls pytest.fail without an analyzer and anonymizer that up.sh never starts, and the pipecat audio suite skips itself at import time unless the NLTK punkt_tab data is present, which nothing installs. tests/e2e/coverage_registry/test_collector.py had the same problem for a different reason. Its nested pytest.main autoloads pytest-retry from the ci group the workflow installs and dies with "INTERNALERROR: no option named 'filtered_exceptions'", so the collect-only pass now disables that plugin. The plugin's entry point is pytest-retry, not retry, so the same one-word fix lands on mutmut's pytest_add_cli_args, where "-p no:retry" was disabling nothing. Two smaller holes in the harness: a canary argument the shell never expanded used to select nothing and let the gate pass green, and a secret that cannot be represented in both bash and dotenv was rejected without naming the key. --- .github/e2e-stack/secrets_to_env.py | 13 +++-- .github/e2e-stack/select_tests.py | 23 +++++++-- pyproject.toml | 2 +- .../test_e2e_changed_gate.py | 49 ++++++++++++++++++- tests/e2e/CONTRIBUTING.md | 4 +- tests/e2e/coverage_registry/collector.py | 2 + 6 files changed, 80 insertions(+), 13 deletions(-) diff --git a/.github/e2e-stack/secrets_to_env.py b/.github/e2e-stack/secrets_to_env.py index e7870a32e42..7913b25918b 100644 --- a/.github/e2e-stack/secrets_to_env.py +++ b/.github/e2e-stack/secrets_to_env.py @@ -20,10 +20,15 @@ def main() -> int: except (ValidationError, UnicodeError): _ = sys.stderr.write("expected a JSON object containing string environment values\n") return 1 - if any( - ENV_NAME.fullmatch(key) is None or any(char in value for char in "'\n\r\0") for key, value in secrets.items() - ): - _ = sys.stderr.write("environment names or values cannot be represented in both bash and dotenv\n") + unusable: Final = tuple( + key + for key, value in secrets.items() + if ENV_NAME.fullmatch(key) is None or any(char in value for char in "'\n\r\0") + ) + if unusable: + _ = sys.stderr.write( + f"these names or values cannot be represented in both bash and dotenv: {' '.join(sorted(unusable))}\n" + ) return 1 for value in secrets.values(): if len(value) >= MIN_MASKED_LENGTH: diff --git a/.github/e2e-stack/select_tests.py b/.github/e2e-stack/select_tests.py index cbe582b5b42..a62358f81ff 100644 --- a/.github/e2e-stack/select_tests.py +++ b/.github/e2e-stack/select_tests.py @@ -3,8 +3,11 @@ import sys from typing import Final SELECTABLE: Final = re.compile(r"^tests/e2e/([A-Za-z0-9_.-]+/)*test_[A-Za-z0-9_.-]+\.py$") -OWN_LANE: Final = re.compile( - r"^tests/e2e/(ui|claude_code|load)/|^tests/e2e/batches/test_managed_files_enforcement_e2e\.py$" +UNSUPPORTED: Final = re.compile( + r"^tests/e2e/(ui|claude_code|load)/" + r"|^tests/e2e/llm_translation/realtime/test_realtime_pipecat_audio_e2e\.py$" + r"|^tests/e2e/batches/test_managed_files_enforcement_e2e\.py$" + r"|^tests/e2e/guardrails/test_presidio_masking_e2e\.py$" ) HARNESS: Final = re.compile( r"^tests/e2e/[A-Za-z0-9_.-]+\.(py|ini)$" @@ -12,18 +15,28 @@ HARNESS: Final = re.compile( r"|^\.github/e2e-stack/" r"|^\.github/workflows/test-e2e-changed\.yml$" ) +UNEXPANDED: Final = re.compile(r"[*?\[]") + + +def is_selectable(path: str) -> bool: + return SELECTABLE.match(path) is not None and UNSUPPORTED.match(path) is None def select(changed: tuple[str, ...], canary: tuple[str, ...]) -> tuple[str, ...]: - direct: Final = frozenset(path for path in changed if SELECTABLE.match(path) and not OWN_LANE.match(path)) + direct: Final = frozenset(path for path in changed if is_selectable(path)) harness_changed: Final = any(HARNESS.match(path) for path in changed) - canary_tests: Final = frozenset(path for path in canary if harness_changed and SELECTABLE.match(path)) + canary_tests: Final = frozenset(path for path in canary if harness_changed and is_selectable(path)) return tuple(sorted(direct | canary_tests)) def main() -> int: + canary: Final = tuple(sys.argv[1:]) + unexpanded: Final = tuple(path for path in canary if UNEXPANDED.search(path)) + if unexpanded: + _ = sys.stderr.write(f"the canary paths reached the selector unexpanded: {' '.join(unexpanded)}\n") + return 1 changed: Final = tuple(line.strip() for line in sys.stdin if line.strip()) - _ = sys.stdout.write(" ".join(select(changed, tuple(sys.argv[1:]))) + "\n") + _ = sys.stdout.write(" ".join(select(changed, canary)) + "\n") return 0 diff --git a/pyproject.toml b/pyproject.toml index b889a3a0e60..f4f238dd4b9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -388,7 +388,7 @@ mutate_only_covered_lines = true # registered hash algorithm". Nothing to do with mutation coverage, and one # erroring test is enough to end the stats phase before any mutant runs. pytest_add_cli_args = [ - "-p", "no:retry", + "-p", "no:pytest-retry", "-p", "no:rerunfailures", "-p", "no:xdist", "--ignore=tests/test_litellm/proxy/management_endpoints/test_saml_sso.py", diff --git a/tests/code_coverage_tests/test_e2e_changed_gate.py b/tests/code_coverage_tests/test_e2e_changed_gate.py index f7c4a5e2527..d2b842364a6 100644 --- a/tests/code_coverage_tests/test_e2e_changed_gate.py +++ b/tests/code_coverage_tests/test_e2e_changed_gate.py @@ -113,6 +113,16 @@ def select_tests(changed: tuple[str, ...]) -> tuple[str, ...]: (("tests/e2e/logging/test_datadog_e2e.py", "litellm/router.py"), ("tests/e2e/logging/test_datadog_e2e.py",)), (("tests/e2e/ui/test_keys.py", "tests/e2e/claude_code/test_cli.py", "tests/e2e/load/test_burst.py"), ()), (("tests/e2e/batches/test_managed_files_enforcement_e2e.py",), ()), + (("tests/e2e/guardrails/test_presidio_masking_e2e.py",), ()), + (("tests/e2e/llm_translation/realtime/test_realtime_pipecat_audio_e2e.py",), ()), + ( + ("tests/e2e/llm_translation/realtime/test_realtime_e2e.py",), + ("tests/e2e/llm_translation/realtime/test_realtime_e2e.py",), + ), + ( + ("tests/e2e/guardrails/test_bedrock_guardrail_e2e.py",), + ("tests/e2e/guardrails/test_bedrock_guardrail_e2e.py",), + ), (("tests/e2e/logging/helpers.py", "docs/my-website/docs/index.md", "tests/e2e/CLAUDE.md"), ()), ( ("tests/e2e/logging/test_datadog_e2e.py", "tests/e2e/logging/test_datadog_e2e.py"), @@ -120,7 +130,7 @@ def select_tests(changed: tuple[str, ...]) -> tuple[str, ...]: ), ), ) -def test_changed_suite_files_are_selected_outside_the_own_lane( +def test_changed_suite_files_are_selected_unless_the_stack_cannot_run_them( changed: tuple[str, ...], expected: tuple[str, ...] ) -> None: assert select_tests(changed) == expected @@ -154,3 +164,40 @@ def test_the_canary_joins_directly_selected_files_in_sorted_order() -> None: def test_a_harness_unit_test_change_runs_itself_and_the_canary() -> None: assert select_tests(("tests/e2e/test_proxy_client.py",)) == (*CANARY, "tests/e2e/test_proxy_client.py") + + +def test_a_canary_argument_the_shell_never_expanded_fails_the_selector() -> None: + result: Final = subprocess.run( + [sys.executable, str(SELECT_TESTS), "tests/e2e/access_control/test_*.py"], + input="tests/e2e/proxy_client.py\n", + capture_output=True, + text=True, + ) + + assert result.returncode == 1 + assert "tests/e2e/access_control/test_*.py" in result.stderr + assert result.stdout == "" + + +@pytest.mark.parametrize( + ("secrets", "offender", "unprintable"), + ( + ('{"AWS_ACCESS_KEY_ID": "AKIAEXAMPLE", "BAD-NAME": "shibboleth"}', "BAD-NAME", "shibboleth"), + ("""{"AWS_SECRET_ACCESS_KEY": "quote'shibboleth"}""", "AWS_SECRET_ACCESS_KEY", "shibboleth"), + ('{"DD_API_KEY": "line\\nshibboleth"}', "DD_API_KEY", "shibboleth"), + ), +) +def test_an_unusable_secret_is_named_without_printing_its_value( + tmp_path: Path, secrets: str, offender: str, unprintable: str +) -> None: + env_path: Final = tmp_path / ".env" + + result: Final = subprocess.run( + [sys.executable, str(SECRETS_TO_ENV), str(env_path)], input=secrets, capture_output=True, text=True + ) + + assert result.returncode == 1 + assert offender in result.stderr + assert unprintable not in result.stderr + assert result.stdout == "" + assert not env_path.exists() diff --git a/tests/e2e/CONTRIBUTING.md b/tests/e2e/CONTRIBUTING.md index 106cfa31f0e..b83300cc8d5 100644 --- a/tests/e2e/CONTRIBUTING.md +++ b/tests/e2e/CONTRIBUTING.md @@ -56,9 +56,9 @@ A couple of logging destinations are configured on the proxy rather than by the ### The pull request check -Every same-repository PR that adds, modifies, or renames a `tests/e2e/**/test_*.py` file runs those changed files three times. A change to the harness itself, meaning a root-level `tests/e2e/*.py` file or `pytest.ini`, `tests/e2e/gateway/`, `.github/e2e-stack/`, or the workflow, also runs the `access_control` suite as a canary, because those files have no test of their own that exercises the stack. `.github/e2e-stack/select_tests.py` applies both rules. The stack config at `tests/e2e/gateway/stage_mirror_ci_config.yml` must declare every model the selected suites use; a missing one shows up as a failed test id in the public log. The suite's own single rerun for network errors and 5xx responses (see `pytest.ini`) applies on every pass, so a transport blip does not fail the check while a race inside a test still does. The stage-mirror stack has a control-plane backend, two gateways behind nginx, Postgres, Jaeger, and TLS cluster-mode Valkey. The stack exports every gateway address in `LITELLM_PROXY_REPLICA_URLS`, so model registration waits until each gateway lists the new model rather than whichever one the load balancer answered from. Documentation, deleted-file, and application-only changes do not start the stack or request environment approval. The `ui/`, `claude_code/`, and `load/` directories and `batches/test_managed_files_enforcement_e2e.py` remain outside this check because they use separate tooling or need a differently configured stack +Every same-repository PR that adds, modifies, or renames a `tests/e2e/**/test_*.py` file runs those changed files three times. A change to the harness itself, meaning a root-level `tests/e2e/*.py` file or `pytest.ini`, `tests/e2e/gateway/`, `.github/e2e-stack/`, or the workflow, also runs the `access_control` suite as a canary, because those files have no test of their own that exercises the stack. `.github/e2e-stack/select_tests.py` applies both rules. The stack config at `tests/e2e/gateway/stage_mirror_ci_config.yml` must declare every model the selected suites use; a missing one shows up as a failed test id in the public log. The suite's own single rerun for network errors and 5xx responses (see `pytest.ini`) applies on every pass, so a transport blip does not fail the check while a race inside a test still does. The stage-mirror stack has a control-plane backend, two gateways behind nginx, Postgres, Jaeger, and TLS cluster-mode Valkey. The stack exports every gateway address in `LITELLM_PROXY_REPLICA_URLS`, so model registration waits until each gateway lists the new model rather than whichever one the load balancer answered from. Documentation, deleted-file, and application-only changes do not start the stack or request environment approval. The `ui/`, `claude_code/`, and `load/` directories, `batches/test_managed_files_enforcement_e2e.py`, `llm_translation/realtime/test_realtime_pipecat_audio_e2e.py`, and `guardrails/test_presidio_masking_e2e.py` remain outside this check because they use separate tooling or need a differently configured stack: the pipecat audio suite skips itself at import time unless the NLTK `punkt_tab` data is installed, and the presidio suite fails without the analyzer and anonymizer services this stack does not start -Every selected file must execute at least one passing test in each pass, and any test failure, collection error, or entirely skipped or deselected file fails the check. A failed pass stops the run. The public log prints pytest's one-line summary for each pass, including the rerun count, and names each failed or errored test as `classname::name`, so a retried network error or a failing test is visible without the raw output. The final `e2e-changed-tests` job succeeds only when no supported test files changed or the approved run completed all three passes. Fork PRs with selected tests fail this gate until a maintainer brings the reviewed change onto a same-repository branch +Every selected file must execute at least one passing test in each pass, and any test failure, collection error, or entirely skipped or deselected file fails the check. A file whose tests are all marked skip therefore cannot pass this check, so unskip at least one of them, or add the file to `UNSUPPORTED` in `select_tests.py` with the reason, before changing one. A failed pass stops the run. The public log prints pytest's one-line summary for each pass, including the rerun count, and names each failed or errored test as `classname::name`, so a retried network error or a failing test is visible without the raw output. The final `e2e-changed-tests` job succeeds only when no supported test files changed or the approved run completed all three passes. Fork PRs with selected tests fail this gate until a maintainer brings the reviewed change onto a same-repository branch Repository admins must require the `e2e-changed-tests` status check for merging and configure the `e2e-changed` environment with required reviewers, self-review disabled, and admin bypass disabled. Each push cancels the previous run; a new run that selects tests needs a fresh approval. Reviewers must inspect the entire executable PR diff, including application code, dependencies, tests, and workflow helpers, before approving the exact revision. Approved code executes with provider credentials, so environment approval is a trust decision about that code diff --git a/tests/e2e/coverage_registry/collector.py b/tests/e2e/coverage_registry/collector.py index e20f7884f55..c5d09820fe7 100644 --- a/tests/e2e/coverage_registry/collector.py +++ b/tests/e2e/coverage_registry/collector.py @@ -105,6 +105,8 @@ def collect_markers(e2e_dir: Path = E2E_DIR) -> CollectedMarkers: "--continue-on-collection-errors", "-p", "no:cacheprovider", + "-p", + "no:pytest-retry", str(e2e_dir), ], plugins=[sink],