test: delete tests a stronger sibling already covers - #4185
Merged
Conversation
The audit behind NVIDIA#3914 flagged something like 11,000 lines of duplicated tests. I went to delete them and found the audit was mostly wrong -- it had counted tests that *look* alike as tests that *cover* alike. So this is the ~3,700 lines I could actually prove, each one checked against the specific surviving test that still covers it. The clearest example of why the gap is so wide: `libmlx` has six `tests/` files that mirror inline `#[cfg(test)]` modules, and the audit wanted all six gone. They aren't duplicates -- `value.rs` says so in its own comments (*"the array variants are already covered by the sibling tests/ file"*, *"the spec branches **not** in the sibling file"*). The two halves were split deliberately. Deleting them would have taken out the only test of `Display for LockStatus`, the only tests of `is_array_type` and `get_set_indices` (both live in production in `runner.rs`), and the only coverage of `IntoMlxValue for Vec<String>`, which is how we parse mlxconfig `--json` arrays. Those files lost about half of what was claimed, and not one of them went away whole. Primary callouts are: - `dpf/duplicate_events.rs` goes entirely. Its setup is byte-identical to `waiting_for_ready.rs`, and the survivor is stronger -- it asserts `ForceOff == 1`, runs five more iterations, then asserts `== 0`, which bounds the over-run the deleted file was reaching for. The file's own module doc was also false: it claims to test watcher event dedup and never touches `DpuWatcher` or the `ON CONFLICT DO NOTHING` that actually does the deduping. - Three narrow NMX tests in the rack handler, subsumed by the e2e below them, which runs the same assertions under a harder two-switch fixture and additionally cross-checks that the cert step's in-memory primary agrees with the configure step's persisted one. - `nvue_startup_with_bridge.yaml.expected` and the test that read it. `diff` against `nvue_startup.yaml.expected` is empty and both tests pass identical arguments -- so a bridge regression needed *both* goldens updated before anything failed. The surviving test is the bridge case; there's a note on it now saying so. - Two "set the state with raw SQL, read it back" tests that run no controller and no handler, and pay for a template database to round-trip serde. - Six of seven `mqttea` trait tests that dispatch to a handler defined in the test file itself and assert on its own atomics. Three things in here aren't deletions. `test_expected_no_definition_stays_parked` was handing the handler a *known* rack profile, so it waited for the wrong reason and left the unresolved-profile branch uncovered -- it passes `None` now and matches on the wait reason, not just the variant, since the device-count branch also returns `Wait`. `test_spec.rs` had a bare `matches!(spec, MlxVariableSpec::Boolean);` with the result thrown away, which is the same class NVIDIA#4173 just swept out. And the surviving nvue test now explains that it *is* the bridge case, so nobody re-adds one that isn't. Proven with a function-level coverage A/B over the eight touched crates: 1,444 `api-core` tests green, and **no production function lost its coverage**. Fourteen entries drop out of the build, all of them generic instantiations that existed only for deleted tests (`register_raw_message::<DogMessage>`, `check_values::<...>`); the generics themselves stay covered 251/252 and 253/254 through other instantiations. Tests updated! This supports NVIDIA#4180 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
|
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 (20)
💤 Files with no reviewable changes (13)
Summary by CodeRabbit
WalkthroughThe change audits test coverage across agent, API, libmlx, and mqttea modules. Redundant tests and fixtures are removed, equivalent cases are consolidated, rack wait assertions are strengthened, and metrics-registration coverage is added. ChangesTest suite audit
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
🐇🔍 ✅ Action performedFull review finished. |
chet
enabled auto-merge (squash)
July 27, 2026 04:15
nick-vetro
approved these changes
Jul 27, 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.
The audit behind #3914 flagged something like 11,000 lines of duplicated tests. I went to delete them and found the audit was mostly wrong -- it had counted tests that look alike as tests that cover alike. So this is the ~3,700 lines I could actually prove, each one checked against the specific surviving test that still covers it.
The clearest example of why the gap is so wide:
libmlxhas sixtests/files that mirror inline#[cfg(test)]modules, and the audit wanted all six gone. They aren't duplicates --value.rssays so in its own comments ("the array variants are already covered by the sibling tests/ file", "the spec branches not in the sibling file"). The two halves were split deliberately. Deleting them would have taken out the only test ofDisplay for LockStatus, the only tests ofis_array_typeandget_set_indices(both live in production inrunner.rs), and the only coverage ofIntoMlxValue for Vec<String>, which is how we parse mlxconfig--jsonarrays. Not one of those files went away whole.What did go:
dpf/duplicate_events.rs, entirely. Its setup is byte-identical towaiting_for_ready.rs, and the survivor is stronger -- it assertsForceOff == 1, runs five more iterations, then asserts== 0. Its module doc was also false: it claims to test watcher event dedup and never touchesDpuWatcheror theON CONFLICT DO NOTHINGthat actually dedupes.nvue_startup_with_bridge.yaml.expectedand its test.diffagainstnvue_startup.yaml.expectedis empty and both tests pass identical arguments, so a bridge regression needed both goldens updated before anything failed.mqtteatrait tests that dispatch to a handler defined in the test file itself and assert on its own atomics.Three changes in here aren't deletions.
test_expected_no_definition_stays_parkedwas handing the handler a known rack profile, so it waited for the wrong reason and left the unresolved-profile branch uncovered -- it passesNonenow, and matches on the wait reason rather than just the variant, since the device-count branch also returnsWait.test_spec.rshad a barematches!(spec, MlxVariableSpec::Boolean);with the result thrown away, the same class #4173 swept out. And the surviving nvue test now says that it is the bridge case, so nobody re-adds one that isn't.Related issues
This supports #4180
Type of Change
Breaking Changes
Testing
Function-level coverage A/B against
mainover the eight touched crates. Line coverage is the wrong instrument for a deletion -- removing test lines makes the percentage rise on its own, and--ignore-filename-regexcan't reach inside an inline#[cfg(test)]module. So the question asked here is narrower: did any production function go from executed to never-executed?api-coretests green (down from 1,454 -- exactly the ten removed), 0 failures across all eight crates.register_raw_message::<DogMessage>,check_values::<...>. The generics themselves stay covered through other instantiations:check_values251/252,Check::check253/254.left: "waiting for devices: compute=0/2, ..."againstright: "no or unknown rack_profile_id". The old bareWait { .. }assertion passed in that state.Additional Notes
Reviewer guidance -- the two places I'd look hardest:
libmlxtrims. Six files, none deleted whole. Everything kept is kept for a stated reason: YAML on-disk schema contracts (grep serde_yaml src/variables/{registry,spec}.rsreturns nothing, so only the mirrors pin the registry file format),log_json_output = true,LockdownManager::with_dry_run, the non-dry-run error path, and three cases where the mirror pinned an exact error variant while the inline twin asserted a bareis_err().handler.rs. 362 lines of NMX tests removed on the claim that one e2e covers all three, plus one of the three near-identicalCreatedtests. I kepttest_expected_incomplete_device_counts_staysas the honest counts-shortfall case and repurposed the other.One I deliberately left in:
libmlx'stest_malformed_json. It's subsumed at equal strength, which is the weakest justification for removal there is, so it stays.crates/rpc/src/model/hardware_info.rsis untouched here on purpose. Removing its projection layer is a rewrite that makes the tests stronger rather than a deletion, so it belongs with the table-test work rather than in this PR.Closes #4180