From 531d4aa0d1fd5b0ad7c0ca7466372ab29aad977d Mon Sep 17 00:00:00 2001 From: Shay Date: Wed, 15 Jul 2026 15:32:58 -0700 Subject: [PATCH] fix(tests): isolate three xdist polluter clusters blocking -n auto default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hunt for the -n auto fast-lane polluters flagged in docs/testing-lanes.md ("Follow-up: xdist by default"). Three root causes, all shared-repo-path writers with no per-test isolation, following the #782 monkeypatch idiom: 1. tests/test_workbench_replay.py::test_replay_leaves_no_trace hardcoded `Path("engine_state")` (the real shared dir) instead of reading `engine_state._DEFAULT_DIR`, which the root-conftest autouse fixture already redirects per-test. It was a victim, not a polluter: any concurrent worker writing the real dir made this snapshot-diff flake. Fixed by reading `engine_state._DEFAULT_DIR` dynamically. 2. evals/gsm8k_math/train_sample/v1/runner.py hardcoded its report.json output to the committed repo path with no override. Two test files (test_rat1_end_to_end_admission.py, test_wave_a_multiplicative_ aggregation_injector.py; 4 tests total) spawn it as a subprocess and read the same file back — a write race under -n auto, and a confirmed downstream victim (test_gsm8k_sealed_attempt_scout.py:: test_report_json_mtime_unchanged_by_scout_import asserts the file's mtime is stable). Added an optional CORE_GSM8K_TRAIN_SAMPLE_REPORT_PATH env override (default unchanged) and pointed the 4 call sites at tmp_path. 3. core/cli.py's `_DEMO_RESULTS_DIR` (evals/forward_semantic_control/ results/) is written, glob-scanned, and index.json-rebuilt by ~11 tests across tests/test_cli_demo.py's TestDemoSubcommand and TestDemoPreambles classes. Added an autouse fixture monkeypatching `cli._DEMO_RESULTS_DIR` to a per-test tmp dir. This also unmasked a latent order-dependent coupling: test_demo_list_results_indexes_reports and test_demo_list_results_json_well_formed never wrote their own report, relying on a sibling test's leftover file in the shared dir (silently correct only because pytest ran the file in declaration order). Made both self-contained. No assertion weakening, no test deletion, no global autouse fixture masking real bugs. All three fixes preserve the real (non-test) default behavior byte-for-byte when unpatched/env-unset. Verification: 8x targeted -n 8 loop over the 7 affected files (before and after) did not force-reproduce the underlying race live (narrow timing window, small-scale run) — confirmation is source-level (hardcoded shared paths bypassing the established isolation idiom) plus the prior documented flake for test_replay_leaves_no_trace in docs/testing-lanes.md. Full fast-lane -n auto run recorded in the PR description. --- evals/gsm8k_math/train_sample/v1/runner.py | 9 +++++- tests/test_cli_demo.py | 32 +++++++++++++++++-- tests/test_rat1_end_to_end_admission.py | 16 +++++----- ...e_a_multiplicative_aggregation_injector.py | 18 ++++++----- tests/test_workbench_replay.py | 16 +++++++--- 5 files changed, 67 insertions(+), 24 deletions(-) diff --git a/evals/gsm8k_math/train_sample/v1/runner.py b/evals/gsm8k_math/train_sample/v1/runner.py index 87195dfc..17249d63 100644 --- a/evals/gsm8k_math/train_sample/v1/runner.py +++ b/evals/gsm8k_math/train_sample/v1/runner.py @@ -25,6 +25,7 @@ single scoring path. from __future__ import annotations import json +import os import sys from pathlib import Path from typing import Any @@ -33,7 +34,13 @@ from evals.gsm8k_math.runner import _score_one_candidate_graph _HERE = Path(__file__).resolve().parent _CASES_PATH = _HERE / "cases.jsonl" -_REPORT_PATH = _HERE / "report.json" +# Overridable so concurrent test invocations (xdist workers) can each point +# the CLI entrypoint at an isolated tmp path instead of racing on the shared +# committed report.json. Unset (the default) preserves the real operator +# contract: `python -m ...runner` writes the tracked repo file. +_REPORT_PATH = Path( + os.environ.get("CORE_GSM8K_TRAIN_SAMPLE_REPORT_PATH", str(_HERE / "report.json")) +) _SAMPLE_REL = "evals/gsm8k_math/train_sample/v1/cases.jsonl" _EXPECTED_COUNT = 50 _CORRECT_MIN = 10 diff --git a/tests/test_cli_demo.py b/tests/test_cli_demo.py index 4dae9693..1aa87d5b 100644 --- a/tests/test_cli_demo.py +++ b/tests/test_cli_demo.py @@ -10,6 +10,24 @@ import pytest from core import cli +@pytest.fixture(autouse=True) +def _isolate_demo_results_dir(tmp_path, monkeypatch): + """Redirect the demo results dir to an isolated per-test tmp dir. + + ``core.cli._DEMO_RESULTS_DIR`` is a module-level constant pointing at the + shared repo dir ``evals/forward_semantic_control/results/`` that every + ``core demo`` subcommand reads, writes, and glob-scans (including the + ``index.json`` rebuild in ``_write_results_index``). Multiple tests below + invoke ``cli.main(["demo", ...])``; left unpatched they race on that one + shared directory under ``-n auto`` (concurrent report writes plus a + scan-then-rewrite of ``index.json``). Follows the same monkeypatch-the- + module-attribute isolation idiom as the root-conftest engine-state fixture + (#782). ``TestResultsReadme`` reads the real checked-in README by a + hardcoded path, so it is unaffected by this redirection. + """ + monkeypatch.setattr(cli, "_DEMO_RESULTS_DIR", tmp_path / "results") + + class TestADR0024SuiteAliases: """Layer 1: pin the new suite aliases so they don't drift.""" @@ -93,7 +111,7 @@ class TestDemoSubcommand: assert "ALL THREE CONDITIONS" in captured.out assert "PASS" in captured.out # Report file present and well-formed. - report = Path("evals/forward_semantic_control/results/phase6_demo_report.json") + report = cli._DEMO_RESULTS_DIR / "phase6_demo_report.json" assert report.exists() data = json.loads(report.read_text()) assert data["metrics"]["all_three_conditions_pass"] is True @@ -109,6 +127,12 @@ class TestDemoSubcommand: assert "all_three_conditions_pass" in payload["metrics"] def test_demo_list_results_indexes_reports(self, capsys) -> None: + # Self-contained: write a report before indexing rather than relying + # on a sibling test's shared-directory side effect (that implicit + # ordering coupling only "worked" by accident of file-declaration + # order and would break under `-n auto` reordering). + cli.main(["demo", "phase6"]) + capsys.readouterr() rc = cli.main(["demo", "list-results"]) assert rc == 0 captured = capsys.readouterr() @@ -116,6 +140,10 @@ class TestDemoSubcommand: assert "phase6_demo_report.json" in captured.out def test_demo_list_results_json_well_formed(self, capsys) -> None: + # Self-contained for the same reason as + # test_demo_list_results_indexes_reports above. + cli.main(["demo", "phase6"]) + capsys.readouterr() rc = cli.main(["demo", "list-results", "--json"]) assert rc == 0 captured = capsys.readouterr() @@ -127,7 +155,7 @@ class TestDemoSubcommand: def test_demo_index_file_refreshed_after_run(self) -> None: cli.main(["demo", "phase6"]) - index_path = Path("evals/forward_semantic_control/results/index.json") + index_path = cli._DEMO_RESULTS_DIR / "index.json" assert index_path.exists() data = json.loads(index_path.read_text()) names = [e["file"] for e in data["reports"]] diff --git a/tests/test_rat1_end_to_end_admission.py b/tests/test_rat1_end_to_end_admission.py index 8002e4b9..51098cd1 100644 --- a/tests/test_rat1_end_to_end_admission.py +++ b/tests/test_rat1_end_to_end_admission.py @@ -114,19 +114,19 @@ def test_canonical_pack_admits_cross_sentence_composition_when_seeded(): ) -def test_wrong_zero_preserved_on_train_sample(): +def test_wrong_zero_preserved_on_train_sample(tmp_path): """The full train_sample eval must preserve wrong == 0 after RAT-1.""" + import os import subprocess import sys + report_path = tmp_path / "report.json" result = subprocess.run( [sys.executable, "-m", "evals.gsm8k_math.train_sample.v1.runner"], cwd=_repo_root(), capture_output=True, text=True, - ) - report_path = ( - _repo_root() / "evals" / "gsm8k_math" / "train_sample" / "v1" / "report.json" + env={**os.environ, "CORE_GSM8K_TRAIN_SAMPLE_REPORT_PATH": str(report_path)}, ) assert report_path.exists(), "train_sample runner must emit report.json" report = json.loads(report_path.read_text()) @@ -134,18 +134,18 @@ def test_wrong_zero_preserved_on_train_sample(): assert counts["wrong"] == 0, f"wrong-zero invariant violated: {counts}" -def test_case_0050_remains_refused_after_rat1(): +def test_case_0050_remains_refused_after_rat1(tmp_path): """Hazard pin: case 0050 must not admit after any RAT-1 wiring.""" + import os import subprocess import sys + report_path = tmp_path / "report.json" subprocess.run( [sys.executable, "-m", "evals.gsm8k_math.train_sample.v1.runner"], cwd=_repo_root(), capture_output=True, - ) - report_path = ( - _repo_root() / "evals" / "gsm8k_math" / "train_sample" / "v1" / "report.json" + env={**os.environ, "CORE_GSM8K_TRAIN_SAMPLE_REPORT_PATH": str(report_path)}, ) report = json.loads(report_path.read_text()) case = next( diff --git a/tests/test_wave_a_multiplicative_aggregation_injector.py b/tests/test_wave_a_multiplicative_aggregation_injector.py index b1eb8232..8e894ae3 100644 --- a/tests/test_wave_a_multiplicative_aggregation_injector.py +++ b/tests/test_wave_a_multiplicative_aggregation_injector.py @@ -171,41 +171,43 @@ def _has_wave_a_seed() -> bool: ) -def test_wrong_zero_preserved(): +def test_wrong_zero_preserved(tmp_path): """The full train_sample eval keeps wrong == 0 after WAVE-A.""" + import os import subprocess import sys here = Path(__file__).resolve() while here.parent != here and not (here / "pyproject.toml").exists(): here = here.parent + report_path = tmp_path / "report.json" subprocess.run( [sys.executable, "-m", "evals.gsm8k_math.train_sample.v1.runner"], cwd=here, capture_output=True, + env={**os.environ, "CORE_GSM8K_TRAIN_SAMPLE_REPORT_PATH": str(report_path)}, ) - report = json.loads( - (here / "evals" / "gsm8k_math" / "train_sample" / "v1" / "report.json").read_text() - ) + report = json.loads(report_path.read_text()) assert report["counts"]["wrong"] == 0 -def test_case_0050_remains_refused(): +def test_case_0050_remains_refused(tmp_path): """Hazard pin.""" + import os import subprocess import sys here = Path(__file__).resolve() while here.parent != here and not (here / "pyproject.toml").exists(): here = here.parent + report_path = tmp_path / "report.json" subprocess.run( [sys.executable, "-m", "evals.gsm8k_math.train_sample.v1.runner"], cwd=here, capture_output=True, + env={**os.environ, "CORE_GSM8K_TRAIN_SAMPLE_REPORT_PATH": str(report_path)}, ) - report = json.loads( - (here / "evals" / "gsm8k_math" / "train_sample" / "v1" / "report.json").read_text() - ) + report = json.loads(report_path.read_text()) case_0050 = next( (c for c in report["per_case"] if c["case_id"].endswith("-0050")), None, diff --git a/tests/test_workbench_replay.py b/tests/test_workbench_replay.py index e7b51722..aae67b0d 100644 --- a/tests/test_workbench_replay.py +++ b/tests/test_workbench_replay.py @@ -12,14 +12,13 @@ from pathlib import Path import pytest +import engine_state from chat.runtime import ChatRuntime from workbench import api as workbench_api from workbench.api import WorkbenchApi, _run_sealed_chat_turn, _with_turn_cost_and_id from workbench.journal import TurnJournal, TurnJournalEntry from workbench.replay import CRITICAL_FIELDS, INFORMATIONAL_FIELDS, replay_turn -_ENGINE_STATE_DIR = Path("engine_state") - def _snapshot(root: Path) -> dict[str, bytes]: snap: dict[str, bytes] = {} @@ -150,19 +149,26 @@ def test_hashless_legacy_turn_is_not_replayable(tmp_path, recorded, monkeypatch) def test_replay_leaves_no_trace(tmp_path, recorded) -> None: - """Obligation 4: GET /replay writes nothing — journal and engine state.""" + """Obligation 4: GET /replay writes nothing — journal and engine state. + + ``engine_state._DEFAULT_DIR`` is read fresh (not a module-level constant) + because the root-conftest ``_isolate_engine_state_default`` autouse + fixture repoints it at a per-test tmp dir; a hardcoded ``Path("engine_state")`` + would instead snapshot the real shared repo dir and false-fail under + ``-n auto`` whenever an unrelated concurrent worker wrote to it. + """ entry, _ = recorded journal = TurnJournal(journal_dir=tmp_path / "workbench_data") journal.append(entry) api = WorkbenchApi(journal=journal) journal_before = _snapshot(tmp_path / "workbench_data") - engine_state_before = _snapshot(_ENGINE_STATE_DIR) + engine_state_before = _snapshot(Path(engine_state._DEFAULT_DIR)) response = api.handle("GET", "/replay/1", b"") assert response.status == 200 assert _snapshot(tmp_path / "workbench_data") == journal_before - assert _snapshot(_ENGINE_STATE_DIR) == engine_state_before + assert _snapshot(Path(engine_state._DEFAULT_DIR)) == engine_state_before def test_wall_clock_divergence_does_not_break_equivalence(tmp_path, recorded) -> None: