From 3dc59020c9a9018320445e28499a04535bd56aaf Mon Sep 17 00:00:00 2001 From: BANANASJIM Date: Tue, 9 Jun 2026 23:18:29 -0700 Subject: [PATCH 1/2] test(discover): isolate fallback tests from RENDERDOC_PYTHON_PATH env leak The four TestFindRenderdocFallback tests did not neutralise the ambient RENDERDOC_PYTHON_PATH variable, which pixi activation sets to a real renderdoc module directory after sync. The env-path candidate was probed with the mocked outcome but _has_renderdoc_module evaluated the real path, populating the diagnostic and failing test_import_failed_without_module_leaves_no_diagnostic. Add monkeypatch.delenv to all four tests and a regression guard (test_import_failed_with_env_module_sets_diagnostic) that pins the product behaviour: an env-path candidate holding a module artifact that fails to import records a diagnostic. No production code changes. --- .../proposal.md | 106 ++++++++++++++++++ .../tasks.md | 27 +++++ .../test-plan.md | 64 +++++++++++ tests/unit/test_discover.py | 30 +++++ 4 files changed, 227 insertions(+) create mode 100644 openspec/changes/2026-06-09-discover-test-isolation/proposal.md create mode 100644 openspec/changes/2026-06-09-discover-test-isolation/tasks.md create mode 100644 openspec/changes/2026-06-09-discover-test-isolation/test-plan.md diff --git a/openspec/changes/2026-06-09-discover-test-isolation/proposal.md b/openspec/changes/2026-06-09-discover-test-isolation/proposal.md new file mode 100644 index 0000000..9d43d85 --- /dev/null +++ b/openspec/changes/2026-06-09-discover-test-isolation/proposal.md @@ -0,0 +1,106 @@ +# Proposal: discover test isolation — RENDERDOC_PYTHON_PATH env leak + +## Root cause + +`test_import_failed_without_module_leaves_no_diagnostic` +(`tests/unit/test_discover.py:274`) fails when run under `pixi run` because +`pixi.toml` sets `RENDERDOC_PYTHON_PATH = ".local/renderdoc"` in the activation +environment, and the test does not neutralise that variable. + +### Exact mechanism (file:line) + +`find_renderdoc()` in `src/rdc/discover.py:150` reads `os.environ.get("RENDERDOC_PYTHON_PATH")`. +When the variable is set to `.local/renderdoc` (a symlink that resolves to +`.local/renderdoc` in the main worktree, which contains `renderdoc.so`), +`os.path.abspath(env_path)` produces a valid directory that is prepended to the +`candidates` list. + +The test mocks `_probe_candidate` with a lambda that ignores its `path` argument +and unconditionally returns `ProbeOutcome(IMPORT_FAILED, str(empty_dir))`. When +`find_renderdoc()` iterates over `candidates` it calls this mock with the real +`.local/renderdoc` path, receives `IMPORT_FAILED` back, then evaluates: + +```python +# discover.py:187 +elif outcome.result == ProbeResult.IMPORT_FAILED and _has_renderdoc_module(path): + _diagnostic = outcome +``` + +`_has_renderdoc_module(path)` is called with the **real** `.local/renderdoc` path +(not `empty_dir`). That directory contains `renderdoc.so`, so the function +returns `True`, and `_diagnostic` is set — making the test's final assertion +`assert _get_diagnostic() is None` fail. + +The `empty_dir` fixture has no `.so` / `.py` / `.pyd` files, so the guard at line +187 was intended to be False. The mismatch arises because the mock returns an +outcome whose `candidate_path` points to `empty_dir`, while `_has_renderdoc_module` +is evaluated against the loop variable `path`, which is the env-path candidate. + +### Why it is env-dependent + +| Condition | `RENDERDOC_PYTHON_PATH` in env | `.local/renderdoc` exists | Result | +|-----------|-------------------------------|--------------------------|--------| +| Clean checkout, no `pixi run sync` | not set or empty | no | PASS | +| `pixi run` (activation sets var) | `.local/renderdoc` | no | PASS (dir missing, not added) | +| `pixi run` after `pixi run sync` | `.local/renderdoc` | yes, has `renderdoc.so` | **FAIL** | +| `pixi run` after `pixi run sync`, even in isolation | `.local/renderdoc` | yes | **FAIL** | + +Agent 1 ran after `pixi run sync` (real renderdoc built) — fails. Agent 2 ran +without `pixi run sync` (`.local/renderdoc` absent or not yet populated) — passes. + +## Fix design + +The test's declared precondition is "import failed without a module present". It +must hold regardless of the ambient `RENDERDOC_PYTHON_PATH`. The correct fix is +to isolate the environment inside the test: use +`monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)` so that no env +candidate is injected into `candidates`. + +This is a **test fix, not a product bug**. `discover.py` correctly reads the env +variable and correctly applies `_has_renderdoc_module`; the real `.local/renderdoc` +directory does contain a module file, so setting `_diagnostic` there is the right +behaviour. The test simply failed to declare an assumption about the environment. + +The same isolation pattern should also be applied to the three sibling tests in +`TestFindRenderdocFallback` that mock `renderdoc_search_paths` and `shutil.which` +but also leave `RENDERDOC_PYTHON_PATH` unmocked. They currently pass only by +accident, each for a different reason: + +- `test_skips_crash_prone_candidate` (line 167) uses a two-outcome iterator; + with the env candidate present, the `CRASH_PRONE` outcome lands on the env + path and `SUCCESS` lands on `crash_dir` — the assertion still holds because + `_try_import_from` is mocked to return the module for any directory, but the + test no longer exercises the scenario it claims, and a third candidate would + raise `StopIteration`. +- `test_diagnostic_set_after_crash_prone` (line 207): the env candidate just + adds one more `CRASH_PRONE` probe whose outcome carries `crash_dir`, so the + assertions hold. +- `test_diagnostic_set_after_import_failed` (line 237): the env candidate + probes `IMPORT_FAILED` and `_has_renderdoc_module(env path)` is `True`, so + `_diagnostic` is set from the env candidate too — the assertions hold only + because the mocked outcome's fields point at `bad_dir` regardless. + +All three carry a latent dependency on the ambient env. Clearing the variable +in all four tests makes the suite robust. + +### Minimal change + +In `tests/unit/test_discover.py`, add one line to each of the four tests in +`TestFindRenderdocFallback`: + +```python +monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False) +``` + +Place it alongside the other `monkeypatch.setattr` calls at the top of each test +body. + +No changes to `src/rdc/discover.py`. + +## Risks + +| Risk | Mitigation | +|------|------------| +| Other env vars also leak into `find_renderdoc()` | `RENDERDOC_PYTHON_PATH` is the only env var read directly (discover.py:150). `PATH` reaches the candidate list indirectly via `shutil.which("renderdoccmd")` (discover.py:159-161) and `PYTHONPATH`/`sys.path` reach `_try_import` — both are already mocked in all four tests, so `delenv` plus the existing mocks give full isolation. | +| Removing env var hides a real test scenario | The env-path scenario is covered by `test_diagnostic_set_after_crash_prone` and `test_diagnostic_set_after_import_failed`; those tests can explicitly set `RENDERDOC_PYTHON_PATH` to a tmp dir if desired. | +| Sibling tests in `TestTryImportFrom` / `TestProbeCandidate` | Those tests do not call `find_renderdoc()` and are unaffected. | diff --git a/openspec/changes/2026-06-09-discover-test-isolation/tasks.md b/openspec/changes/2026-06-09-discover-test-isolation/tasks.md new file mode 100644 index 0000000..b760d6f --- /dev/null +++ b/openspec/changes/2026-06-09-discover-test-isolation/tasks.md @@ -0,0 +1,27 @@ +# Tasks: discover-test-isolation + +## Phase A: fix the failing test + +- [ ] In `tests/unit/test_discover.py`, + `TestFindRenderdocFallback.test_import_failed_without_module_leaves_no_diagnostic` (line 274): + add `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)` alongside the other + `monkeypatch.setattr` calls. + +## Phase B: harden sibling tests + +- [ ] `test_skips_crash_prone_candidate` (line 167): add `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)`. +- [ ] `test_diagnostic_set_after_crash_prone` (line 207): same. +- [ ] `test_diagnostic_set_after_import_failed` (line 237): same. + +## Phase C: add regression guard (TC-2) + +- [ ] Add `test_import_failed_with_env_module_sets_diagnostic` to + `TestFindRenderdocFallback`, implementing test-plan TC-2 exactly (tmp-dir + `mkdir()` + `renderdoc.so` stub file, `setenv`, mocks, assert no module + + diagnostic `IMPORT_FAILED` with `candidate_path == str(real_dir)`). + +## Phase D: verification + +- [ ] Run `pixi run uv run pytest tests/unit/test_discover.py -v` — all 17+ tests pass. +- [ ] Run `pixi run test` — full unit suite passes, coverage ≥ 80%. +- [ ] Confirm TC-1 passes with `RENDERDOC_PYTHON_PATH` set in the outer env (i.e. within `pixi run`). diff --git a/openspec/changes/2026-06-09-discover-test-isolation/test-plan.md b/openspec/changes/2026-06-09-discover-test-isolation/test-plan.md new file mode 100644 index 0000000..5a263a4 --- /dev/null +++ b/openspec/changes/2026-06-09-discover-test-isolation/test-plan.md @@ -0,0 +1,64 @@ +# Test plan: discover test isolation + +## Regression guard (primary) + +The corrected test must pass in BOTH environments: + +| Environment | Expected result | +|-------------|----------------| +| `pixi run test` with `pixi run sync` done (`.local/renderdoc` populated, `RENDERDOC_PYTHON_PATH=.local/renderdoc`) | PASS | +| Clean env, `RENDERDOC_PYTHON_PATH` unset | PASS | +| `RENDERDOC_PYTHON_PATH` set to a directory that contains `renderdoc.so` | PASS | + +To verify the "real renderdoc present" case explicitly, run: + +``` +pixi run uv run pytest tests/unit/test_discover.py::TestFindRenderdocFallback::test_import_failed_without_module_leaves_no_diagnostic -v +``` + +This already exercises the failure condition (pixi activation sets `RENDERDOC_PYTHON_PATH`). +After the fix is applied this must exit 0 in this worktree. + +## Test cases + +### TC-1 — originally failing: no-module dir, env var populated + +- Setup: `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)` added. +- Input: `empty_dir` (no `.so` / `.py` / `.pyd`), `_probe_candidate` returns + `IMPORT_FAILED`, `renderdoc_search_paths` returns `[str(empty_dir)]`, + `shutil.which` returns `None`. +- Expected: `find_renderdoc()` returns `None`; `_get_diagnostic()` returns `None`. +- Must pass with `RENDERDOC_PYTHON_PATH=.local/renderdoc` in the outer env. + +### TC-2 — regression guard: env-path candidate with real module produces diagnostic + +- New test (or inline parametrize). +- Setup: `real_dir = tmp_path / "real-rdoc"; real_dir.mkdir()`; write a stub + `(real_dir / "renderdoc.so").write_bytes(b"fake")`. A plain file named + `renderdoc.so` is sufficient: `_has_renderdoc_module` (discover.py:59-66) + only checks `renderdoc.py` is_file / glob `renderdoc*.so` / glob + `renderdoc*.pyd`; it never loads the file. `monkeypatch.setenv("RENDERDOC_PYTHON_PATH", str(real_dir))`. + Mock `_try_import -> None`, + `_probe_candidate -> ProbeOutcome(IMPORT_FAILED, str(real_dir))`, + `_try_import_from -> None`, `renderdoc_search_paths -> []`, `shutil.which -> None`. +- Expected: `find_renderdoc()` returns `None`; `_get_diagnostic()` is **not** `None`, + has `result == IMPORT_FAILED` and `candidate_path == str(real_dir)`. +- Deterministic in both ambient env states (the test sets the variable itself), + so it runs green inside and outside `pixi run`. +- This documents and protects the product behaviour (env-path module present → diagnostic set). + +### TC-3 through TC-5 — latent env leak in sibling tests + +Each of `test_skips_crash_prone_candidate`, `test_diagnostic_set_after_crash_prone`, +and `test_diagnostic_set_after_import_failed` must include: + +```python +monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False) +``` + +These tests already pass but carry a hidden dependency. Adding the `delenv` call +makes them unconditionally safe. Confirm they still pass after the addition. + +## Full suite check + +Run `pixi run test` (all unit tests, coverage ≥ 80%); all must pass. diff --git a/tests/unit/test_discover.py b/tests/unit/test_discover.py index 04b44d6..58615b7 100644 --- a/tests/unit/test_discover.py +++ b/tests/unit/test_discover.py @@ -194,6 +194,7 @@ def mock_renderdoc_search_paths(): def mock_which(cmd: str): return None + monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False) monkeypatch.setattr("rdc.discover._try_import", lambda: None) monkeypatch.setattr("rdc.discover._probe_candidate", mock_probe) monkeypatch.setattr("rdc.discover._try_import_from", lambda d: real_fake_mod) @@ -217,6 +218,7 @@ def mock_renderdoc_search_paths(): def mock_which(cmd: str): return None + monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False) monkeypatch.setattr("rdc.discover._try_import", lambda: None) monkeypatch.setattr( "rdc.discover._probe_candidate", @@ -254,6 +256,7 @@ def mock_renderdoc_search_paths(): def mock_which(cmd: str): return None + monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False) monkeypatch.setattr("rdc.discover._try_import", lambda: None) monkeypatch.setattr( "rdc.discover._probe_candidate", @@ -280,6 +283,7 @@ def test_import_failed_without_module_leaves_no_diagnostic( empty_dir = tmp_path / "no-module" empty_dir.mkdir() + monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False) monkeypatch.setattr("rdc.discover._try_import", lambda: None) monkeypatch.setattr( "rdc.discover._probe_candidate", @@ -292,6 +296,32 @@ def test_import_failed_without_module_leaves_no_diagnostic( assert find_renderdoc() is None assert _get_diagnostic() is None + def test_import_failed_with_env_module_sets_diagnostic( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """An env-path candidate holding a module artifact that fails to import + records a diagnostic, even with all other candidate sources empty. + """ + real_dir = tmp_path / "real-rdoc" + real_dir.mkdir() + (real_dir / "renderdoc.so").write_bytes(b"fake") + + monkeypatch.setenv("RENDERDOC_PYTHON_PATH", str(real_dir)) + monkeypatch.setattr("rdc.discover._try_import", lambda: None) + monkeypatch.setattr( + "rdc.discover._probe_candidate", + lambda path, timeout=5.0: ProbeOutcome(ProbeResult.IMPORT_FAILED, str(real_dir)), + ) + monkeypatch.setattr("rdc.discover._try_import_from", lambda d: None) + monkeypatch.setattr("rdc._platform.renderdoc_search_paths", lambda: []) + monkeypatch.setattr("rdc.discover.shutil.which", lambda cmd: None) + + assert find_renderdoc() is None + diag = _get_diagnostic() + assert diag is not None + assert diag.result == ProbeResult.IMPORT_FAILED + assert diag.candidate_path == str(real_dir) + class TestArmStudioDir: """_is_arm_studio_dir detects ARM PS directory layout.""" From f44d4788474c5953755127cfcd3685c4c1fe3493 Mon Sep 17 00:00:00 2001 From: BANANASJIM Date: Tue, 9 Jun 2026 23:33:09 -0700 Subject: [PATCH 2/2] docs(spec): tick completed task checkboxes --- .../2026-06-09-discover-test-isolation/tasks.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/openspec/changes/2026-06-09-discover-test-isolation/tasks.md b/openspec/changes/2026-06-09-discover-test-isolation/tasks.md index b760d6f..78f3676 100644 --- a/openspec/changes/2026-06-09-discover-test-isolation/tasks.md +++ b/openspec/changes/2026-06-09-discover-test-isolation/tasks.md @@ -2,26 +2,26 @@ ## Phase A: fix the failing test -- [ ] In `tests/unit/test_discover.py`, +- [x] In `tests/unit/test_discover.py`, `TestFindRenderdocFallback.test_import_failed_without_module_leaves_no_diagnostic` (line 274): add `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)` alongside the other `monkeypatch.setattr` calls. ## Phase B: harden sibling tests -- [ ] `test_skips_crash_prone_candidate` (line 167): add `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)`. -- [ ] `test_diagnostic_set_after_crash_prone` (line 207): same. -- [ ] `test_diagnostic_set_after_import_failed` (line 237): same. +- [x] `test_skips_crash_prone_candidate` (line 167): add `monkeypatch.delenv("RENDERDOC_PYTHON_PATH", raising=False)`. +- [x] `test_diagnostic_set_after_crash_prone` (line 207): same. +- [x] `test_diagnostic_set_after_import_failed` (line 237): same. ## Phase C: add regression guard (TC-2) -- [ ] Add `test_import_failed_with_env_module_sets_diagnostic` to +- [x] Add `test_import_failed_with_env_module_sets_diagnostic` to `TestFindRenderdocFallback`, implementing test-plan TC-2 exactly (tmp-dir `mkdir()` + `renderdoc.so` stub file, `setenv`, mocks, assert no module + diagnostic `IMPORT_FAILED` with `candidate_path == str(real_dir)`). ## Phase D: verification -- [ ] Run `pixi run uv run pytest tests/unit/test_discover.py -v` — all 17+ tests pass. -- [ ] Run `pixi run test` — full unit suite passes, coverage ≥ 80%. -- [ ] Confirm TC-1 passes with `RENDERDOC_PYTHON_PATH` set in the outer env (i.e. within `pixi run`). +- [x] Run `pixi run uv run pytest tests/unit/test_discover.py -v` — all 17+ tests pass. +- [x] Run `pixi run test` — full unit suite passes, coverage ≥ 80%. +- [x] Confirm TC-1 passes with `RENDERDOC_PYTHON_PATH` set in the outer env (i.e. within `pixi run`).