test(admin-cli): enumerate health-report template behavior#3966
Conversation
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughHealth report template tests were consolidated into deterministic projected assertions. A table-driven test now covers every ChangesHealth report test refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/admin-cli/src/machine/health_report/cmd.rs (1)
282-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPositional string parameters in
expected_reportare a misuse footgun.
source,alert_id,target, andmessageare all string-shaped positional parameters with no type distinction, so a future accidental swap (e.g., oftargetandmessage) at one of the 10 call sites in the next test will compile silently and only surface as a same-shaped assertion mismatch. As per coding guidelines, designs should be "hard to misuse," and struct literals are preferred for plain data. Consider a small named-field struct or builder for the expected values instead of five positional args.♻️ Example refactor using a named-field struct
- fn expected_report( - source: &str, - alert_id: &str, - target: Option<&str>, - message: &str, - classifications: &[&str], - ) -> ReportProjection { + struct ExpectedAlert<'a> { + source: &'a str, + alert_id: &'a str, + target: Option<&'a str>, + message: &'a str, + classifications: &'a [&'a str], + } + + fn expected_report(expected: ExpectedAlert) -> ReportProjection { ReportProjection { - source: source.to_string(), + source: expected.source.to_string(), triggered_by: None, observed_at_present: true, successes: vec![], alerts: vec![AlertProjection { - id: alert_id.to_string(), - target: target.map(str::to_string), + id: expected.alert_id.to_string(), + target: expected.target.map(str::to_string), in_alert_since_present: false, - message: message.to_string(), + message: expected.message.to_string(), tenant_message: None, - classifications: classifications + classifications: expected + .classifications .iter() .map(|classification| (*classification).to_string()) .collect(), }], } }🤖 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 `@crates/admin-cli/src/machine/health_report/cmd.rs` around lines 282 - 306, Refactor the test helper expected_report to accept a named-field input struct or equivalent builder instead of positional source, alert_id, target, message, and classifications arguments. Update all expected_report call sites to use the named fields, preserving the resulting ReportProjection values and existing assertion behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/admin-cli/src/machine/health_report/cmd.rs`:
- Around line 282-306: Refactor the test helper expected_report to accept a
named-field input struct or equivalent builder instead of positional source,
alert_id, target, message, and classifications arguments. Update all
expected_report call sites to use the named fields, preserving the resulting
ReportProjection values and existing assertion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 67ce0a06-6e08-4a47-a649-aebb9a0d8a3c
📒 Files selected for processing (1)
crates/admin-cli/src/machine/health_report/cmd.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/admin-cli/src/machine/health_report/cmd.rs (1)
318-490: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the cases from the enum. This test hand-lists every
HealthReportTemplatesvariant; if the enum is expected to grow, drive the scenarios from a single source of truth (for exampleEnumIteror a small exhaustive helpermatch) so new variants cannot be added without updating coverage.🤖 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 `@crates/admin-cli/src/machine/health_report/cmd.rs` around lines 318 - 490, The health_report_templates test manually duplicates every HealthReportTemplates variant, allowing new variants to miss coverage. Replace the hand-built cases with enum-derived iteration or an exhaustive helper match, while retaining each variant’s existing expected report data and ensuring adding a new enum variant requires updating the test’s single source of truth.
🤖 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.
Nitpick comments:
In `@crates/admin-cli/src/machine/health_report/cmd.rs`:
- Around line 318-490: The health_report_templates test manually duplicates
every HealthReportTemplates variant, allowing new variants to miss coverage.
Replace the hand-built cases with enum-derived iteration or an exhaustive helper
match, while retaining each variant’s existing expected report data and ensuring
adding a new enum variant requires updating the test’s single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9b5a6a8f-de6c-4700-9a1b-cd69b9ca6794
📒 Files selected for processing (1)
crates/admin-cli/src/machine/health_report/cmd.rs
Replace nine overlapping template tests with one labeled table that compares every stable report field. Cover all eleven template variants and keep the empty report constructor as a separate structural contract. Record conversion parity before taking both constructors to full line and region coverage. This supports NVIDIA#3964 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Addressed the review feedback in e622bbe: expected report fields are named, the table is checked against every derived ValueEnum variant with readable failures, and the projections destructure each source struct so new fields cannot be skipped silently. @coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
1 participant
Health-report template tests covered only five of the eleven constructor variants and repeated partial message and classification assertions across nine standalone tests. The six remaining match arms and the separate empty-template constructor had no direct coverage.
This replaces those overlapping tests with one labeled
check_valuestable that projects every stable report field. The table owns all elevenHealthReportTemplatesvariants, while one focused structural test ownsget_empty_template.Related issues
Supports #3964.
Type of Change
Breaking Changes
Testing
Additional Notes
Coverage was measured from the same base at original, conversion-only, and expanded checkpoints:
The raw test count moved from 400 to 392 at the conversion checkpoint because nine overlapping tests became one table, then to 393 when the empty-template contract was added.
Validation:
cargo +1.96.0 test --locked -p nico-admin-cli --bin nico-admin-cli machine::health_report::cmd::testsnico-admin-clicoverage run: 393 tests passedcargo make format-nightlycargo make clippycargo make carbide-lints