test(api-core): parameterize cross-entity test duplication#2717
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 (18)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (16)
Summary by CodeRabbit
WalkthroughThree independent test-suite consolidations within ChangesTest Consolidation: find_*_by_ids guards, HealthCrud harness, component_manager table tests
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
gh pr view 2717 --repo NVIDIA/infra-controller --json title,body,files,additions,deletions,changedFiles | jq '.'Length of output: 4420 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 2717 --repo NVIDIA/infra-controller 2>/dev/null | head -3000Length of output: 109858
1.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/api-core/src/tests/common/health_crud.rs (1)
215-223: ⚡ Quick winReduce duplicate insert wiring in aggregation checks.
check_health_aggregationcurrently takesreal_id+insertseparately, which forces each entity test module to duplicate the same insert request-shape closure already present in itsHealthCrudsetup. Consider accepting a&HealthCrud<...>(or a small shared adapter) so CRUD and aggregation share one wiring path.🤖 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/api-core/src/tests/common/health_crud.rs` around lines 215 - 223, The check_health_aggregation function currently accepts real_id and insert as separate parameters, forcing duplicate insert request-shape closure definitions in each entity test module. Refactor the function signature to accept a reference to HealthCrud instead of these separate parameters, extracting the real_id and insert logic from the HealthCrud object within the function body. This will allow all test modules to reuse the same insert wiring from their existing HealthCrud setup rather than duplicating it.crates/api-core/src/tests/power_shelf_health.rs (1)
172-176: ⚡ Quick winAvoid creating a real entity in the missing-entity test.
check_missing_entity()only exercisesnonexistent_id, so creating a persisted power shelf here is unused setup and adds avoidable DB work.Lean test setup
async fn test_missing_power_shelf_id(pool: sqlx::PgPool) -> Result<(), Box<dyn std::error::Error>> { let env = test_env(pool).await; - let id = new_power_shelf(&env, None, None, None, None).await?; - power_shelf_crud(&env, id).check_missing_entity().await; + let dummy_real_id = PowerShelfId::from(uuid::Uuid::new_v4()); + power_shelf_crud(&env, dummy_real_id).check_missing_entity().await; Ok(()) }🤖 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/api-core/src/tests/power_shelf_health.rs` around lines 172 - 176, The test_missing_power_shelf_id function unnecessarily creates a real power shelf entity via new_power_shelf() when check_missing_entity() only exercises nonexistent_id and doesn't use the created entity. Remove the new_power_shelf call and instead create a nonexistent ID directly (likely a UUID or equivalent identifier) and pass that to power_shelf_crud() to eliminate unnecessary database setup work.
🤖 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-core/src/tests/common/health_crud.rs`:
- Around line 137-149: The check_missing_entity method currently uses a generic
is_err() assertion which will pass for any error type, making it vulnerable to
regressions where the error status changes from NotFound to something else.
Instead of just asserting that result is an error, extract the actual error from
the result and verify that it contains the specific NotFound gRPC status code.
This ensures the test properly validates that the expected NotFound status is
returned when attempting to insert an override for a nonexistent entity.
In `@crates/api-core/src/tests/find_by_ids_guards.rs`:
- Around line 151-153: The over-max guard test case in find_vpcs_by_ids is
creating multiple copies of VpcId::default() instead of generating distinct IDs.
This means if the VPC path ever adds deduplication logic before the guard check,
the test would silently stop testing the actual over-limit path. Replace the map
function that generates vpc_ids to create unique/distinct VpcId values (like
incrementing based on the loop variable) instead of reusing VpcId::default() for
each iteration.
---
Nitpick comments:
In `@crates/api-core/src/tests/common/health_crud.rs`:
- Around line 215-223: The check_health_aggregation function currently accepts
real_id and insert as separate parameters, forcing duplicate insert
request-shape closure definitions in each entity test module. Refactor the
function signature to accept a reference to HealthCrud instead of these separate
parameters, extracting the real_id and insert logic from the HealthCrud object
within the function body. This will allow all test modules to reuse the same
insert wiring from their existing HealthCrud setup rather than duplicating it.
In `@crates/api-core/src/tests/power_shelf_health.rs`:
- Around line 172-176: The test_missing_power_shelf_id function unnecessarily
creates a real power shelf entity via new_power_shelf() when
check_missing_entity() only exercises nonexistent_id and doesn't use the created
entity. Remove the new_power_shelf call and instead create a nonexistent ID
directly (likely a UUID or equivalent identifier) and pass that to
power_shelf_crud() to eliminate unnecessary database setup work.
🪄 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: 666e6119-07bc-46f8-bbb8-e38fcc2678d2
📒 Files selected for processing (18)
crates/api-core/src/handlers/component_manager.rscrates/api-core/src/tests/common/health_crud.rscrates/api-core/src/tests/common/mod.rscrates/api-core/src/tests/explored_endpoint_find.rscrates/api-core/src/tests/explored_managed_host_find.rscrates/api-core/src/tests/find_by_ids_guards.rscrates/api-core/src/tests/ib_partition_find.rscrates/api-core/src/tests/instance_find.rscrates/api-core/src/tests/machine_find.rscrates/api-core/src/tests/mod.rscrates/api-core/src/tests/network_segment_find.rscrates/api-core/src/tests/power_shelf_find.rscrates/api-core/src/tests/power_shelf_health.rscrates/api-core/src/tests/rack_health.rscrates/api-core/src/tests/switch_find.rscrates/api-core/src/tests/switch_health.rscrates/api-core/src/tests/tenant_keyset_find.rscrates/api-core/src/tests/vpc_find.rs
|
@coderabbitai On the rest -- all intended/minor: |
|
Tip For best results, initiate chat on the files or code changes.
For the two deferred items — the unnecessary |
|
@coderabbitai No need to open a separate issue -- both are trivial test-only tidy-ups, fine to leave. Thanks for the thorough pass! |
|
Tip For best results, initiate chat on the files or code changes.
(=^ ᴥ ^=) |
Every `find_*_by_ids` RPC re-proved the same two API-layer guards -- empty list and over-max -- once per entity, ~20 near-identical `#[sqlx_test]` tests across ten `*_find.rs` files. They collapse into two shared tests (`find_by_ids_guards.rs`), each running a labeled spread of the ten RPCs over one pool and asserting the exact `InvalidArgument` code and message per RPC. The guard fires ahead of any DB work, so proving it once per guard is enough. The power-shelf, switch, and rack health-override suites each implemented the same six CRUD behaviors fn-for-fn. Those bodies move once into a `HealthCrud` harness driven by per-entity closures (`common/health_crud.rs`); each entity's `#[sqlx_test]` builds the harness over its own pool and calls the shared checks. Rack's unique override-precedence and host-propagation tests stay per-entity. The six `power_action_*` and five `error_to_status_*` per-variant tests in `component_manager` become two tables; the error-mapping table also gains the previously-untested `Transport` (-> `Unavailable`) and `Rms` (-> `Internal`) arms, so the merge raises coverage rather than just shrinking the file. No production change. About 1,500 lines of duplicated suite collapse into two shared harnesses. Verified against Postgres -- the changed modules (70 tests) pass. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
2 participants
Wave 1 of the carbide-test-support integration (#2692).
Three pockets of api-core cross-entity duplication, consolidated:
find_*_by_idsRPC re-proved the same two API-layer guards (empty list, over-max) once per entity (~20#[sqlx_test]tests across ten*_find.rs). Now two shared tests (find_by_ids_guards.rs), each looping a labeled spread of the ten RPCs over one pool and asserting the exactInvalidArgumentcode + message per RPC. Guard fires before any DB work, so once per guard suffices.HealthCrudharness driven by per-entity closures (common/health_crud.rs), each entity's#[sqlx_test]calling the shared checks over its own pool. Rack's unique precedence/propagation tests stay per-entity.power_action_*(6) anderror_to_status_*(5) per-variant tests become two tables; the error-mapping table adds the previously-untestedTransport→UnavailableandRms→Internalarms, so coverage goes up.No production change; ~1,500 lines of duplicated suite collapse into two shared harnesses. Verified against Postgres (changed modules: 70 tests pass), clippy-clean with
-D warnings.Addresses #2699.