test(health): cover inventory and entity metric projections#4080
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: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Summary by CodeRabbit
WalkthroughAdds an in-memory Axum Redfish fixture and expands health collector tests for inventory projections, child metrics, event emission, sparse resources, missing links, empty resources, and malformed responses. ChangesHealth projection test coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇✅ ✅ Action performedFull review finished. |
![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/health/src/collectors/projection_test_support.rs (1)
201-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an explicit enum for the metrics variant to close a silent-misconfiguration gap.
The
attributesbuilder attaches aMetricslink for anySome(_), yet only the"empty"and"malformed"arms materialize the backing resource. A future typo'd tag would fall throughSome(_) => {}, leaving a dangling link that 404s at fetch time and quietly alters the scenario under test. Modeling the finite set as an enum makes thematchexhaustive and removes the stringly-typed coupling.♻️ Sketch: replace the `Option<&str>` tag with an enum
enum SparseMetrics { None, Empty, Malformed, } for (id, metrics) in [ ("CPU-sparse", SparseMetrics::None), ("CPU-empty", SparseMetrics::Empty), ("CPU-malformed", SparseMetrics::Malformed), ] { let path = processor(id); let metrics_path = format!("{path}/Metrics"); let attributes = match metrics { SparseMetrics::None => json!({}), _ => json!({ "Metrics": reference(&metrics_path) }), }; // ...insert resource... match metrics { SparseMetrics::Empty => { /* insert empty metrics */ } SparseMetrics::Malformed => { resources.insert(metrics_path, MockResponse::Malformed); } SparseMetrics::None => {} } }As per coding guidelines: "Represent finite sets of values with enums or enum-backed structs ... rather than passing stringly typed values."
🤖 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/health/src/collectors/projection_test_support.rs` around lines 201 - 240, Replace the Option<&str> metrics tags in the test fixture loop with a local enum representing None, Empty, and Malformed. Update the attributes construction and resource-materialization match statements to match exhaustively on that enum, preserving the existing links and backing-resource behavior without a wildcard arm.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/health/src/collectors/projection_test_support.rs`:
- Around line 201-240: Replace the Option<&str> metrics tags in the test fixture
loop with a local enum representing None, Empty, and Malformed. Update the
attributes construction and resource-materialization match statements to match
exhaustively on that enum, preserving the existing links and backing-resource
behavior without a wildcard arm.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c4e8d6d5-2bf8-496a-b3c5-bbab17a460fb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/health/Cargo.tomlcrates/health/src/collectors/entity_metrics.rscrates/health/src/collectors/inventory.rscrates/health/src/collectors/mod.rscrates/health/src/collectors/projection_test_support.rs
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/collectors/entity_metrics.rs (1)
570-601: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider preserving multiplicity/order in the projection assertions.
projectfolds the producedVec<MetricField>into aBTreeMapkeyed solely bymetric_type. This silently collapses any duplicatemetric_typeand discards emission order, so a regression that emitted the same metric twice (or reordered fields) would passchild_metric_projection_casesundetected. Since the production path returns aVec, a sortedVec<(String, &'static str, f64)>would retain that fidelity while remaining deterministic.🤖 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/health/src/collectors/entity_metrics.rs` around lines 570 - 601, Update the test helper project and matching expected helper to return sorted Vec<(String, &'static str, f64)> values instead of BTreeMap keyed by metric_type. Preserve every emitted field, including duplicates, while sorting both actual and expected results deterministically so child_metric_projection_cases detects multiplicity and ordering regressions.
🤖 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/health/src/collectors/entity_metrics.rs`:
- Around line 570-601: Update the test helper project and matching expected
helper to return sorted Vec<(String, &'static str, f64)> values instead of
BTreeMap keyed by metric_type. Preserve every emitted field, including
duplicates, while sorting both actual and expected results deterministically so
child_metric_projection_cases detects multiplicity and ordering regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3225d988-6518-4556-ab83-0ec9e95d75c2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/health/Cargo.tomlcrates/health/src/collectors/entity_metrics.rscrates/health/src/collectors/inventory.rscrates/health/src/collectors/mod.rscrates/health/src/collectors/projection_test_support.rs
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/health/src/collectors/projection_test_support.rs (1)
58-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a combined
insert_resourcehelper to eliminate the duplicated path argument.Throughout
mock_resources, each self-describing resource passes its path twice — once as the map key toinsertand once as the@odata.idinsideresource. A silent divergence between the two would break link resolution in a way that is tedious to diagnose. Folding both into a single helper makes the key and self-link authoritatively identical.♻️ Proposed helper to keep key and self-link in lockstep
fn insert_resource( resources: &mut HashMap<String, MockResponse>, path: &str, odata_type: &str, id: &str, name: &str, attributes: Value, ) { insert(resources, path, resource(path, odata_type, id, name, attributes)); }Call sites then collapse from
insert(&mut resources, SYSTEM, resource(SYSTEM, ...))toinsert_resource(&mut resources, SYSTEM, ...).🤖 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/health/src/collectors/projection_test_support.rs` around lines 58 - 90, Combine the existing resource construction and insertion flow by adding an `insert_resource` helper alongside `resource` and `insert`; it should accept the shared path and resource fields, build the value with `resource(path, ...)`, then insert it under that same path. Update self-describing resource call sites in `mock_resources` to use `insert_resource`, while leaving standalone `insert` and reference handling unchanged.
🤖 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/health/src/collectors/projection_test_support.rs`:
- Around line 58-90: Combine the existing resource construction and insertion
flow by adding an `insert_resource` helper alongside `resource` and `insert`; it
should accept the shared path and resource fields, build the value with
`resource(path, ...)`, then insert it under that same path. Update
self-describing resource call sites in `mock_resources` to use
`insert_resource`, while leaving standalone `insert` and reference handling
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4fc5abb6-71a3-47c4-8d13-fbdf0f560b4d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/health/Cargo.tomlcrates/health/src/collectors/entity_metrics.rscrates/health/src/collectors/inventory.rscrates/health/src/collectors/mod.rscrates/health/src/collectors/projection_test_support.rs
Health's inventory projection had no direct coverage, and the child metric tests stopped before `MetricsCollector::collect_entity`. So, this replaces five standalone entries with one table and adds owner tables for every `DiscoveredEntity` variant plus representative collection behavior. A `#[cfg(test)]` in-process Redfish fixture lets the same rows cover populated, sparse, null, malformed, ignored, and sinkless paths without repeating setup. Selected production coverage moves from 7/30 to 30/30 functions, 104/339 to 335/339 lines, and 129/534 to 509/534 regions. The health library now has 403 test entries instead of 405, with 32 labeled cases across the three tables. Tests updated! This supports NVIDIA#4075 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇🔎 ✅ Action performedFull review finished. |
2 participants
Health's inventory projection had no direct coverage, and the child metric tests stopped before
MetricsCollector::collect_entity.So, this replaces five standalone entries with one table and adds owner tables for every
DiscoveredEntityvariant plus representative collection behavior. A#[cfg(test)]in-process Redfish fixture lets the same rows cover populated, sparse, null, malformed, ignored, and sinkless paths without repeating setup.Related issues
This supports #4075
Type of Change
Breaking Changes
Testing
cargo test -p carbide-health --libcargo make format-nightlycargo make clippycargo make carbide-lintsAdditional Notes
Production behavior is unchanged. The selected projection code now measures:
The four uncovered source lines are three repeated generic miss arms for the non-processor entity types and one lazy tracing field. Processor rows already exercise the missing-resource and malformed-fetch behavior. Sensor projection remains separate for the next focused change.