test: collapse per-variant test clusters in ib-fabric, admin-cli, and libmlx#2710
test: collapse per-variant test clusters in ib-fabric, admin-cli, and libmlx#2710chet wants to merge 1 commit into
Conversation
|
@coderabbitai PTAL, thanks! |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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 (13)
💤 Files with no reviewable changes (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
Summary by CodeRabbit
WalkthroughTest suites across ChangesTest Suite Consolidation into Scenario-Driven Tables
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get PR details
gh pr view 2710 --repo NVIDIA/infra-controller --json title,body,files,state,additions,deletions
echo "---FILES---"
gh pr diff 2710 --repo NVIDIA/infra-controllerLength of output: 67935
The last two rows exercise an identical input set: "neither SKU nor instance -> do not track" {
TrackPortDown { guid: "guid1", guid_to_index: &[("guid1", 0)], expected_inactive: None, instance_guids: None }
} => false,
"SKU has no IB devices (None), no instance -> do not track" {
TrackPortDown { guid: "guid1", guid_to_index: &[("guid1", 0)], expected_inactive: None, instance_guids: None }
} => false,Both have
Everything else is well-executed. The removal of construction-then-drop, |
|
@coderabbitai Thanks for the eyes. Updated -- dropped the duplicate |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the latest diff to verify the duplicate row fix
gh pr diff 2710 --repo NVIDIA/infra-controller | grep -A 60 "should_track_port_as_down_follows_sku_then_instance_precedence"Length of output: 2555 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the full current file content for the relevant test function
gh pr diff 2710 --repo NVIDIA/infra-controller | grep -A 120 "should_track_port_as_down_follows_sku_then_instance_precedence" | tail -80Length of output: 3293 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Look at the current file content around the test function to see the final state
gh api repos/NVIDIA/infra-controller/contents/crates/ib-fabric/src/lib.rs \
--jq '.content' | base64 -d | grep -A 80 "should_track_port_as_down_follows_sku_then_instance_precedence" | grep -v "^--$" | head -90Length of output: 568 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the PR branch name and fetch file from it
branch=$(gh pr view 2710 --repo NVIDIA/infra-controller --json headRefName --jq '.headRefName')
echo "Branch: $branch"
gh api "repos/NVIDIA/infra-controller/contents/crates/ib-fabric/src/lib.rs?ref=$branch" \
--jq '.content' | base64 -d | grep -A 90 "should_track_port_as_down_follows_sku_then_instance_precedence" | head -95Length of output: 751 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Get the PR head SHA
head_sha=$(gh pr view 2710 --repo NVIDIA/infra-controller --json headRefOid --jq '.headRefOid')
echo "Head SHA: $head_sha"
# Fetch via tree API to find the file
gh api "repos/NVIDIA/infra-controller/git/trees/$head_sha?recursive=1" \
--jq '.tree[] | select(.path | contains("ib-fabric")) | .path' 2>/dev/null | head -20Length of output: 963 🏁 Script executed: #!/bin/bash
set -euo pipefail
head_sha=$(gh pr view 2710 --repo NVIDIA/infra-controller --json headRefOid --jq '.headRefOid')
# Get the blob SHA for the file
blob_sha=$(gh api "repos/NVIDIA/infra-controller/git/trees/$head_sha?recursive=1" \
--jq '.tree[] | select(.path == "crates/ib-fabric/src/lib.rs") | .sha')
echo "Blob SHA: $blob_sha"
# Fetch the blob content
gh api "repos/NVIDIA/infra-controller/git/blobs/$blob_sha" \
--jq '.content' | base64 -d | grep -n "neither SKU\|no IB devices\|None, None\|instance_guids: None\|expected_inactive: None" | head -20Length of output: 1669 🏁 Script executed: #!/bin/bash
set -euo pipefail
# List coderabbit's review comments to find the one about duplicate rows
gh api "repos/NVIDIA/infra-controller/pulls/2710/comments" \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: (.body | .[0:120])}' | head -60Length of output: 514
The second observation from the prior review — |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/admin-cli/src/cfg/dispatch.rs (1)
68-77: ⚡ Quick winRemove
#[allow(dead_code)]by adding explicit compile-time usage of derived enums.Now that runtime scaffolding is removed, these
#[allow(dead_code)]attributes can be replaced with a tiny type-assertion test (e.g.,fn assert_dispatch<T: Dispatch>() {}and calls for both enums), preserving intent without suppressing lints.As per coding guidelines, “Strongly avoid using
#[allow(dead_code)]for Rust code… Dead code detection is important to catch mistakes and avoid unused code building up.”🤖 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/cfg/dispatch.rs` around lines 68 - 77, Remove the `#[allow(dead_code)]` attributes from both the AllRunCmd and MixedCmd enum definitions. Instead, add a small generic assertion function at the bottom of the test module (e.g., a function named assert_dispatch that takes a generic type parameter constrained to the Dispatch trait with an empty body). Then call this assertion function once for AllRunCmd and once for MixedCmd to provide compile-time proof that these enums are being used, eliminating the need for the dead code suppression lint.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.
Inline comments:
In `@crates/libmlx/tests/runner/traits_tests.rs`:
- Around line 350-365: The value_scenarios! table-driven test currently only
covers the owned array form in the call to names() with ["GPIO_ENABLED",
"THERMAL_SENSORS"], but is missing coverage for the borrowed form with
&["GPIO_ENABLED", "THERMAL_SENSORS"]. Add a second test row to the table that
calls names() with the borrowed form &["GPIO_ENABLED", "THERMAL_SENSORS"] and
the same registry parameter, using the identical expected expansion list with
all the indexed array elements. This ensures both the owned and borrowed
container forms are tested with the same expansion operation in the table.
---
Nitpick comments:
In `@crates/admin-cli/src/cfg/dispatch.rs`:
- Around line 68-77: Remove the `#[allow(dead_code)]` attributes from both the
AllRunCmd and MixedCmd enum definitions. Instead, add a small generic assertion
function at the bottom of the test module (e.g., a function named
assert_dispatch that takes a generic type parameter constrained to the Dispatch
trait with an empty body). Then call this assertion function once for AllRunCmd
and once for MixedCmd to provide compile-time proof that these enums are being
used, eliminating the need for the dead code suppression lint.
🪄 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: 42b76065-2345-4577-acf8-1118d4a6850b
📒 Files selected for processing (13)
crates/admin-cli/src/boot_override/tests.rscrates/admin-cli/src/cfg/dispatch.rscrates/admin-cli/src/dpu/tests.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/firmware/tests.rscrates/admin-cli/src/machine/tests.rscrates/ib-fabric/src/lib.rscrates/libmlx/tests/lockdown/test_lockdown.rscrates/libmlx/tests/lockdown/test_runner.rscrates/libmlx/tests/runner/command_builder_tests.rscrates/libmlx/tests/runner/exec_options_tests.rscrates/libmlx/tests/runner/runner_integration_tests.rscrates/libmlx/tests/runner/traits_tests.rs
💤 Files with no reviewable changes (6)
- crates/libmlx/tests/runner/exec_options_tests.rs
- crates/admin-cli/src/dpu/tests.rs
- crates/libmlx/tests/lockdown/test_lockdown.rs
- crates/libmlx/tests/lockdown/test_runner.rs
- crates/libmlx/tests/runner/command_builder_tests.rs
- crates/libmlx/tests/runner/runner_integration_tests.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
… libmlx Three already-migrated crates still carried one-function-per-variant clusters and a handful of tests that asserted nothing. `ib-fabric`'s 12 `should_track_port_*` functions and 3 `ib_port_down_alert_*` functions become two tables; `admin-cli`'s six `dpu_mode` parse tests and the `boot_override` get/clear pair collapse into tables; `libmlx`'s four `&[...]`-vs-`[...]` trait-impl pairs become four small tables (each row monomorphizes its own container impl). The same pass drops tests that exercised nothing but the stdlib or a trivial setter -- `str::contains`, the `Command` `Debug` string, construction-then-drop "creation"/"default" tests, two empty `assert_dispatch::<T>()` compile-checks, and the no-assertion CLI routing tests. It also rewrites `admin-cli`'s `health_override_templates` check so the variant-to-name map is exhaustive: a newly-added template now fails to compile without a row, closing the silent `_ => unreachable!()` gap the old test had. About 290 fewer test lines, no production change. Verified per crate (ib-fabric 21, admin-cli 346, libmlx 135 + 23) and clippy-clean with `-D warnings`. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
2 participants
Wave 1 of the carbide-test-support integration (#2692).
Three already-migrated crates still had one-function-per-variant test clusters and a few tests that asserted nothing.
ib-fabric's 12should_track_port_*+ 3ib_port_down_alert_*functions,admin-cli's 6dpu_modeparse tests + theboot_overrideget/clear pair, andlibmlx's four&[...]-vs-[...]trait-impl pairs all collapse into tables (each libmlx row monomorphizes its own container impl).Drops tests that exercised nothing but the stdlib or a trivial setter (
str::contains, theCommandDebugstring, construction-then-drop tests, two emptyassert_dispatch::<T>()compile-checks, the no-assertion CLI routing tests). Rewritesadmin-cli'shealth_override_templatesso its variant-to-name map is exhaustive -- a newly-added template now fails to compile without a row, closing the silent_ => unreachable!()gap.~290 fewer test lines, no production change. Verified per crate (
cargo test: ib-fabric 21, admin-cli 346, libmlx 135 + 23), clippy-clean with-D warnings.Addresses #2696.