From aa48fa331300e2c34abea759b41a00008b4cebee Mon Sep 17 00:00:00 2001 From: Jeremy Manning Date: Sat, 1 Aug 2026 15:42:03 -0400 Subject: [PATCH] Stop the test suite writing into its own repository (#431) tests/scenarios/test_network_failures.py generated pipeline YAML that embedded whichever ephemeral port its mock server happened to bind, then wrote it into tests/scenarios/test_pipelines/ -- a tracked directory. So every run left the working tree dirty with a different random port, and whichever port was committed last became the fixture everyone else got. Observed three times in one session; the diff is always the same two files with a new port number. The directory was also spelled as an absolute path to one developer's home directory: Path("/Users/jmanning/orchestrator/tests/scenarios/test_pipelines") On any other machine `mkdir(exist_ok=True)` raises FileNotFoundError on the missing parent, so both classes errored out rather than ran. Three more absolute paths did the same elsewhere: two `sys.path.insert` calls, and an examples directory whose absence made every real-world example test `pytest.skip` silently on any machine but one. All six YAML files in tests/scenarios/test_pipelines/ were written by this test module and read by nothing else -- outputs masquerading as fixtures -- so they are deleted rather than relocated. The paths are now repo-relative, and generated pipelines go to pytest's `tmp_path` via a small `_UsesTemporaryPipelineDir` mixin shared by the two classes that need it. CI gains a guard: after the legacy suite runs, `git diff --quiet` must hold. Deliberately NOT continue-on-error -- the backlog is allowed to be red, but it is not allowed to modify tracked files. Without the guard this regresses silently the next time someone writes a test that saves output "somewhere convenient". While in that job, `-rN` becomes `-rfE`. `-rN` suppresses the summary entirely, so the job publishes a single scalar and "did anything regress?" cannot be answered from CI output -- attributing a change meant re-running both sides locally. `-rfE` names the failures and errors, making the backlog diffable run to run. The tally the summary step reads is still the last line, so that step is unaffected. Verified by A/B on the same 8 tests: main: 8 failed, 2 tracked files modified this: 8 failed, working tree clean The 8 failures are the pre-existing "no models available" backlog and are unchanged; only the mutation is gone. Blocking suite 456 passed, 13 skipped, 0 failed. Collection 3251, unchanged. ruff clean on src and on every file touched. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 20 ++++++++++- tests/scenarios/test_network_failures.py | 36 +++++++++++++------ .../test_pipelines/concurrent_network.yaml | 30 ---------------- .../test_pipelines/model_timeout.yaml | 24 ------------- .../test_pipelines/network_recovery.yaml | 32 ----------------- .../scenarios/test_pipelines/rate_limit.yaml | 24 ------------- .../test_pipelines/web_request_test.yaml | 24 ------------- .../test_pipelines/web_search_timeout.yaml | 25 ------------- tests/scenarios/test_real_world_examples.py | 4 ++- tests/test_control_flow_conditional.py | 4 ++- tests/test_control_flow_dynamic.py | 4 ++- 11 files changed, 53 insertions(+), 174 deletions(-) delete mode 100644 tests/scenarios/test_pipelines/concurrent_network.yaml delete mode 100644 tests/scenarios/test_pipelines/model_timeout.yaml delete mode 100644 tests/scenarios/test_pipelines/network_recovery.yaml delete mode 100644 tests/scenarios/test_pipelines/rate_limit.yaml delete mode 100644 tests/scenarios/test_pipelines/web_request_test.yaml delete mode 100644 tests/scenarios/test_pipelines/web_search_timeout.yaml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 48acb7f9..f2d12e03 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -165,9 +165,27 @@ jobs: set -o pipefail uv run --no-sync python -m pytest \ -m "not unit and not contract and not e2e and not live and not integration and not docker and not slow" \ - -q --timeout=60 --timeout-method=signal --tb=no -rN \ + -q --timeout=60 --timeout-method=signal --tb=no -rfE \ | tee legacy-results.txt + - name: Fail if the suite dirtied the repository + # NOT continue-on-error, deliberately: the backlog is allowed to be + # red, but it is not allowed to modify tracked files. Tests used to + # write generated pipelines -- embedding whichever ephemeral port their + # mock server bound -- straight into tests/scenarios/test_pipelines/, + # so every run left a diff and whichever port was committed last became + # everyone else's fixture (#431). This is the guard that keeps it fixed. + if: always() + run: | + if ! git diff --quiet; then + echo "::error::The test suite modified tracked files:" + git diff --stat + echo + echo "Tests must write generated files to a temp directory" + echo "(pytest's tmp_path), never into the repository." + exit 1 + fi + - name: Publish backlog counts # Runs even when the step above fails, which is the normal case. # diff --git a/tests/scenarios/test_network_failures.py b/tests/scenarios/test_network_failures.py index 1d59b04a..3233fa21 100644 --- a/tests/scenarios/test_network_failures.py +++ b/tests/scenarios/test_network_failures.py @@ -84,15 +84,32 @@ def stop(self): self.thread.join(timeout=1) -class TestNetworkFailures: +class _UsesTemporaryPipelineDir: + """Generated pipelines go to a temp directory, never into the repository. + + These tests build pipeline YAML that embeds the ephemeral port their mock + server happened to bind. They used to write it into + tests/scenarios/test_pipelines/, which is tracked, so every run left the + working tree dirty with a different random port -- and whichever port was + committed last became the fixture everyone else got (#431). + + The directory was also an absolute path to one developer's home directory, + so on any other machine `mkdir(exist_ok=True)` raised FileNotFoundError on + the missing parent and the whole class errored out. + """ + + @pytest.fixture(autouse=True) + def _pipeline_dir(self, tmp_path): + self.test_pipelines_dir = tmp_path + + +class TestNetworkFailures(_UsesTemporaryPipelineDir): """Test network failure scenarios and timeout handling.""" - + def setup_method(self): """Set up test fixtures.""" self.mock_server = MockSlowServer() - self.test_pipelines_dir = Path("/Users/jmanning/orchestrator/tests/scenarios/test_pipelines") - self.test_pipelines_dir.mkdir(exist_ok=True) - + def teardown_method(self): """Clean up test fixtures.""" self.mock_server.stop() @@ -393,7 +410,7 @@ async def test_rate_limiting_response(self): print("✓ Rate limiting scenario tested") -class TestAPITimeoutScenarios: +class TestAPITimeoutScenarios(_UsesTemporaryPipelineDir): """Test various API timeout scenarios with real services.""" @pytest.mark.asyncio @@ -425,9 +442,7 @@ async def test_model_api_timeout(self): - slow_model_call """ - test_dir = Path("/Users/jmanning/orchestrator/tests/scenarios/test_pipelines") - test_dir.mkdir(exist_ok=True) - pipeline_path = test_dir / "model_timeout.yaml" + pipeline_path = self.test_pipelines_dir / "model_timeout.yaml" pipeline_path.write_text(pipeline_content) executor = Orchestrator() @@ -479,8 +494,7 @@ async def test_web_search_timeout(self): - web_search_with_timeout """ - test_dir = Path("/Users/jmanning/orchestrator/tests/scenarios/test_pipelines") - pipeline_path = test_dir / "web_search_timeout.yaml" + pipeline_path = self.test_pipelines_dir / "web_search_timeout.yaml" pipeline_path.write_text(pipeline_content) executor = Orchestrator() diff --git a/tests/scenarios/test_pipelines/concurrent_network.yaml b/tests/scenarios/test_pipelines/concurrent_network.yaml deleted file mode 100644 index e1d7be72..00000000 --- a/tests/scenarios/test_pipelines/concurrent_network.yaml +++ /dev/null @@ -1,30 +0,0 @@ - -name: concurrent_network_test -version: "1.0" - -steps: - - id: concurrent_requests - action: parallel - parameters: - max_concurrent: 10 - tasks: - - action: web_request - parameters: - url: "http://localhost:58202/test1" - timeout: 10 - - action: web_request - parameters: - url: "http://localhost:58202/test2" - timeout: 10 - - action: web_request - parameters: - url: "http://localhost:58202/test3" - timeout: 10 - - action: web_request - parameters: - url: "http://localhost:58202/test4" - timeout: 10 - - action: web_request - parameters: - url: "http://localhost:58202/test5" - timeout: 10 diff --git a/tests/scenarios/test_pipelines/model_timeout.yaml b/tests/scenarios/test_pipelines/model_timeout.yaml deleted file mode 100644 index d0bebeec..00000000 --- a/tests/scenarios/test_pipelines/model_timeout.yaml +++ /dev/null @@ -1,24 +0,0 @@ - -name: model_timeout_test -version: "1.0" - -steps: - - id: slow_model_call - action: llm - parameters: - model: "ollama/llama3.2:1b" # Use local model to avoid external API costs - prompt: "Generate a very long story about artificial intelligence and robotics with at least 2000 words" - max_tokens: 4000 - timeout: 5 # Very short timeout - outputs: - - story_text - - - id: fallback_response - action: python_code - parameters: - code: | - if 'story_text' not in globals() or not story_text: - story_text = "Fallback: Unable to generate full story due to timeout" - print(f"Final result: {story_text[:100]}...") - depends_on: - - slow_model_call diff --git a/tests/scenarios/test_pipelines/network_recovery.yaml b/tests/scenarios/test_pipelines/network_recovery.yaml deleted file mode 100644 index b1908dc9..00000000 --- a/tests/scenarios/test_pipelines/network_recovery.yaml +++ /dev/null @@ -1,32 +0,0 @@ - -name: network_recovery_test -version: "1.0" - -steps: - - id: first_request - action: web_request - parameters: - url: "http://localhost:58198/test" - timeout: 5 - outputs: - - first_response - - - id: wait_step - action: python_code - parameters: - code: | - import time - print("Waiting before second request...") - time.sleep(1) - depends_on: - - first_request - - - id: second_request - action: web_request - parameters: - url: "http://localhost:58198/test" - timeout: 5 - retry_count: 2 - retry_delay: 1 - depends_on: - - wait_step diff --git a/tests/scenarios/test_pipelines/rate_limit.yaml b/tests/scenarios/test_pipelines/rate_limit.yaml deleted file mode 100644 index 902f1f45..00000000 --- a/tests/scenarios/test_pipelines/rate_limit.yaml +++ /dev/null @@ -1,24 +0,0 @@ - -name: rate_limit_test -version: "1.0" - -steps: - - id: simulate_rate_limit - action: python_code - parameters: - code: | - import requests - import time - - # Simulate rate limiting behavior - for i in range(3): - try: - # This will likely get rate limited by httpbin - response = requests.get("https://httpbin.org/delay/1", timeout=5) - print(f"Request {i+1}: Status {response.status_code}") - time.sleep(0.1) # Very short delay to trigger rate limiting - except Exception as e: - print(f"Request {i+1} failed: {e}") - if "429" in str(e) or "rate" in str(e).lower(): - print("Rate limiting detected") - break diff --git a/tests/scenarios/test_pipelines/web_request_test.yaml b/tests/scenarios/test_pipelines/web_request_test.yaml deleted file mode 100644 index 04cf6395..00000000 --- a/tests/scenarios/test_pipelines/web_request_test.yaml +++ /dev/null @@ -1,24 +0,0 @@ - -name: network_test_pipeline -version: "1.0" -description: Test pipeline for network failure scenarios - -steps: - - id: web_request - action: web_request - parameters: - url: "http://this-domain-definitely-does-not-exist-12345.com/api" - timeout: 10 - method: "GET" - outputs: - - response_data - - - id: process_response - action: python_code - parameters: - code: | - import json - print(f"Received response: {response_data}") - result = json.loads(response_data) if response_data else {"error": "no data"} - depends_on: - - web_request diff --git a/tests/scenarios/test_pipelines/web_search_timeout.yaml b/tests/scenarios/test_pipelines/web_search_timeout.yaml deleted file mode 100644 index b8f41ecc..00000000 --- a/tests/scenarios/test_pipelines/web_search_timeout.yaml +++ /dev/null @@ -1,25 +0,0 @@ - -name: web_search_timeout_test -version: "1.0" - -steps: - - id: web_search_with_timeout - action: web_search - parameters: - query: "artificial intelligence research 2024" - max_results: 20 - timeout: 3 # Short timeout - outputs: - - search_results - - - id: process_results - action: python_code - parameters: - code: | - if 'search_results' in globals() and search_results: - print(f"Found {len(search_results)} results") - else: - print("No search results due to timeout") - search_results = [] - depends_on: - - web_search_with_timeout diff --git a/tests/scenarios/test_real_world_examples.py b/tests/scenarios/test_real_world_examples.py index a0957f2e..21297eaa 100644 --- a/tests/scenarios/test_real_world_examples.py +++ b/tests/scenarios/test_real_world_examples.py @@ -31,7 +31,9 @@ def setup_method(self): self.orchestrator = Orchestrator(model_registry=self.model_registry) # Path to examples directory - self.examples_dir = Path("/Users/jmanning/orchestrator/examples") + # Repo-relative: an absolute path to one developer's home directory + # meant every example silently `pytest.skip`ped on any other machine. + self.examples_dir = Path(__file__).resolve().parents[2] / "examples" def load_example_pipeline(self, filename: str) -> str: """Load an example pipeline from the examples directory.""" diff --git a/tests/test_control_flow_conditional.py b/tests/test_control_flow_conditional.py index 1026df3c..bbf60fae 100644 --- a/tests/test_control_flow_conditional.py +++ b/tests/test_control_flow_conditional.py @@ -8,7 +8,9 @@ import shutil import sys -sys.path.insert(0, '/Users/jmanning/orchestrator/src') +# Repo-relative: an absolute path to one developer's home directory made +# this import work on exactly one machine. +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "src")) from orchestrator.orchestrator import Orchestrator diff --git a/tests/test_control_flow_dynamic.py b/tests/test_control_flow_dynamic.py index 2c311aa5..1974349c 100644 --- a/tests/test_control_flow_dynamic.py +++ b/tests/test_control_flow_dynamic.py @@ -10,7 +10,9 @@ import sys import json -sys.path.insert(0, '/Users/jmanning/orchestrator/src') +# Repo-relative: an absolute path to one developer's home directory made +# this import work on exactly one machine. +sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "src")) from orchestrator.orchestrator import Orchestrator