Snapshot localized CLI help output (#290)#326
Open
lodyai[bot] wants to merge 3 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 15 minutes and 54 seconds. Learn how PR review limits work. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds test harness wiring in ChangesCLI Help Snapshot Tests
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (16 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideAdds a parameterized, locale-aware CLI help snapshot test to ensure the localized help text includes the --config flag and its description, and introduces insta snapshots for en-US and es-ES outputs. Sequence diagram for localized CLI help snapshot testsequenceDiagram
participant Test as localized_help_snapshots_include_config_flag
participant Localizer as build_localizer
participant Cli as Cli_command
participant Localized as localize_command
participant Normalizer as normalize_fluent_isolates
participant Settings as snapshot_settings
participant Insta as assert_snapshot
Test->>Localizer: build_localizer(locale)
Localizer-->>Test: localizer
Test->>Cli: Cli::command()
Cli-->>Test: command
Test->>Localized: localize_command(command, localizer)
Localized-->>Test: localized_command
Test->>Localized: render_long_help()
Localized-->>Test: rendered_help
Test->>Normalizer: normalize_fluent_isolates(rendered_help)
Normalizer-->>Test: normalized_help
Test->>Test: assert contains("--config <FILE>")
Test->>Test: assert contains(config_help)
Test->>Settings: snapshot_settings("cli")
Settings-->>Test: settings
Test->>Settings: bind()
activate Settings
Settings->>Insta: assert_snapshot(snapshot_name, normalized_help)
deactivate Settings
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
issue-290-snapshot-test-for-help-output
branch
from
f209922 to
ec8d068
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
issue-290-snapshot-test-for-help-output
branch
from
ec8d068 to
f782af9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add `insta` coverage for localized top-level help in `en-US` and `es-ES`. Assert the `--config` flag and resolved Fluent description before snapshotting the full rendered help output so future CLI or localisation drift is caught in review.
lodyai
Bot
force-pushed
the
issue-290-snapshot-test-for-help-output
branch
from
f782af9 to
7888588
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/cli/parser_tests.rs`:
- Around line 31-38: The two assertions in parser_tests.rs use a literal
"{locale}" instead of interpolating the locale variable; update both assertion
messages to format the locale (e.g., use "localized help for {} should include
the config flag" and pass locale as the format argument) so failures show the
actual locale value; change the messages associated with the assertions checking
normalized_help.contains("--config <FILE>") and
normalized_help.contains(config_help) to include locale via formatting.
🪄 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: 2ef37420-11f7-4fc0-b8b6-bf0de02b65ad
⛔ Files ignored due to path filters (2)
src/snapshots/cli/netsuke__cli__parser__tests__help_en_us.snapis excluded by!**/*.snapsrc/snapshots/cli/netsuke__cli__parser__tests__help_es_es.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/cli/parser.rssrc/cli/parser_tests.rs
Owner
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Expand the parser snapshot test module documentation so its purpose and coverage are clear, and make assertion failures report the locale under test. Keep the change scoped to the still-valid snapshot-test diagnostics feedback.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cli/parser_tests.rs (1)
31-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix assertion message formatting to interpolate the locale variable.
The assertion messages use
{locale}within the string literal but do not passlocaleas a format argument. When the assertion fails, the error message will display{locale}literally rather than showing which locale failed (en-US or es-ES). Fix both assertions to properly interpolate the variable for clearer test diagnostics.Proposed fix
assert!( normalized_help.contains("--config <FILE>"), - "localised help for {locale} should include the config flag" + "localised help for {} should include the config flag", + locale ); assert!( normalized_help.contains(config_help), - "localised help for {locale} should include the config flag description" + "localised help for {} should include the config flag description", + locale );🤖 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 `@src/cli/parser_tests.rs` around lines 31–38, the two assertions use a literal `{locale}` instead of interpolating the locale variable; update both assertion messages to format the locale (e.g., use "localised help for {} should include the config flag" and pass locale as the format argument) so failures show the actual locale value; change the messages associated with the assertions checking `normalized_help.contains("--config <FILE>")` and `normalized_help.contains(config_help)` to include locale via formatting.🤖 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 `@src/cli/parser_tests.rs` around lines 31 - 38, The two assert macro calls in the config flag validation section are using literal `{locale}` text in the assertion messages instead of properly interpolating the locale variable. Fix both assertions by changing the message strings to use Rust format string syntax with `{}` placeholders and passing the `locale` variable as an argument to the format macro (e.g., change the first assertion's message from "localized help for {locale} should include the config flag" to "localized help for {} should include the config flag" and add locale as the format argument). Apply the same fix to the second assertion checking the config_help description so that test failures will display the actual locale value rather than the literal string {locale}.
🤖 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.
Duplicate comments:
In `@src/cli/parser_tests.rs`:
- Around line 31-38: The two assert macro calls in the config flag validation
section are using literal `{locale}` text in the assertion messages instead of
properly interpolating the locale variable. Fix both assertions by changing the
message strings to use Rust format string syntax with `{}` placeholders and
passing the `locale` variable as an argument to the format macro (e.g., change
the first assertion's message from "localized help for {locale} should include
the config flag" to "localized help for {} should include the config flag" and
add locale as the format argument). Apply the same fix to the second assertion
checking the config_help description so that test failures will display the
actual locale value rather than the literal string {locale}.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 74e10f1f-7080-40b3-ac11-307d60384384
⛔ Files ignored due to path filters (2)
src/snapshots/cli/netsuke__cli__parser__tests__help_en_us.snapis excluded by!**/*.snapsrc/snapshots/cli/netsuke__cli__parser__tests__help_es_es.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
src/cli/parser.rssrc/cli/parser_tests.rs
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
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 `@src/cli/parser_tests.rs`:
- Around line 34-36: Remove the intermediate variable assignments for
missing_config_flag and missing_config_description that are created only to be
used once in assertions. Instead, inline the format! calls directly into the
assertion macros where these strings are used. Apply this same refactoring
pattern to the other occurrences at lines 40 and 44, eliminating intermediate
variables and using format arguments directly within the assertion macro
invocations for more idiomatic Rust code.
🪄 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: e8a2b12c-c1c6-49fd-9b8f-d3e2c6966790
📒 Files selected for processing (1)
src/cli/parser_tests.rs
Owner
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Remove one-use formatted strings from the localized help snapshot test so the assertion messages use Rust's inline format capture directly.
Owner
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (3 errors, 1 warning)
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current content of parser.rs around the mod tests declaration
cat src/cli/parser.rs | tail -20
echo "---"
# Check full content of parser_tests.rs
cat src/cli/parser_tests.rsLength of output: 2401 🏁 Script executed: #!/bin/bash
# Search for any existing issue covering unit architecture / snapshot I/O side-effects
gh issue list --repo leynos/netsuke --state open --search "snapshot architecture side-effect" --json number,title,url
gh issue list --repo leynos/netsuke --state open --search "unit architecture insta" --json number,title,urlLength of output: 375 🏁 Script executed: #!/bin/bash
gh issue create \
--repo leynos/netsuke \
--title "Separate snapshot persistence from query logic in parser rendering tests" \
--body "## Background
In PR `#326`, snapshot regression tests were added for localised CLI long-help output (\`src/cli/parser_tests.rs\`). The tests currently embed \`insta::assert_snapshot!\` directly inside the test body, which causes irreversible filesystem I/O (reading or writing snapshot files) as an indistinguishable side-effect of what should be a pure query operation.
## Problem
\`insta\`'s \`assert_snapshot!\` macro unconditionally performs filesystem mutations during test execution. There is no clear boundary between:
- the **query path** – rendering, normalising, and asserting on the help text; and
- the **command path** – persisting or verifying the snapshot on disk.
This violates the CQS principle and makes test intent harder to reason about.
## Proposed resolution
Refactor the snapshot tests to separate concerns explicitly:
1. Extract a pure helper function that returns the normalised rendered help string for a given locale.
2. Assert on the returned string in unit-style tests (no I/O).
3. Invoke \`assert_snapshot!\` only from a clearly labelled snapshot-acceptance layer, or use \`insta\`'s explicit \`Settings\` API (\`Settings::bind\`, \`Settings::set_snapshot_path\`, etc.) in a dedicated snapshot-acceptance test function that is clearly named and documented as having filesystem side-effects.
## References
- PR `#326`: https://github.com/leynos/netsuke/pull/326
- Raised by `@leynos`
" \
--assignee leynosLength of output: 1682
Resolution summary
Check 1 — Module-Level Documentation: agent promptApply this change to Check 2 — Unit Architecture: deferred to
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch adds regression coverage for issue #290 by snapshotting the
localised top-level CLI help output in
en-USandes-ES. It directlyasserts that
--config <FILE>is present with the Fluent-resolvedconfiguration-file description before storing the full rendered help text as
instasnapshots.Closes #290.
Review walkthrough
Validation
cargo test localized_help_snapshots_include_config_flag --all-targets --all-features: passed.make check-fmt: passed.make lint: passed.make test: passed.Notes
coderabbit review --agentwas attempted after the focused snapshot test, but the command stalled after reporting sandbox setup. The stalled process group from this worktree was stopped; no CodeRabbit findings were returned.gh issue edit 290 --add-label lowfailed withGraphQL: Resource not accessible by integration, so the issue label could not be applied from this environment.Summary by Sourcery
Add regression coverage to ensure localized CLI help output includes the configuration flag and is snapshotted for specific locales.
Tests: