test(discover): isolate fallback tests from RENDERDOC_PYTHON_PATH env leak#243
Conversation
… 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.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR fixes environment-dependent test failures by documenting and implementing test isolation for ChangesDiscover test isolation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openspec/changes/2026-06-09-discover-test-isolation/test-plan.md (1)
15-17: ⚡ Quick winAdd language identifier to fenced code block.
The shell command block should specify
shellorbashas the language identifier for proper syntax highlighting and tooling support.📝 Proposed fix
-``` +```shell pixi run uv run pytest tests/unit/test_discover.py::TestFindRenderdocFallback::test_import_failed_without_module_leaves_no_diagnostic -v</details> As per coding guidelines, the markdown linter flags fenced code blocks without language specifications. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@openspec/changes/2026-06-09-discover-test-isolation/test-plan.mdaround
lines 15 - 17, The fenced code block containing the shell command "pixi run uv
run pytest
tests/unit/test_discover.py::TestFindRenderdocFallback::test_import_failed_without_module_leaves_no_diagnostic
-v" needs a language identifier; update the opening fence to use "shell" (or "bash") so the block is marked as shell script for linting and syntax
highlighting, leaving the command text unchanged.</details> <!-- cr-comment:v1:101495a44d6611e18ac9d6f7 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Nitpick comments:
In@openspec/changes/2026-06-09-discover-test-isolation/test-plan.md:
- Around line 15-17: The fenced code block containing the shell command "pixi
run uv run pytest
tests/unit/test_discover.py::TestFindRenderdocFallback::test_import_failed_without_module_leaves_no_diagnostic
-v" needs a language identifier; update the opening fence to use "shell" (or "bash") so the block is marked as shell script for linting and syntax
highlighting, leaving the command text unchanged.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `c91fa532-6073-4949-9a07-43db00345ad9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 8923bc62381a5b7681919dbc7b33f08edfdad687 and cd9c9825b562848d2a79c097c39cb3c234f47668. </details> <details> <summary>📒 Files selected for processing (4)</summary> * `openspec/changes/2026-06-09-discover-test-isolation/proposal.md` * `openspec/changes/2026-06-09-discover-test-isolation/tasks.md` * `openspec/changes/2026-06-09-discover-test-isolation/test-plan.md` * `tests/unit/test_discover.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
1 participant
Summary
TestFindRenderdocFallback::test_import_failed_without_module_leaves_no_diagnosticfailed whenever the test environment had a real renderdoc module reachable throughRENDERDOC_PYTHON_PATH(whichpixi.tomlinjects as.local/renderdoc). Afterpixi run syncpopulates that directory,find_renderdoc()legitimately picks it up as a candidate,_has_renderdoc_module()returns true for it, and a diagnostic is set — breaking the test's "no diagnostic" assertion. The product code is correct; the test lacked environment isolation.This explains the intermittent failures observed across CI-adjacent and dev environments: the test passed in checkouts that had never run
pixi run syncand failed in ones that had.Fix (test-side only,
discover.pyuntouched)TestFindRenderdocFallbacktests now clearRENDERDOC_PYTHON_PATHviamonkeypatch.delenv, pinning their candidate lists to exactly what they mock.test_import_failed_with_env_module_sets_diagnosticcovers the inverse: withRENDERDOC_PYTHON_PATHpointing at a directory that does contain arenderdoc.sowhose import fails, the diagnostic must be populated with that path. Deterministic in both environment states.Verification
renderdoc.sounder.local/renderdoc): base test file reproduces the failure; with this change, all 18 tests pass.RENDERDOC_PYTHON_PATHunset): 18 passed.Summary by CodeRabbit
Release Notes
Documentation
Tests