test(api-core): move power management tests to integration suites#3116
test(api-core): move power management tests to integration suites#3116poroh wants to merge 1 commit into
Conversation
Split power option API coverage into api-core integration tests and machine-controller power management behavior into controller integration tests. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
WalkthroughThis PR removes the legacy machine_power unit test module from api-core and replaces it with new power_options integration tests validating maintenance-gated power state updates. Separately, machine-controller gains a redfish_sim field in its test Env, a tonic dev-dependency, and a new power_management integration test module validating controller polling, retry limits, and state persistence. Changesapi-core power options tests
machine-controller power management tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant Controller
participant BMC
participant DB
Test->>DB: force next poll timestamp
Test->>Controller: run controller iteration
Controller->>BMC: fetch current power state
BMC-->>Controller: report power state
Controller->>DB: persist power_options update
Test->>DB: assert desired/last_fetched state and counters
Related PRs: None found. Suggested labels: tests, machine-controller, api-core Suggested reviewers: None found. A rabbit hops through power states bright, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/api-core/tests/integration/power_options.rs (1)
60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate initial-state assertion block.
Both tests repeat the identical "assert exactly one power option in the
Onstate formh.host.id" block. Consider extracting a small helper (e.g.assert_initial_power_state(&env, &mh)) to avoid drift between the two copies as the schema evolves.♻️ Proposed helper extraction
+async fn assert_initial_power_state(env: &TestHarness, mh: &TestManagedHost) { + let mut txn = env.db_txn().await; + let power_options = db::power_options::get_all(&mut txn).await.unwrap(); + assert_eq!(power_options.len(), 1); + assert_eq!(power_options[0].host_id, mh.host.id); + assert_eq!(power_options[0].desired_power_state, PowerState::On); + txn.rollback().await.unwrap(); +}Also applies to: 97-102
🤖 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/tests/integration/power_options.rs` around lines 60 - 65, The initial power-option assertion is duplicated in both integration tests, so update the repeated block in the power options test helpers to use a shared helper instead of copy-pasting the same checks. Extract the “one PowerState::On entry for mh.host.id” logic into a small helper such as assert_initial_power_state in the power_options test module, and have both test cases call it so the assertions stay consistent as db::power_options or env.db_txn behavior evolves.crates/machine-controller/tests/integration/power_management.rs (2)
95-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRaw SQL bypasses compile-time query checking.
sqlx::query("UPDATE power_options SET last_fetched_next_try_at = now() WHERE host_id = $1")is a plain string query, not asqlx::query!macro invocation, so column/table drift (e.g. renaminghost_idorlast_fetched_next_try_at) won't be caught at compile time — it will only surface as a runtime failure in this test. As per coding guidelines, "Use SQLx with compile-time checked queries for database access in Rust."🛠️ Proposed fix using the compile-time checked macro
- sqlx::query("UPDATE power_options SET last_fetched_next_try_at = now() WHERE host_id = $1") - .bind(self.host.id) - .execute(txn.as_mut()) - .await - .expect("next power poll should be updated"); + sqlx::query!( + "UPDATE power_options SET last_fetched_next_try_at = now() WHERE host_id = $1", + self.host.id + ) + .execute(txn.as_mut()) + .await + .expect("next power poll should be updated");🤖 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/machine-controller/tests/integration/power_management.rs` around lines 95 - 110, The test helper set_next_power_poll_now is using a raw sqlx::query string, which bypasses SQLx compile-time checking. Replace that database update with the compile-time checked SQLx macro form in set_next_power_poll_now, keeping the same UPDATE against power_options and the same host_id bind so schema drift on host_id or last_fetched_next_try_at is caught at compile time.Source: Coding guidelines
119-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated poll-and-assert boilerplate into a helper.
The
let mut txn = env.test_harness.db_txn().await; let power_options = db::power_options::get_all(&mut txn).await?; assert_eq!(...); txn.rollback().await?;sequence is duplicated near-verbatim across all three tests (and multiple times within each). A small helper (e.g.async fn fetch_power_options(env: &Env) -> Vec<PowerOptions>) would remove this repetition and make each assertion site a one-liner.♻️ Proposed helper
async fn fetch_power_options(env: &Env) -> Vec<db::power_options::PowerOptions> { let mut txn = env.test_harness.db_txn().await; let power_options = db::power_options::get_all(&mut txn) .await .expect("power options should be fetchable"); txn.rollback().await.expect("rollback should succeed"); power_options }Then call sites collapse to
let power_options = fetch_power_options(&env).await;.Also applies to: 164-183, 190-194, 216-220, 242-245, 253-256, 264-269
🤖 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/machine-controller/tests/integration/power_management.rs` around lines 119 - 153, The power-management integration tests repeat the same db transaction/query/rollback assertion boilerplate many times, so extract it into a small async helper near the existing test code (for example, a fetch_power_options-style helper) that opens the txn, calls db::power_options::get_all, and rolls back before returning the result. Update the repeated call sites in the test functions that exercise mh, env.test_harness.db_txn, and db::power_options::get_all to use this helper so each assertion reads as a single fetch-and-check step.
🤖 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.
Nitpick comments:
In `@crates/api-core/tests/integration/power_options.rs`:
- Around line 60-65: The initial power-option assertion is duplicated in both
integration tests, so update the repeated block in the power options test
helpers to use a shared helper instead of copy-pasting the same checks. Extract
the “one PowerState::On entry for mh.host.id” logic into a small helper such as
assert_initial_power_state in the power_options test module, and have both test
cases call it so the assertions stay consistent as db::power_options or
env.db_txn behavior evolves.
In `@crates/machine-controller/tests/integration/power_management.rs`:
- Around line 95-110: The test helper set_next_power_poll_now is using a raw
sqlx::query string, which bypasses SQLx compile-time checking. Replace that
database update with the compile-time checked SQLx macro form in
set_next_power_poll_now, keeping the same UPDATE against power_options and the
same host_id bind so schema drift on host_id or last_fetched_next_try_at is
caught at compile time.
- Around line 119-153: The power-management integration tests repeat the same db
transaction/query/rollback assertion boilerplate many times, so extract it into
a small async helper near the existing test code (for example, a
fetch_power_options-style helper) that opens the txn, calls
db::power_options::get_all, and rolls back before returning the result. Update
the repeated call sites in the test functions that exercise mh,
env.test_harness.db_txn, and db::power_options::get_all to use this helper so
each assertion reads as a single fetch-and-check step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e3ab4de-421a-4ecc-bdb8-d29764b22e9c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/api-core/src/tests/machine_power.rscrates/api-core/src/tests/mod.rscrates/api-core/tests/integration/main.rscrates/api-core/tests/integration/power_options.rscrates/machine-controller/Cargo.tomlcrates/machine-controller/tests/integration/env.rscrates/machine-controller/tests/integration/main.rscrates/machine-controller/tests/integration/power_management.rs
💤 Files with no reviewable changes (2)
- crates/api-core/src/tests/mod.rs
- crates/api-core/src/tests/machine_power.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
1 participant
Split power option API coverage into api-core integration tests and machine-controller power management behavior into controller integration tests.
Related issues
#2001
Type of Change
Breaking Changes
Testing
Additional Notes