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

chet · web-flow · commit cb07b44fce4c · 2026-06-21T09:38:32.000-07:00
diff --git a/crates/api-db/src/expected_machine/tests.rs b/crates/api-db/src/expected_machine/tests.rs
@@ -172,29 +172,15 @@ async fn test_update_bmc_credentials(pool: sqlx::PgPool) -> Result<(), Box<dyn s
 #[crate::sqlx_test]
 async fn test_delete(pool: sqlx::PgPool) -> () {
     create_fixture_expected_machines(&pool).await;
-    let mut txn = pool
-        .begin()
-        .await
-        .expect("unable to create transaction on database pool");
-    let machine = get_expected_machine_1(&mut txn)
-        .await
-        .expect("Expected machine not found");
-
-    assert_eq!(machine.data.serial_number, "VVG121GG");
-
-    db::expected_machine::delete_by_mac(&mut txn, machine.bmc_mac_address)
-        .await
-        .expect("Error deleting expected_machine");
-
-    txn.commit().await.expect("Failed to commit transaction");
-    let mut txn = pool
-        .begin()
-        .await
-        .expect("unable to create transaction on database pool");
-
-    get_expected_machine_1(&mut txn).await;
+    let mac = "0a:0b:0c:0d:0e:0f".parse().unwrap();
 
-    assert!(get_expected_machine_1(&mut txn).await.is_none())
+    crate::test_support::expected_host::assert_delete_by_mac_removes_row(
+        &pool,
+        mac,
+        async |txn, mac| db::expected_machine::delete_by_mac(txn, mac).await,
+        async |txn, mac| db::expected_machine::find_by_bmc_mac_address(txn, mac).await,
+    )
+    .await;
 }
 
 #[crate::sqlx_test]
diff --git a/crates/api-db/src/expected_power_shelf/tests.rs b/crates/api-db/src/expected_power_shelf/tests.rs
@@ -144,29 +144,16 @@ async fn test_update_bmc_credentials(pool: sqlx::PgPool) -> Result<(), Box<dyn s
 
 #[crate::sqlx_test]
 async fn test_delete(pool: sqlx::PgPool) -> () {
-    let mut txn = pool
-        .begin()
-        .await
-        .expect("unable to create transaction on database pool");
+    let mut txn = pool.begin().await.unwrap();
     let shelves = create_expected_power_shelves(&mut txn).await;
-    let power_shelf = &shelves[0];
-
-    assert_eq!(power_shelf.serial_number, "PS-SN-001");
-
-    db::expected_power_shelf::delete_by_mac(&mut txn, power_shelf.bmc_mac_address)
-        .await
-        .expect("Error deleting expected_power_shelf");
-
+    let mac = shelves[0].bmc_mac_address;
     txn.commit().await.expect("Failed to commit transaction");
-    let mut txn = pool
-        .begin()
-        .await
-        .expect("unable to create transaction on database pool");
 
-    assert!(
-        db::expected_power_shelf::find_by_bmc_mac_address(&mut txn, shelves[0].bmc_mac_address)
-            .await
-            .unwrap()
-            .is_none()
+    crate::test_support::expected_host::assert_delete_by_mac_removes_row(
+        &pool,
+        mac,
+        async |txn, mac| db::expected_power_shelf::delete_by_mac(txn, mac).await,
+        async |txn, mac| db::expected_power_shelf::find_by_bmc_mac_address(txn, mac).await,
     )
+    .await;
 }
diff --git a/crates/api-db/src/expected_rack/tests.rs b/crates/api-db/src/expected_rack/tests.rs
@@ -81,23 +81,6 @@ async fn seed_expected_racks(txn: &mut sqlx::PgConnection) -> Vec<RackId> {
     ids
 }
 
-#[crate::sqlx_test]
-async fn test_db_find_by_rack_id(pool: sqlx::PgPool) -> Result<(), Box<dyn std::error::Error>> {
-    let mut txn = pool.begin().await?;
-    let ids = seed_expected_racks(&mut txn).await;
-
-    let expected_rack = find_by_rack_id(&mut txn, &ids[0])
-        .await?
-        .expect("Expected rack not found");
-
-    assert_eq!(expected_rack.rack_id, ids[0]);
-    assert_eq!(expected_rack.rack_profile_id.as_str(), "NVL72");
-    assert_eq!(expected_rack.metadata.name, "rack-1");
-    assert_eq!(expected_rack.metadata.description, "Test rack 1");
-
-    Ok(())
-}
-
 #[crate::sqlx_test]
 async fn test_db_find_all(pool: sqlx::PgPool) -> Result<(), Box<dyn std::error::Error>> {
     let mut txn = pool.begin().await?;
@@ -117,45 +100,6 @@ async fn test_db_find_nonexistent(pool: sqlx::PgPool) -> Result<(), Box<dyn std:
     Ok(())
 }
 
-#[crate::sqlx_test]
-async fn test_db_create_and_find(pool: sqlx::PgPool) -> Result<(), Box<dyn std::error::Error>> {
-    let mut txn = pool.begin().await?;
-
-    let rack_id = new_rack_id();
-    let metadata = Metadata {
-        name: "test-rack".to_string(),
-        description: "A test rack".to_string(),
-        labels: [("env".to_string(), "test".to_string())]
-            .into_iter()
-            .collect(),
-    };
-
-    let created = create(
-        &mut txn,
-        &ExpectedRack {
-            rack_id: rack_id.clone(),
-            rack_profile_id: RackProfileId::new("NVL72"),
-
-            metadata,
-        },
-    )
-    .await?;
-
-    assert_eq!(created.rack_id, rack_id);
-    assert_eq!(created.rack_profile_id.as_str(), "NVL72");
-    assert_eq!(created.metadata.name, "test-rack");
-    assert_eq!(created.metadata.labels.get("env").unwrap(), "test");
-
-    let found = find_by_rack_id(&mut txn, &rack_id)
-        .await?
-        .expect("Should find the rack we just created");
-
-    assert_eq!(found.rack_id, rack_id);
-    assert_eq!(found.rack_profile_id.as_str(), "NVL72");
-
-    Ok(())
-}
-
 #[crate::sqlx_test]
 async fn test_db_duplicate_create(pool: sqlx::PgPool) -> Result<(), Box<dyn std::error::Error>> {
     let mut txn = pool.begin().await?;
diff --git a/crates/api-db/src/expected_switch/tests.rs b/crates/api-db/src/expected_switch/tests.rs
@@ -164,25 +164,14 @@ async fn test_update_bmc_credentials(pool: sqlx::PgPool) -> Result<(), Box<dyn s
 async fn test_delete(pool: sqlx::PgPool) -> () {
     let mut txn = pool.begin().await.unwrap();
     let switches = create_expected_switches(&mut txn).await;
-
-    let switch = &switches[0];
-
-    assert_eq!(switch.serial_number, "SW-SN-001");
-
-    db::expected_switch::delete_by_mac(&mut txn, switch.bmc_mac_address)
-        .await
-        .expect("Error deleting expected_switch");
-
+    let mac = switches[0].bmc_mac_address;
     txn.commit().await.expect("Failed to commit transaction");
-    let mut txn = pool
-        .begin()
-        .await
-        .expect("unable to create transaction on database pool");
 
-    assert!(
-        db::expected_switch::find_by_bmc_mac_address(&mut txn, switches[0].bmc_mac_address)
-            .await
-            .unwrap()
-            .is_none()
+    crate::test_support::expected_host::assert_delete_by_mac_removes_row(
+        &pool,
+        mac,
+        async |txn, mac| db::expected_switch::delete_by_mac(txn, mac).await,
+        async |txn, mac| db::expected_switch::find_by_bmc_mac_address(txn, mac).await,
     )
+    .await;
 }
diff --git a/crates/api-db/src/ip_allocator.rs b/crates/api-db/src/ip_allocator.rs
@@ -575,6 +575,9 @@ fn next_available_prefix(
 
 #[cfg(test)]
 mod tests {
+    use carbide_test_support::Outcome::*;
+    use carbide_test_support::{Case, check_cases};
+
     use super::*;
 
     #[test]
@@ -911,14 +914,55 @@ mod tests {
 
     #[test]
     fn test_v4_candidate() {
+        // A /30 from 192.168.1.0/24 always lands on 192.168.1.8/30 once .0 and the
+        // .4/30 block are taken — and stays there however the already-allocated set
+        // is spelled: with a bare pair, with a duplicate /32, or with a smaller /31
+        // already covered by the .4/30. The candidate search collapses all three.
+        struct AllocatedSet {
+            scenario: &'static str,
+            allocated_cidrs: Vec<IpNetwork>,
+        }
+
         let cidr = "192.168.1.0/24".parse().unwrap();
         let prefix_length = 30;
-        let allocated_cidrs = vec![
-            "192.168.1.0/32".parse().unwrap(),
-            "192.168.1.4/30".parse().unwrap(),
-        ];
-        let next_prefix = next_available_prefix(cidr, prefix_length, allocated_cidrs).unwrap();
-        assert!(next_prefix.is_some_and(|prefix| prefix.to_string() == "192.168.1.8/30"));
+
+        check_cases(
+            [
+                AllocatedSet {
+                    scenario: "distinct networks",
+                    allocated_cidrs: vec![
+                        "192.168.1.0/32".parse().unwrap(),
+                        "192.168.1.4/30".parse().unwrap(),
+                    ],
+                },
+                AllocatedSet {
+                    scenario: "duplicate /32",
+                    allocated_cidrs: vec![
+                        "192.168.1.0/32".parse().unwrap(),
+                        "192.168.1.0/32".parse().unwrap(),
+                        "192.168.1.4/30".parse().unwrap(),
+                    ],
+                },
+                AllocatedSet {
+                    scenario: "covered /31 inside the /30",
+                    allocated_cidrs: vec![
+                        "192.168.1.0/32".parse().unwrap(),
+                        "192.168.1.0/32".parse().unwrap(),
+                        "192.168.1.4/31".parse().unwrap(),
+                        "192.168.1.4/30".parse().unwrap(),
+                    ],
+                },
+            ]
+            .map(|set| Case {
+                scenario: set.scenario,
+                input: set.allocated_cidrs,
+                expect: Yields(Some("192.168.1.8/30".parse().unwrap())),
+            }),
+            |allocated_cidrs| {
+                next_available_prefix(cidr, prefix_length, allocated_cidrs)
+                    .map_err(|e| e.to_string())
+            },
+        );
     }
 
     #[test]
@@ -936,33 +980,6 @@ mod tests {
         assert_eq!(next_prefix.ip().to_string(), "192.168.1.1");
     }
 
-    #[test]
-    fn test_v4_candidate_with_duplicate() {
-        let cidr = "192.168.1.0/24".parse().unwrap();
-        let prefix_length = 30;
-        let allocated_cidrs = vec![
-            "192.168.1.0/32".parse().unwrap(),
-            "192.168.1.0/32".parse().unwrap(),
-            "192.168.1.4/30".parse().unwrap(),
-        ];
-        let next_prefix = next_available_prefix(cidr, prefix_length, allocated_cidrs).unwrap();
-        assert!(next_prefix.is_some_and(|prefix| prefix.to_string() == "192.168.1.8/30"));
-    }
-
-    #[test]
-    fn test_v4_candidate_with_covered() {
-        let cidr = "192.168.1.0/24".parse().unwrap();
-        let prefix_length = 30;
-        let allocated_cidrs = vec![
-            "192.168.1.0/32".parse().unwrap(),
-            "192.168.1.0/32".parse().unwrap(),
-            "192.168.1.4/31".parse().unwrap(),
-            "192.168.1.4/30".parse().unwrap(),
-        ];
-        let next_prefix = next_available_prefix(cidr, prefix_length, allocated_cidrs).unwrap();
-        assert!(next_prefix.is_some_and(|prefix| prefix.to_string() == "192.168.1.8/30"));
-    }
-
     #[test]
     fn test_v6_candidate() {
         // Ode to the 2012 Aston Martin DB9.
diff --git a/crates/api-db/src/route_servers.rs b/crates/api-db/src/route_servers.rs
@@ -402,32 +402,31 @@ mod tests {
         Ok(())
     }
 
-    // test_sync_with_duplicate_addresses_in_input tests to
-    // make sure sync handles gracefully when the input array
-    // contains duplicate addresses.
+    // test_sync_with_duplicate_addresses_in_input pins the contract that
+    // `replace` does not de-duplicate its input: addresses are handed straight
+    // to a single batched INSERT, so a repeated address trips the
+    // `address inet NOT NULL UNIQUE` constraint and the whole call is rejected.
+    // Callers are responsible for passing a unique address set.
     #[crate::sqlx_test]
     async fn test_sync_with_duplicate_addresses_in_input(
         pool: sqlx::PgPool,
     ) -> Result<(), Box<dyn std::error::Error>> {
         let mut txn = pool.begin().await?;
 
-        // Create input with duplicates
+        // Input carrying the same address twice.
         let mut addresses = test_ips(3);
-        addresses.push(addresses[0]); // Add duplicate
+        addresses.push(addresses[0]);
 
-        // Should handle gracefully (database constraint will handle uniqueness)
         let result = super::replace(&mut txn, &addresses, RouteServerSourceType::ConfigFile).await;
 
-        // This might succeed (if database handles duplicates) or fail - both are acceptable
-        // The important thing is that it doesn't panic
+        // The duplicate must trip the UNIQUE constraint specifically, not merely
+        // fail for some unrelated reason.
         match result {
-            Ok(_) => {
-                let entries = super::get(txn.as_mut()).await?;
-                assert_eq!(entries.len(), 3); // Should only have unique entries
-            }
-            Err(_) => {
-                // Also acceptable if database rejects duplicates
-            }
+            Err(DatabaseError::Sqlx(e)) if matches!(&e.source, sqlx::Error::Database(db) if db.is_unique_violation()) =>
+                {}
+            other => panic!(
+                "replace should reject a duplicate address with a unique-violation error, got: {other:?}"
+            ),
         }
 
         Ok(())
diff --git a/crates/api-db/src/test_support/expected_host.rs b/crates/api-db/src/test_support/expected_host.rs
diff --git a/crates/api-db/src/test_support/mod.rs b/crates/api-db/src/test_support/mod.rs
diff --git a/crates/health/src/sharding.rs b/crates/health/src/sharding.rs
