test: convert the mechanical tables and finish the silent-test sweep - #4230
Merged
Conversation
`carbide-test-support` gives us two ways to write a table test -- the explicit `Case{}`/`Check{}` array and the `scenarios!`/`value_scenarios!` macros -- and the macro form is roughly a third of the lines per row. Where the row's input reads as well as its hand-written label, the explicit form is just ceremony, and three of the label-mapping tables here were exactly that: a `FirmwareComponentType` or a `MethodLabel` on one side, its snake_case string on the other.
The narrowness is the point, though. `scenarios!` builds its label as `concat!(group, " / ", stringify!(input))`, so a row whose input is a multi-field struct comes back with the whole literal as its label -- worse than the prose it replaced. Of the 1,839 explicit rows in the tree, 658 are that case and another 756 have a label saying something the input never will (`"punctuation is normalized"` is not derivable from `"a,b"`). Only about 360 are safe to move, and they thin out to two or three per file after the handful converted here. I left the rest alone.
Primary callouts are:
- Three label tables collapse: `power_operation_label_covers_every_system_power_control` (52 lines to 19), `firmware_component_label_renders_snake_case` (62 to 20), and `label_values_render_as_snake_case` in `bmc-proxy` (77 to 27). Same rows, same assertions, same order -- and `stringify!` gives a *better* label here than the prose did, since `PowerOperation::from(SystemPowerControl::GracefulShutdown)` says more than "graceful shutdown".
- The three `dpf/sdk.rs` tests left over from NVIDIA#4190 now assert something. All three drive the branch where an existing, non-terminating resource is deliberately left alone -- and all three checked only that no error came back, so a call that quietly created a duplicate would have passed. They now assert the mock still holds exactly one device, node, or flavor. `create_dpu_flavor` also returns the existing name, which is the whole idempotent-reuse contract and was being discarded.
Coverage A/B over `preingestion-manager`, `bmc-proxy` and `dpf`, baselined on the same `06495cda5` this branch sits on: **1,748 production functions covered before and after, exactly flat**, with every test still passing. That's the result you want from a conversion -- the rows and their assertions are unchanged, only how they're written is. The `dpf` half tightens assertions on paths that were already being executed, so it doesn't move the function-level number either.
Worth noting for anyone converting more of these: `label_value()` returns `opentelemetry::StringValue`, and the explicit form was inferring that through `check_values`. The macro form needs it spelled out on the `run =` closure.
Tests updated!
This supports NVIDIA#4224
Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇 ✅ Action performedFull review finished. |
Contributor
Summary by CodeRabbit
WalkthroughThe changes modernize metric label tests with ChangesMetric label test modernization
DPF SDK reuse-path assertions
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 `@crates/dpf/src/sdk.rs`:
- Around line 3374-3378: Update the no-op reuse tests to observe repository
create calls rather than relying only on final collection sizes: at
crates/dpf/src/sdk.rs lines 3374-3378, record and assert zero device-create
calls; at lines 3456-3458, record and assert zero node-create calls; and at
lines 3626-3631, record and assert zero flavor-create calls. Use the existing
mock call-tracking mechanisms and preserve the current no-op assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b433faf0-624c-4a6e-ad0e-b5e545a84bb5
📒 Files selected for processing (3)
crates/bmc-proxy/src/metrics.rscrates/dpf/src/sdk.rscrates/preingestion-manager/src/metrics.rs
Comment on lines
+3374
to
+3378
|
|
||
| // This branch is a deliberate no-op: an existing, non-terminating device is left | ||
| // alone. `.unwrap()` only said no error came back -- assert no second device was | ||
| // created alongside it, which is the whole of what "left alone" means here. | ||
| assert_eq!(mock.devices.read().unwrap().len(), 1); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify reuse by observing repository calls, not only final state.
The mock’s create methods reject existing keys with AlreadyExists, so unchanged store sizes do not prove that the no-op branches avoided duplicate creation.
crates/dpf/src/sdk.rs#L3374-L3378: record and assert zero device-create calls.crates/dpf/src/sdk.rs#L3456-L3458: record and assert zero node-create calls.crates/dpf/src/sdk.rs#L3626-L3631: record and assert zero flavor-create calls.
📍 Affects 1 file
crates/dpf/src/sdk.rs#L3374-L3378(this comment)crates/dpf/src/sdk.rs#L3456-L3458crates/dpf/src/sdk.rs#L3626-L3631
🤖 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/dpf/src/sdk.rs` around lines 3374 - 3378, Update the no-op reuse tests
to observe repository create calls rather than relying only on final collection
sizes: at crates/dpf/src/sdk.rs lines 3374-3378, record and assert zero
device-create calls; at lines 3456-3458, record and assert zero node-create
calls; and at lines 3626-3631, record and assert zero flavor-create calls. Use
the existing mock call-tracking mechanisms and preserve the current no-op
assertions.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/dpf/src/sdk.rs (1)
3374-3378: 🎯 Functional Correctness | 🟡 MinorVerify reuse through repository-call observations.
The three tests should assert zero create calls, because the mock rejects duplicate keys without tracking invocation; unchanged store sizes can therefore hide duplicate-create attempts.
crates/dpf/src/sdk.rs#L3374-L3378: record and assert zero device-create calls.crates/dpf/src/sdk.rs#L3456-L3458: record and assert zero node-create calls.crates/dpf/src/sdk.rs#L3626-L3631: record and assert zero flavor-create calls.🤖 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/dpf/src/sdk.rs` around lines 3374 - 3378, Update the reuse tests in crates/dpf/src/sdk.rs at lines 3374-3378, 3456-3458, and 3626-3631 to record repository create-call invocations and assert zero calls for device, node, and flavor creation respectively, rather than relying only on unchanged store sizes. Ensure each test verifies the existing resource was reused without attempting a duplicate create.
🤖 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 `@crates/dpf/src/sdk.rs`:
- Around line 3374-3378: Update the reuse tests in crates/dpf/src/sdk.rs at
lines 3374-3378, 3456-3458, and 3626-3631 to record repository create-call
invocations and assert zero calls for device, node, and flavor creation
respectively, rather than relying only on unchanged store sizes. Ensure each
test verifies the existing resource was reused without attempting a duplicate
create.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e2ea7fa2-762d-47d9-9d02-d6cf9f04a8d5
📒 Files selected for processing (3)
crates/bmc-proxy/src/metrics.rscrates/dpf/src/sdk.rscrates/preingestion-manager/src/metrics.rs
wminckler
approved these changes
Jul 28, 2026
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
2 participants
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.
carbide-test-supportgives us two ways to write a table test -- the explicitCase{}/Check{}array and thescenarios!/value_scenarios!macros -- and the macro form is roughly a third of the lines per row. Where the row's input reads as well as its hand-written label, the explicit form is just ceremony, and three of the label-mapping tables here were exactly that: aFirmwareComponentTypeor aMethodLabelon one side, its snake_case string on the other.The narrowness is the point, though.
scenarios!builds its label asconcat!(group, " / ", stringify!(input)), so a row whose input is a multi-field struct comes back with the whole literal as its label -- worse than the prose it replaced. Of the 1,839 explicit rows in the tree, 658 are that case and another 756 have a label saying something the input never will ("punctuation is normalized"is not derivable from"a,b"). Only about 360 are safe to move, and past the handful converted here they thin out to two or three per file. I left the rest alone rather than churn 70-odd files for the tail.power_operation_label_covers_every_system_power_control(52 lines to 19),firmware_component_label_renders_snake_case(62 to 20), andlabel_values_render_as_snake_caseinbmc-proxy(77 to 27). Same rows, same assertions, same order -- andstringify!gives a better label here than the prose did, sincePowerOperation::from(SystemPowerControl::GracefulShutdown)says more than"graceful shutdown".dpf/sdk.rstests left over from Add Missing Assertions To Silent Tests #4190 now assert something. All three drive the branch where an existing, non-terminating resource is deliberately left alone, and all three checked only that no error came back -- so a call that quietly created a duplicate would have passed. They now assert the mock still holds exactly one device, node, or flavor.create_dpu_flavoralso returns the existing name, which is the whole idempotent-reuse contract and was being discarded.Related issues
This supports #4224
Type of Change
Breaking Changes
Testing
Function-level coverage A/B over
preingestion-manager,bmc-proxyanddpf, baselined on the same06495cda5this branch sits on:Exactly flat, which is the result you want from a conversion -- the rows and their assertions are unchanged, only how they're written is. The
dpfhalf tightens assertions on paths already being executed, so it doesn't move the function-level number either. All suites green.Lint gate green (
format-nightly,clippy,carbide-lints), noCargo.lockdrift.Additional Notes
For anyone converting more of these later:
label_value()returnsopentelemetry::StringValue, and the explicit form was inferring that throughcheck_values. The macro form needs it spelled out on therun =closure, and the import added.This is the last of the five PRs under #3914. Worth recording that the audit driving that epic was directionally useful and numerically unreliable throughout -- it claimed ~11,000 lines of duplicate tests (3,713 were real), 119 silent tests (44 were real), and ~6,500 lines of collapsible table rows (about 360 rows were genuinely safe). Every PR in the series re-verified its candidates against
mainbefore touching them, and the coverage A/B was the gate throughout.Closes #4224