test(api-db,health): consolidate duplicated CRUD and IP-candidate tests by chet · Pull Request #2714 · NVIDIA/infra-controller

chet · 2026-06-20T16:05:20Z

Wave 1 of the carbide-test-support integration (#2692).

api-db: the three expected_{machine,switch,power_shelf} suites shared an identical delete-then-confirm-absent body -- it now lives once in a test_support helper that each entity's #[sqlx_test] calls with its own delete_by_mac / find_by_bmc_mac_address (per-entity lookup/duplicate/update tests stay, since constructors differ). The ip_allocator IPv4-candidate trio becomes one check_cases table; expected_rack's pure insert-read-back round-trips are removed.

Two weak tests are repaired, not just dropped: route_servers's test_sync_with_duplicate_addresses_in_input accepted either Ok or Err (couldn't fail) and now pins the real contract -- a duplicate address trips the UNIQUE constraint, so replace rejects it. health's should_monitor_key test asserted f(x) == f(x) and is removed.

No production change. Verified against Postgres (cargo test: changed api-db modules 55, health sharding 4).

Addresses #2698.

chet · 2026-06-20T16:05:23Z

@coderabbitai PTAL, thanks!

copy-pr-bot · 2026-06-20T16:05:23Z

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 · 2026-06-20T16:05:28Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8de33660-a71b-4e56-b2a9-6ded582e4acd

📥 Commits

Reviewing files that changed from the base of the PR and between 8bc2512 and 7cb6e98.

📒 Files selected for processing (9)

crates/api-db/src/expected_machine/tests.rs
crates/api-db/src/expected_power_shelf/tests.rs
crates/api-db/src/expected_rack/tests.rs
crates/api-db/src/expected_switch/tests.rs
crates/api-db/src/ip_allocator.rs
crates/api-db/src/route_servers.rs
crates/api-db/src/test_support/expected_host.rs
crates/api-db/src/test_support/mod.rs
crates/health/src/sharding.rs

💤 Files with no reviewable changes (2)

crates/health/src/sharding.rs
crates/api-db/src/expected_rack/tests.rs

🚧 Files skipped from review as they are similar to previous changes (7)

crates/api-db/src/test_support/mod.rs
crates/api-db/src/route_servers.rs
crates/api-db/src/expected_power_shelf/tests.rs
crates/api-db/src/expected_switch/tests.rs
crates/api-db/src/expected_machine/tests.rs
crates/api-db/src/ip_allocator.rs
crates/api-db/src/test_support/expected_host.rs

Summary by CodeRabbit

Release Notes

Tests
- Consolidated expected-host deletion verification across database modules using shared test utilities.
- Refactored IPv4 allocator candidate checks into a table-driven (data-driven) test.
- Strengthened route server duplicate-address tests to assert the specific unique-violation failure contract.
- Removed/trimmed outdated or redundant tests, including expected-rack create/find-by-rack-id coverage and a shard key consistency unit test.

Note: This release focuses on test coverage and reliability improvements only; no user-facing behavior changes.

Walkthrough

This PR introduces a shared assert_delete_by_mac_removes_row helper that encapsulates the delete-and-verify transaction pattern across expected-host test suites, migrating test_delete in three modules to eliminate duplicated inline transaction management. Independently, it consolidates IPv4 candidate tests into a data-driven table, pins the route-server duplicate-address test to enforce unique-violation errors, removes redundant expected_rack CRUD tests, and eliminates a tautological shard consistency test.

Changes

Expected-host delete test refactor

Layer / File(s)	Summary
Shared delete-and-verify helper `crates/api-db/src/test_support/mod.rs`, `crates/api-db/src/test_support/expected_host.rs`	New `pub(crate) mod expected_host` submodule declared; new `pub(crate) async fn assert_delete_by_mac_removes_row<T>` helper added that accepts a `PgPool`, a `MacAddress`, and two async closures (delete, find). The helper orchestrates a delete-in-transaction followed by a fresh-transaction lookup, asserting the row is absent with explicit error messages for each stage.
Delete tests migrated to helper `crates/api-db/src/expected_machine/tests.rs`, `crates/api-db/src/expected_power_shelf/tests.rs`, `crates/api-db/src/expected_switch/tests.rs`	`test_delete` in each module now extracts the BMC MAC from the seeded fixture, commits the seeding transaction, and delegates to `assert_delete_by_mac_removes_row` via `delete_by_mac` and `find_by_bmc_mac_address` async closures, removing all inline transaction management and post-delete absence assertions.

Test consolidation and cleanup

Layer / File(s)	Summary
Redundant expected_rack tests removed `crates/api-db/src/expected_rack/tests.rs`	Removes `test_db_find_by_rack_id` and `test_db_create_and_find`, eliminating basic find/create coverage that is redundant given the remaining mutation and edge-case test suite.
IPv4 candidate tests consolidated to check_cases `crates/api-db/src/ip_allocator.rs`	Imports `Outcome`, `Case`, and `check_cases` from `carbide_test_support`; rewrites `test_v4_candidate` using an `AllocatedSet` struct and `check_cases` to cover multiple allocated-CIDR scenarios (distinct allocations, duplicate `/32`, covered `/31`) in a single assertion that all yield `192.168.1.8/30`; removes the now-redundant separate `test_v4_candidate_with_duplicate` and `test_v4_candidate_with_covered` tests.
Route-server duplicate-address test pinned to unique-violation error `crates/api-db/src/route_servers.rs`	`test_sync_with_duplicate_addresses_in_input` now unconditionally asserts that `replace` returns `Err(DatabaseError::Sqlx)` wrapping a `sqlx::Error::Database` where `is_unique_violation()` is true, removing the prior permissive branching that accepted either success or any error. This pins the contract for handling duplicate address inputs.
Shard consistency test removed `crates/health/src/sharding.rs`	Removes the `test_should_monitor_key_consistency` unit test from the `ShardManager` test suite.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

Consolidate the duplicated CRUD and IP-candidate test trios in api-db and health #2698 — This PR directly implements the consolidation objectives outlined in that issue by introducing the assert_delete_by_mac_removes_row helper, converting IPv4 candidate tests to check_cases-driven format, pinning the duplicate-address route-server uniqueness constraint, and removing redundant expected_rack tests.
carbide-test-support integration -- finish the migration, extend the coverage #2692 — This PR aligns with Wave 1 of the test modernization Epic, collapsing redundant test functions into data-driven patterns via carbide_test_support and removing low-signal test cases across the api-db and health crates.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately captures the main objective: consolidating duplicated test code across api-db and health modules through the carbide-test-support integration.
Description check	✅ Passed	The description comprehensively explains the consolidation of duplicated CRUD tests, the IP-candidate table-driven refactoring, test repairs, and their scope across modules.
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 · 2026-06-20T16:05:37Z

@chet: Certainly — I will review the PR now. Thank you for the ping!

✅ 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 · 2026-06-20T17:13:15Z

@coderabbitai review

coderabbitai · 2026-06-20T17:13:21Z

✅ 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 · 2026-06-20T17:42:19Z

@coderabbitai full review

coderabbitai · 2026-06-20T17:42:26Z

✅ Action performed

Full review finished.

coderabbitai

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/api-db/src/route_servers.rs`:
- Around line 424-427: The assertion in the replace test is using a generic
is_err() check that will pass for any error, not just duplicate constraint
violations. Replace the generic assert!(result.is_err()) call with a more
specific assertion that validates the error is specifically a duplicate or
UNIQUE-constraint violation error. Check the result for the corresponding
DatabaseError variant that represents constraint violations (such as a
constraint error type or similar) to ensure the test truly pins the
duplicate-rejection behavior rather than masking unrelated failures.

🪄 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: 77068414-d3f1-4af8-83d5-acd54852d47a

📥 Commits

Reviewing files that changed from the base of the PR and between 79e54c0 and 9c3cd4f.

📒 Files selected for processing (9)

crates/api-db/src/expected_machine/tests.rs
crates/api-db/src/expected_power_shelf/tests.rs
crates/api-db/src/expected_rack/tests.rs
crates/api-db/src/expected_switch/tests.rs
crates/api-db/src/ip_allocator.rs
crates/api-db/src/route_servers.rs
crates/api-db/src/test_support/expected_host.rs
crates/api-db/src/test_support/mod.rs
crates/health/src/sharding.rs

💤 Files with no reviewable changes (2)

crates/health/src/sharding.rs
crates/api-db/src/expected_rack/tests.rs

The `expected_{machine,switch,power_shelf}` suites each repeated an identical delete-then-confirm-absent body. That flow now lives once in a `test_support` helper that takes each entity's `delete_by_mac` / `find_by_bmc_mac_address` as async closures; every entity keeps its own `#[sqlx_test]` (one pool per test) and its own lookup / duplicate / update tests, since those have distinct constructors. `ip_allocator`'s three IPv4-candidate tests become one `check_cases` table, and `expected_rack`'s pure insert-then-read-back round-trips -- which asserted no transformation -- are removed. Two weak tests are repaired rather than just dropped where a real contract exists. `route_servers`'s `test_sync_with_duplicate_addresses_in_input` used to accept either `Ok` or `Err`, so it could not fail; it now pins the actual behavior -- `replace` hands its input to a single batched INSERT, so a duplicate address trips the `address inet NOT NULL UNIQUE` constraint and the whole call is rejected. `health`'s `should_monitor_key` "consistency" test asserted `f(x) == f(x)` and is removed outright. No production change. Verified against Postgres -- the changed api-db modules (55 tests) and `health` sharding (4) pass. Signed-off-by: Chet Nichols III <chetn@nvidia.com>

github-actions · 2026-06-21T05:39:22Z

🔍 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.

coderabbitai Bot reviewed Jun 20, 2026

View reviewed changes

Comment thread crates/api-db/src/route_servers.rs Outdated

chet force-pushed the gh-issue-2698 branch from 9c3cd4f to 8bc2512 Compare June 20, 2026 18:20

chet marked this pull request as ready for review June 21, 2026 04:16

chet requested a review from a team as a code owner June 21, 2026 04:16

rwthompsonii approved these changes Jun 21, 2026

View reviewed changes

chet force-pushed the gh-issue-2698 branch from 8bc2512 to 7cb6e98 Compare June 21, 2026 05:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(api-db,health): consolidate duplicated CRUD and IP-candidate tests#2714

test(api-db,health): consolidate duplicated CRUD and IP-candidate tests#2714
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-2698

chet commented Jun 20, 2026

Uh oh!

chet commented Jun 20, 2026

Uh oh!

copy-pr-bot Bot commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

chet commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

chet commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jun 21, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

chet commented Jun 20, 2026

Uh oh!

chet commented Jun 20, 2026

Uh oh!

copy-pr-bot Bot commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Release Notes

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Uh oh!

coderabbitai Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chet commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chet commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔍 Container Scan Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

github-actions Bot commented Jun 21, 2026 •

edited

Loading