Add Ninja snapshot test for command_available expansion (#310) - #360
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: 3 reviews are currently available. Based on recent review activity, included reviews refill at 4 per hour. Summary
WalkthroughAdd configurable manifest parsing with injected environment and standard-library configuration. Add an end-to-end Ninja snapshot test for ChangesManifest configuration and Ninja coverage
Possibly related PRs
Poem
Merge Risk: ⚪ Minimal · up to This PR adds a localized deterministic Ninja snapshot test and related documentation; no actionable merge-blocking risk remains after normal checks and review. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (8 inconclusive)
✅ Passed checks (12 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds a new deterministic Ninja snapshot test to validate File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
5aebb0b to
6eff68e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6eff68ec38
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ninja_snapshot_tests.rs`:
- Around line 135-145: Update the manifest expansion setup in the ninja snapshot
test to make the unavailable-command fixture independent of the host PATH.
Configure the relevant StdlibConfig using with_path_override with a controlled
path, or use an absent temporary direct path, while preserving the existing
command_available conditions and avoiding process-environment mutation.
🪄 Autofix
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: f6c7d451-1265-4c3f-bc8b-1d84a829ddbf
⛔ Files ignored due to path filters (1)
tests/snapshots/ninja/ninja_snapshot_tests__command_available_manifest_ninja.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
tests/ninja_snapshot_tests.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
6eff68e to
7a5b507
Compare
PR #309 introduced the `command_available` stdlib predicate but no snapshot pinned the generated Ninja output for the path where a `when: command_available(...)` / `when: not command_available(...)` pair filters top-level actions before `ninja_gen::generate`. Add `command_available_manifest_ninja_snapshot`, modelled on `conditional_manifest_ninja_snapshot`: the manifest guards two complementary actions on a command name guaranteed absent (with `cwd_mode="never"` for determinism), asserts the fallback action is present and the preferred action absent, and snapshots the Ninja output to `tests/snapshots/ninja`.
The fixture relied on a command name guaranteed absent, but
`command_available` still searches every directory on the host PATH even
with `cwd_mode="never"`; a developer or CI image carrying a binary named
`netsuke-command-that-should-not-exist` would select the preferred action
and break both the assertions and the snapshot.
Add a public `manifest::from_str_with_env_and_config` entrypoint so a test
can inject a full `StdlibConfig` alongside the existing `EnvReader` seam,
then pin the resolver to an empty PATH with
`StdlibConfig::from_current_dir()?.with_path_override("")`. With
`cwd_mode="never"` the workspace fallback is disabled too, so the
expansion is deterministic on every host without touching the process
environment. The new entrypoint lives in `src/manifest/parse_with_config.rs`
to keep `src/manifest/mod.rs` inside the 400-line Whitaker limit.
Document the seam in the developers-guide injected-environment list.
Co-Authored-By: Claude <noreply@anthropic.com>
8ea268a to
3315340
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
Closes #310
Adds the snapshot test proposed in the issue:
command_available_manifest_ninja_snapshotintests/ninja_snapshot_tests.rs, following the structure ofconditional_manifest_ninja_snapshot.when: command_available("netsuke-command-that-should-not-exist", cwd_mode="never")and its negation, plus a simpletargetssection.manifest::from_str→BuildGraph::from_manifest→ninja_gen::generate.ensure!assertions confirm the fallback action is present and the preferred action absent.tests/snapshots/ninjaascommand_available_manifest_ninja; the committed.snapshows the fallback rule/build edge and the unconditional target, with nopreferredcontent.Deterministic with no real binary on
PATH(absent command name +cwd_mode="never").Validation
cargo test --test ninja_snapshot_tests— 4 passedmake check-fmt/make lint/make test— pass (37 suites)🤖 Generated with Claude Code
Summary by Sourcery
Tests:
References