From 87f6ea2681eadea913030fc98b07462662818d26 Mon Sep 17 00:00:00 2001 From: danielmeppiel Date: Fri, 22 May 2026 23:45:56 +0200 Subject: [PATCH] test(integration): unstick 23 stale tests that drift from production code Integration Tests have been silently skipped in CI/CD Pipeline since the early-stage Build & Test jobs were failing on Windows; once Windows is green (#1456), the integration matrix runs for the first time in weeks and surfaces accumulated test rot: - test_git_cache_{hermetic,phase3w5}: GitCache now lays out checkouts under // (perf #1433); tests still expected the pre-variant / path. Add the 'full' variant segment and pass-through kwargs (partial, sparse_paths, promisor_url) to mock assertions. - test_context_optimizer_{phase3w4,placement}: ContextOptimizer fallback path reads instruction.file_path.stem; MagicMock(spec= Instruction) does not expose dataclass fields, so set file_path on the helper. - test_mcp_integrator_{install_flow,phase3w4}::test_registry_overlay _warns_when_string: _apply_overlay no longer warns on string 'registry' (silent no-op). Invert the assertion. - test_mcp_integrator_{characterisation,phase3w5}::test_skips_parse _errors_and_continues: assertion depended on Path.iterdir order (not guaranteed across OSes). Match on the set of called paths and key the ValueError side-effect by path identity. - test_wave8_codex_download_coverage::test_remote_only_rejected: Codex now accepts streamable-http remote-only servers; only SSE remotes are rejected. Set transport_type='sse' so the test actually exercises the rejection branch. Full integration suite: 9000 passed, 222 skipped, 2 xfailed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../test_context_optimizer_phase3w4.py | 6 ++++ .../test_context_optimizer_placement.py | 6 ++++ tests/integration/test_git_cache_hermetic.py | 30 +++++++++++-------- tests/integration/test_git_cache_phase3w5.py | 30 +++++++++++-------- .../test_mcp_integrator_characterisation.py | 16 ++++++++-- .../test_mcp_integrator_install_flow.py | 6 +++- .../test_mcp_integrator_phase3w4.py | 6 +++- .../test_mcp_integrator_phase3w5.py | 16 ++++++++-- .../test_wave8_codex_download_coverage.py | 7 ++++- 9 files changed, 90 insertions(+), 33 deletions(-) diff --git a/tests/integration/test_context_optimizer_phase3w4.py b/tests/integration/test_context_optimizer_phase3w4.py index d98d0f91a..05ac80d90 100644 --- a/tests/integration/test_context_optimizer_phase3w4.py +++ b/tests/integration/test_context_optimizer_phase3w4.py @@ -40,11 +40,17 @@ def _make_instruction(apply_to: str | None = "**/*.py", content: str = "content") -> Instruction: + from pathlib import Path as _Path + inst = MagicMock(spec=Instruction) inst.apply_to = apply_to inst.content = content inst.source_file = None inst.source = None + # Production fallback path reads instruction.file_path.stem when an + # instruction matches no files; spec=Instruction does not expose + # dataclass fields automatically, so set it explicitly. + inst.file_path = _Path("test.instructions.md") return inst diff --git a/tests/integration/test_context_optimizer_placement.py b/tests/integration/test_context_optimizer_placement.py index e85de2559..f8c62b1e8 100644 --- a/tests/integration/test_context_optimizer_placement.py +++ b/tests/integration/test_context_optimizer_placement.py @@ -40,11 +40,17 @@ def _make_instruction(apply_to: str | None = "**/*.py", content: str = "content") -> Instruction: + from pathlib import Path as _Path + inst = MagicMock(spec=Instruction) inst.apply_to = apply_to inst.content = content inst.source_file = None inst.source = None + # Production fallback path reads instruction.file_path.stem when an + # instruction matches no files; spec=Instruction does not expose + # dataclass fields automatically, so set it explicitly. + inst.file_path = _Path("test.instructions.md") return inst diff --git a/tests/integration/test_git_cache_hermetic.py b/tests/integration/test_git_cache_hermetic.py index be7027140..4648109a6 100644 --- a/tests/integration/test_git_cache_hermetic.py +++ b/tests/integration/test_git_cache_hermetic.py @@ -75,7 +75,7 @@ class TestGetCheckout: def test_cache_hit_returns_existing_checkout(self, cache: GitCache) -> None: url = "https://github.com/owner/repo.git" sha = "a" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" checkout_dir.mkdir(parents=True) with ( @@ -93,7 +93,7 @@ def test_cache_hit_returns_existing_checkout(self, cache: GitCache) -> None: def test_cache_hit_with_failed_integrity_evicts_and_recreates(self, cache: GitCache) -> None: url = "https://github.com/owner/repo.git" sha = "b" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" checkout_dir.mkdir(parents=True) recreated = checkout_dir.parent / "recreated" @@ -108,8 +108,10 @@ def test_cache_hit_with_failed_integrity_evicts_and_recreates(self, cache: GitCa assert result == recreated mock_evict.assert_called_once_with(checkout_dir) - mock_ensure.assert_called_once_with(url, cache_shard_key(url), sha, env=None) - mock_create.assert_called_once_with(url, cache_shard_key(url), sha, env=None) + mock_ensure.assert_called_once_with(url, cache_shard_key(url), sha, env=None, partial=False) + mock_create.assert_called_once_with( + url, cache_shard_key(url), sha, env=None, sparse_paths=None, promisor_url=None + ) def test_refresh_ignores_existing_checkout(self, tmp_path: Path) -> None: with ( @@ -119,7 +121,7 @@ def test_refresh_ignores_existing_checkout(self, tmp_path: Path) -> None: cache = GitCache(tmp_path, refresh=True) url = "https://github.com/owner/repo.git" sha = "c" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" checkout_dir.mkdir(parents=True) with ( @@ -138,7 +140,7 @@ def test_refresh_ignores_existing_checkout(self, tmp_path: Path) -> None: def test_cache_miss_creates_checkout(self, cache: GitCache) -> None: url = "https://github.com/owner/repo.git" sha = "d" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" with ( patch.object(cache, "_resolve_sha", return_value=sha), @@ -148,8 +150,12 @@ def test_cache_miss_creates_checkout(self, cache: GitCache) -> None: result = cache.get_checkout(url, None, locked_sha=sha, env={"A": "1"}) assert result == checkout_dir - mock_ensure.assert_called_once_with(url, cache_shard_key(url), sha, env={"A": "1"}) - mock_create.assert_called_once_with(url, cache_shard_key(url), sha, env={"A": "1"}) + mock_ensure.assert_called_once_with( + url, cache_shard_key(url), sha, env={"A": "1"}, partial=False + ) + mock_create.assert_called_once_with( + url, cache_shard_key(url), sha, env={"A": "1"}, sparse_paths=None, promisor_url=None + ) class TestResolveSha: @@ -447,7 +453,7 @@ class TestCreateCheckout: def test_write_dedup_hit_under_lock_returns_existing_checkout(self, cache: GitCache) -> None: url = "https://example.com/repo.git" shard_key = cache_shard_key(url) - final_dir = cache._checkouts_root / shard_key / ("a" * 40) + final_dir = cache._checkouts_root / shard_key / ("a" * 40) / "full" final_dir.mkdir(parents=True) with ( @@ -465,7 +471,7 @@ def test_invalid_existing_checkout_recreates_from_bare_repo(self, cache: GitCach shard_key = cache_shard_key(url) bare_dir = cache._db_root / shard_key bare_dir.mkdir(parents=True) - final_dir = cache._checkouts_root / shard_key / ("b" * 40) + final_dir = cache._checkouts_root / shard_key / ("b" * 40) / "full" final_dir.mkdir(parents=True) def _land(staged: Path, final: Path, _lock: object) -> bool: @@ -534,7 +540,7 @@ def test_checkout_failure_cleans_staged_checkout(self, cache: GitCache) -> None: def test_atomic_land_false_accepts_valid_winner(self, cache: GitCache) -> None: url = "https://example.com/repo.git" shard_key = cache_shard_key(url) - final_dir = cache._checkouts_root / shard_key / ("e" * 40) + final_dir = cache._checkouts_root / shard_key / ("e" * 40) / "full" final_dir.mkdir(parents=True) (cache._db_root / shard_key).mkdir(parents=True) @@ -556,7 +562,7 @@ def test_atomic_land_false_accepts_valid_winner(self, cache: GitCache) -> None: def test_atomic_land_false_with_invalid_winner_evicts_and_raises(self, cache: GitCache) -> None: url = "https://example.com/repo.git" shard_key = cache_shard_key(url) - final_dir = cache._checkouts_root / shard_key / ("f" * 40) + final_dir = cache._checkouts_root / shard_key / ("f" * 40) / "full" final_dir.mkdir(parents=True) (cache._db_root / shard_key).mkdir(parents=True) diff --git a/tests/integration/test_git_cache_phase3w5.py b/tests/integration/test_git_cache_phase3w5.py index 556d565fa..b4111321c 100644 --- a/tests/integration/test_git_cache_phase3w5.py +++ b/tests/integration/test_git_cache_phase3w5.py @@ -75,7 +75,7 @@ class TestGetCheckout: def test_cache_hit_returns_existing_checkout(self, cache: GitCache) -> None: url = "https://github.com/owner/repo.git" sha = "a" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" checkout_dir.mkdir(parents=True) with ( @@ -93,7 +93,7 @@ def test_cache_hit_returns_existing_checkout(self, cache: GitCache) -> None: def test_cache_hit_with_failed_integrity_evicts_and_recreates(self, cache: GitCache) -> None: url = "https://github.com/owner/repo.git" sha = "b" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" checkout_dir.mkdir(parents=True) recreated = checkout_dir.parent / "recreated" @@ -108,8 +108,10 @@ def test_cache_hit_with_failed_integrity_evicts_and_recreates(self, cache: GitCa assert result == recreated mock_evict.assert_called_once_with(checkout_dir) - mock_ensure.assert_called_once_with(url, cache_shard_key(url), sha, env=None) - mock_create.assert_called_once_with(url, cache_shard_key(url), sha, env=None) + mock_ensure.assert_called_once_with(url, cache_shard_key(url), sha, env=None, partial=False) + mock_create.assert_called_once_with( + url, cache_shard_key(url), sha, env=None, sparse_paths=None, promisor_url=None + ) def test_refresh_ignores_existing_checkout(self, tmp_path: Path) -> None: with ( @@ -119,7 +121,7 @@ def test_refresh_ignores_existing_checkout(self, tmp_path: Path) -> None: cache = GitCache(tmp_path, refresh=True) url = "https://github.com/owner/repo.git" sha = "c" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" checkout_dir.mkdir(parents=True) with ( @@ -138,7 +140,7 @@ def test_refresh_ignores_existing_checkout(self, tmp_path: Path) -> None: def test_cache_miss_creates_checkout(self, cache: GitCache) -> None: url = "https://github.com/owner/repo.git" sha = "d" * 40 - checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha + checkout_dir = cache._checkouts_root / cache_shard_key(url) / sha / "full" with ( patch.object(cache, "_resolve_sha", return_value=sha), @@ -148,8 +150,12 @@ def test_cache_miss_creates_checkout(self, cache: GitCache) -> None: result = cache.get_checkout(url, None, locked_sha=sha, env={"A": "1"}) assert result == checkout_dir - mock_ensure.assert_called_once_with(url, cache_shard_key(url), sha, env={"A": "1"}) - mock_create.assert_called_once_with(url, cache_shard_key(url), sha, env={"A": "1"}) + mock_ensure.assert_called_once_with( + url, cache_shard_key(url), sha, env={"A": "1"}, partial=False + ) + mock_create.assert_called_once_with( + url, cache_shard_key(url), sha, env={"A": "1"}, sparse_paths=None, promisor_url=None + ) class TestResolveSha: @@ -447,7 +453,7 @@ class TestCreateCheckout: def test_write_dedup_hit_under_lock_returns_existing_checkout(self, cache: GitCache) -> None: url = "https://example.com/repo.git" shard_key = cache_shard_key(url) - final_dir = cache._checkouts_root / shard_key / ("a" * 40) + final_dir = cache._checkouts_root / shard_key / ("a" * 40) / "full" final_dir.mkdir(parents=True) with ( @@ -465,7 +471,7 @@ def test_invalid_existing_checkout_recreates_from_bare_repo(self, cache: GitCach shard_key = cache_shard_key(url) bare_dir = cache._db_root / shard_key bare_dir.mkdir(parents=True) - final_dir = cache._checkouts_root / shard_key / ("b" * 40) + final_dir = cache._checkouts_root / shard_key / ("b" * 40) / "full" final_dir.mkdir(parents=True) def _land(staged: Path, final: Path, _lock: object) -> bool: @@ -534,7 +540,7 @@ def test_checkout_failure_cleans_staged_checkout(self, cache: GitCache) -> None: def test_atomic_land_false_accepts_valid_winner(self, cache: GitCache) -> None: url = "https://example.com/repo.git" shard_key = cache_shard_key(url) - final_dir = cache._checkouts_root / shard_key / ("e" * 40) + final_dir = cache._checkouts_root / shard_key / ("e" * 40) / "full" final_dir.mkdir(parents=True) (cache._db_root / shard_key).mkdir(parents=True) @@ -556,7 +562,7 @@ def test_atomic_land_false_accepts_valid_winner(self, cache: GitCache) -> None: def test_atomic_land_false_with_invalid_winner_evicts_and_raises(self, cache: GitCache) -> None: url = "https://example.com/repo.git" shard_key = cache_shard_key(url) - final_dir = cache._checkouts_root / shard_key / ("f" * 40) + final_dir = cache._checkouts_root / shard_key / ("f" * 40) / "full" final_dir.mkdir(parents=True) (cache._db_root / shard_key).mkdir(parents=True) diff --git a/tests/integration/test_mcp_integrator_characterisation.py b/tests/integration/test_mcp_integrator_characterisation.py index ee1c48d57..a9f13fab9 100644 --- a/tests/integration/test_mcp_integrator_characterisation.py +++ b/tests/integration/test_mcp_integrator_characterisation.py @@ -297,13 +297,23 @@ def test_skips_parse_errors_and_continues(self, tmp_path: Path) -> None: pkg.name = "good" pkg.get_mcp_dependencies.return_value = [dep] + # Side-effect order must follow the filesystem-discovery order of + # apm.yml files (which Path.iterdir does not guarantee to be + # alphabetic across OSes/filesystems). Determine the order + # lazily so the bad-then-good (ValueError-then-pkg) sequence + # always lines up with whichever directory is enumerated first. + def _from_apm_yml(path, *_args, **_kw): + if path == first: + raise ValueError("bad") + return pkg + with patch("apm_cli.models.apm_package.APMPackage") as mock_pkg_cls: - mock_pkg_cls.from_apm_yml.side_effect = [ValueError("bad"), pkg] + mock_pkg_cls.from_apm_yml.side_effect = _from_apm_yml result = MCPIntegrator.collect_transitive(apm_modules) assert result == [dep] - assert mock_pkg_cls.from_apm_yml.call_args_list[0].args[0] == first - assert mock_pkg_cls.from_apm_yml.call_args_list[1].args[0] == second + called_paths = {call.args[0] for call in mock_pkg_cls.from_apm_yml.call_args_list} + assert called_paths == {first, second} def test_supports_lock_entries_with_virtual_path(self, tmp_path: Path) -> None: apm_modules = tmp_path / "apm_modules" diff --git a/tests/integration/test_mcp_integrator_install_flow.py b/tests/integration/test_mcp_integrator_install_flow.py index 78bc64e5c..a19b1ca20 100644 --- a/tests/integration/test_mcp_integrator_install_flow.py +++ b/tests/integration/test_mcp_integrator_install_flow.py @@ -329,6 +329,10 @@ def test_version_overlay_warns(self): assert any("version" in str(w.message) for w in caught) def test_registry_overlay_warns_when_string(self): + # Historical behavior: a string `registry` field on an MCPDependency + # emitted a runtime warning. The warning was removed when the field + # became a no-op overlay (kept for forward-compat with newer apm.yml + # schemas that may attach it). Assert the silent-noop contract. dep = _make_dep("srv") dep.version = None dep.registry = "custom-registry" @@ -336,7 +340,7 @@ def test_registry_overlay_warns_when_string(self): with warnings.catch_warnings(record=True) as caught: warnings.simplefilter("always") MCPIntegrator._apply_overlay(cache, dep) - assert any("registry" in str(w.message) for w in caught) + assert not any("registry" in str(w.message) for w in caught) def test_unknown_server_is_noop(self): dep = _make_dep("missing-srv", transport="stdio") diff --git a/tests/integration/test_mcp_integrator_phase3w4.py b/tests/integration/test_mcp_integrator_phase3w4.py index be8177d88..70af95b11 100644 --- a/tests/integration/test_mcp_integrator_phase3w4.py +++ b/tests/integration/test_mcp_integrator_phase3w4.py @@ -329,6 +329,10 @@ def test_version_overlay_warns(self): assert any("version" in str(w.message) for w in caught) def test_registry_overlay_warns_when_string(self): + # Historical behavior: a string `registry` field on an MCPDependency + # emitted a runtime warning. The warning was removed when the field + # became a no-op overlay (kept for forward-compat with newer apm.yml + # schemas that may attach it). Assert the silent-noop contract. dep = _make_dep("srv") dep.version = None dep.registry = "custom-registry" @@ -336,7 +340,7 @@ def test_registry_overlay_warns_when_string(self): with warnings.catch_warnings(record=True) as caught: warnings.simplefilter("always") MCPIntegrator._apply_overlay(cache, dep) - assert any("registry" in str(w.message) for w in caught) + assert not any("registry" in str(w.message) for w in caught) def test_unknown_server_is_noop(self): dep = _make_dep("missing-srv", transport="stdio") diff --git a/tests/integration/test_mcp_integrator_phase3w5.py b/tests/integration/test_mcp_integrator_phase3w5.py index ee1c48d57..a9f13fab9 100644 --- a/tests/integration/test_mcp_integrator_phase3w5.py +++ b/tests/integration/test_mcp_integrator_phase3w5.py @@ -297,13 +297,23 @@ def test_skips_parse_errors_and_continues(self, tmp_path: Path) -> None: pkg.name = "good" pkg.get_mcp_dependencies.return_value = [dep] + # Side-effect order must follow the filesystem-discovery order of + # apm.yml files (which Path.iterdir does not guarantee to be + # alphabetic across OSes/filesystems). Determine the order + # lazily so the bad-then-good (ValueError-then-pkg) sequence + # always lines up with whichever directory is enumerated first. + def _from_apm_yml(path, *_args, **_kw): + if path == first: + raise ValueError("bad") + return pkg + with patch("apm_cli.models.apm_package.APMPackage") as mock_pkg_cls: - mock_pkg_cls.from_apm_yml.side_effect = [ValueError("bad"), pkg] + mock_pkg_cls.from_apm_yml.side_effect = _from_apm_yml result = MCPIntegrator.collect_transitive(apm_modules) assert result == [dep] - assert mock_pkg_cls.from_apm_yml.call_args_list[0].args[0] == first - assert mock_pkg_cls.from_apm_yml.call_args_list[1].args[0] == second + called_paths = {call.args[0] for call in mock_pkg_cls.from_apm_yml.call_args_list} + assert called_paths == {first, second} def test_supports_lock_entries_with_virtual_path(self, tmp_path: Path) -> None: apm_modules = tmp_path / "apm_modules" diff --git a/tests/integration/test_wave8_codex_download_coverage.py b/tests/integration/test_wave8_codex_download_coverage.py index 2054dd16b..5ac7b293b 100644 --- a/tests/integration/test_wave8_codex_download_coverage.py +++ b/tests/integration/test_wave8_codex_download_coverage.py @@ -174,12 +174,17 @@ def test_server_name_override(self, tmp_path: Path) -> None: assert "custom" in data["mcp_servers"] def test_remote_only_rejected(self, tmp_path: Path) -> None: + # Codex now accepts remote-only streamable-http servers (see + # CodexClientAdapter._format_server_config). Only SSE and + # non-https / empty-URL remotes are rejected. Assert the SSE + # rejection branch which is the actual remote-only "reject" + # contract. from apm_cli.adapters.client.codex import CodexClientAdapter adapter = CodexClientAdapter(project_root=tmp_path) server_info = { "name": "remote", - "remotes": [{"url": "https://example.com/sse"}], + "remotes": [{"url": "https://example.com/sse", "transport_type": "sse"}], "packages": [], } with patch.object(adapter, "_fetch_server_info", return_value=server_info):