Make Whitaker fallback tests hermetic (#53)#54
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesSubprocess environment isolation
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (18 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Strip inherited make and Whitaker resolution variables from generated-project lint subprocesses so the fallback cases exercise the intended PATH, HOME, and missing-tool branches regardless of the parent test environment. Include captured stdout and stderr in fallback assertions so future resolution failures report the selected Whitaker command directly.
7584444 to
b062f30
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b062f30234
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MAKE_RESOLUTION_VARIABLES = frozenset( | ||
| { | ||
| "WHITAKER", | ||
| "MAKEFLAGS", | ||
| "MFLAGS", | ||
| "MAKELEVEL", | ||
| } |
There was a problem hiding this comment.
GNU make also reads GNUMAKEFLAGS: the manual says it is parsed just before MAKEFLAGS in the same way (https://www.gnu.org/software/make/manual/html_node/Options_002fRecursion.html). Because this helper copies it through, a parent shell with GNUMAKEFLAGS=WHITAKER=/path/to/fake or an equivalent --eval assignment still overrides the generated Makefile before the WHITAKER ?= fallback, so the whitaker_location == "missing" case can run that binary and pass instead of exercising the missing-tool branch. Please strip GNUMAKEFLAGS alongside the other make-control variables.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_template/test_lint_targets.py`:
- Around line 96-103: Update the test assertions around the generated lint
target execution to verify that result.stdout contains “Whitaker binary:
{expected_whitaker}” for cases where WHITAKER is present, using the resolved
expected path or home-directory value. Keep the existing return-code and marker
assertions, but ensure the test distinguishes the intended WHITAKER resolution
from the bare whitaker fallback.
- Around line 71-75: Update the PATH setup in the test targeting logic to use an
isolated temporary tool directory for the home and missing cases instead of
/usr/bin:/bin, while retaining path_bin for the path case. Ensure any required
test utilities are available in that sandbox or rely on shell built-ins so
host-installed whitaker cannot affect the intended branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18c89b8e-1163-42a7-862e-ee029f3b1fe8
📒 Files selected for processing (2)
tests/helpers/subprocess_env.pytests/test_template/test_lint_targets.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/whitaker(auto-detected)
| "PATH": os.pathsep.join( | ||
| [str(path_bin), "/usr/bin", "/bin"] | ||
| if whitaker_location == "path" | ||
| else ["/usr/bin", "/bin"] | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove host tool directories from the fallback PATH.
Use an isolated temporary tool directory for the home and missing cases instead of /usr/bin:/bin. The generated Makefile checks command -v whitaker first, so a host-installed binary can win and prevent the test from exercising the intended branch. Provide required test utilities inside the sandbox or use shell built-ins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_template/test_lint_targets.py` around lines 71 - 75, Update the
PATH setup in the test targeting logic to use an isolated temporary tool
directory for the home and missing cases instead of /usr/bin:/bin, while
retaining path_bin for the path case. Ensure any required test utilities are
available in that sandbox or rely on shell built-ins so host-installed whitaker
cannot affect the intended branch.
| assert result.returncode == 0, ( | ||
| f"stdout:\n{result.stdout}\n" | ||
| f"stderr:\n{result.stderr}" | ||
| ) | ||
| assert marker.exists(), ( | ||
| f"expected generated lint target to execute {whitaker_location} Whitaker" | ||
| f"expected generated lint target to execute {whitaker_location} Whitaker\n" | ||
| f"stdout:\n{result.stdout}\n" | ||
| f"stderr:\n{result.stderr}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the resolved WHITAKER value.
Assert that stdout contains Whitaker binary: {expected_whitaker} for present cases. The recipe prepends $(USER_BIN_PATH) to PATH, so merely checking the marker can pass even when Make selected the bare whitaker fallback rather than the intended path or home-directory resolution.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_template/test_lint_targets.py` around lines 96 - 103, Update the
test assertions around the generated lint target execution to verify that
result.stdout contains “Whitaker binary: {expected_whitaker}” for cases where
WHITAKER is present, using the resolved expected path or home-directory value.
Keep the existing return-code and marker assertions, but ensure the test
distinguishes the intended WHITAKER resolution from the bare whitaker fallback.
Summary
This branch makes the generated-project Whitaker fallback tests hermetic by stripping inherited
WHITAKER,MAKEFLAGS,MFLAGS, andMAKELEVELvalues before invokingmake lint. It preserves the template Makefile behaviour and keeps the fallback cases focused on the intendedPATH,$HOME/.local/bin, and missing-tool branches.Closes #53.
Review walkthrough
Validation
WHITAKER=true uvx --with pytest-copier --with pyyaml --with syrupy --with make-parser --with hypothesis pytest -vv tests/test_template/test_lint_targets.py::test_makefile_resolves_whitaker_fallback: 3 passed.uvx --with pytest-copier --with pyyaml --with syrupy --with make-parser --with hypothesis pytest tests/: 50 passed, 1 skipped.coderabbit review --agent: first pass reported one docstring concern, which was fixed.coderabbit review --agent: second pass completed with 0 findings.Notes
No template or snapshot changes were needed because the failure was caused by inherited environment state in the test subprocess, not by the generated Makefile fallback chain.
References