test(api-core): re-enable the three rack-handler tests against current behavior

[chet]

The three rack state-controller tests surfaced by #2694 were #[ignore]d because they assert an old state-machine vocabulary the handlers no longer use -- not because of any handler bug (confirmed: created.rs/discovering.rs are correct, corroborated by adjacent passing tests). They now pin current behavior and run again:

• test_expected_incomplete_device_counts_stays → asserts Wait (was DoNothing).
• test_discovering_waits_for_compute_ready → renamed test_discovering_waits_when_compute_not_ready, asserts Wait (was Err).
• test_expected_more_discovered_than_expected_transitions → now seeds two hosts against the single-compute profile so over-discovery genuinely exceeds expected, keeping the Transition(Discovering) assertion; dropped the dead mac1 line + stale comment.

No production change. Verified: cargo test -p carbide-api-core --lib -- rack_state_controller::handler → 42 passed, 0 ignored; clippy --locked --all-targets --all-features + nightly rustfmt clean.

Addresses #2715.

[chet]

@coderabbitai PTAL, thanks!

[copy-pr-bot]

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.

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 67322df3-ccf3-413b-b414-de2309713ebb

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

Three previously ignored rack state-controller handler tests are re-enabled by removing their #[ignore] attributes. Their assertions are updated to expect StateHandlerOutcome::Wait in place of prior error or stale outcomes, and the over-discovery test receives a reworked fixture that seeds two managed host records to correctly exceed the expected compute minimum.

Changes

Rack State-Controller Test Fixes

Layer / File(s)	Summary
Re-enable incomplete device counts test crates/api-core/src/tests/rack_state_controller/handler.rs	Removes #[ignore] from test_expected_incomplete_device_counts_stays and updates the assertion to StateHandlerOutcome::Wait { .. } with "wait in Created" messaging.
Re-enable over-discovery test with fixture rework crates/api-core/src/tests/rack_state_controller/handler.rs	Removes #[ignore] from test_expected_more_discovered_than_expected_transitions, seeds two managed host fixtures under the "Single" rack profile to exceed the expected compute minimum, and removes a stale ordering-branch comment.
Rename and fix Discovering-state wait test crates/api-core/src/tests/rack_state_controller/handler.rs	Renames the test to test_discovering_waits_when_compute_not_ready, removes #[ignore], clarifies the rack setup intent by documenting absence of Ready/Assigned compute states, and replaces the result.is_err() assertion with StateHandlerOutcome::Wait { .. }.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• Re-enable or correct 3 rack-handler state-transition tests that fail against current behavior #2715: The three re-enabled tests and corrected assertions directly address the failing rack state-controller tests identified in this issue.
• Restore coverage from four ignored rack-handler tests and an always-true assertion #2694: The removal of #[ignore] attributes and the updated expectations align precisely with the scope specified in this issue.

Possibly related PRs

• NVIDIA/infra-controller#2716: Modifies the same rack state-controller test file with analogous changes to #[ignore] removal and expected transition assertion updates.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the primary change: re-enabling three previously ignored rack-handler tests with updated behavior assertions.
Description check	✅ Passed	The description comprehensively details the rationale, specific test changes, and validation results, clearly relating to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== PR Details ==="
gh pr view 2735 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions,state

echo ""
echo "=== PR Diff ==="
gh pr diff 2735 --repo NVIDIA/infra-controller

Length of output: 7570

---

---

@chet: The changes are clean and well-reasoned. Here is my assessment:

Overall: Approved — test-only, no production risk.

---

Per-test review

Test	Assessment
test_expected_incomplete_device_counts_stays	✅ Straightforward DoNothing → Wait correction. Error message updated to match.
test_expected_more_discovered_than_expected_transitions	✅ Significant improvement. The old path called db_rack::update on a connection that had already gone out of meaningful scope and never actually seeded compute hosts — the test was asserting nothing real. Two new_host() calls now genuinely exercise the over-discovery threshold. The explicit drop(txn) before those calls is correct.
test_discovering_waits_when_compute_not_ready	✅ The rename is more precise. Switching from result.is_err() to .await? + matches!(outcome, Wait) is a tighter contract: if the handler faults, the test now fails with an error rather than silently passing as a false positive.

---

One minor observation

In test_discovering_waits_when_compute_not_ready, the assertion diagnostic uses:

"Discovering should wait when compute hosts are not yet ready, got {:?}",
std::mem::discriminant(&outcome)

std::mem::discriminant yields an opaque integer, not a variant name. If StateHandlerOutcome derives Debug, replacing this with {outcome:?} would give a more actionable failure message. Non-blocking — the test logic itself is correct.

[approve]

[chet]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[chet]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[chet]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

🔍 Container Scan Summary

Service	Total	Critical	High	Medium	Low	Other
boot-artifacts-aarch64	3	0	0	3	0	0
boot-artifacts-x86_64	3	0	0	3	0	0
forge-admin-cli-x86_64	264	6	23	99	6	130
machine-validation-runner	704	34	183	258	35	194
machine_validation	704	34	183	258	35	194
nvmetal-carbide	704	34	183	258	35	194
TOTAL	2382	108	572	879	111	712

Per-CVE detail lives in the per-service grype-* artifacts (JSON + SARIF). Severity counts only — no CVE IDs published here.