diff --git a/Cargo.lock b/Cargo.lock index 782218a..3b00cae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1329,6 +1329,7 @@ dependencies = [ "paste", "rand 0.9.2", "regex", + "scopeguard", "serde", "serde_json", "tempfile", diff --git a/crates/integration-tests/Cargo.toml b/crates/integration-tests/Cargo.toml index 4b388ef..daacd9c 100644 --- a/crates/integration-tests/Cargo.toml +++ b/crates/integration-tests/Cargo.toml @@ -35,6 +35,7 @@ regex = "1" linkme = "0.3.30" paste = "1.0" rand = { workspace = true } +scopeguard = "1" [lints] workspace = true diff --git a/crates/integration-tests/src/main.rs b/crates/integration-tests/src/main.rs index c8c9b82..93bc7d5 100644 --- a/crates/integration-tests/src/main.rs +++ b/crates/integration-tests/src/main.rs @@ -1,7 +1,5 @@ //! Integration tests for bcvk -use std::process::Output; - use camino::Utf8Path; use color_eyre::eyre::{eyre, Context}; @@ -84,36 +82,6 @@ pub(crate) fn get_all_test_images() -> Vec { } } -/// Captured output from a command with decoded stdout/stderr strings -pub(crate) struct CapturedOutput { - pub output: Output, - pub stdout: String, - pub stderr: String, -} - -impl CapturedOutput { - /// Create from a raw Output - pub fn new(output: Output) -> Self { - let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); - let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); - Self { - output, - stdout, - stderr, - } - } - - /// Get the exit code - pub fn exit_code(&self) -> Option { - self.output.status.code() - } - - /// Check if the command succeeded - pub fn success(&self) -> bool { - self.output.status.success() - } -} - fn test_images_list() -> Result<()> { println!("Running test: bcvk images list --json"); diff --git a/crates/integration-tests/src/tests/libvirt_base_disks.rs b/crates/integration-tests/src/tests/libvirt_base_disks.rs index f8a4e89..6ae372f 100644 --- a/crates/integration-tests/src/tests/libvirt_base_disks.rs +++ b/crates/integration-tests/src/tests/libvirt_base_disks.rs @@ -8,6 +8,7 @@ use color_eyre::Result; use integration_tests::integration_test; +use scopeguard::defer; use xshell::cmd; use regex::Regex; @@ -36,27 +37,21 @@ fn test_base_disk_creation_and_reuse() -> Result<()> { cleanup_domain(&vm1_name); cleanup_domain(&vm2_name); + // Set up cleanup guard that will run on scope exit (success or failure) + defer! { + cleanup_domain(&vm1_name); + cleanup_domain(&vm2_name); + } + // Create first VM - this should create a new base disk println!("Creating first VM (should create base disk)..."); - let vm1_output = cmd!( + let vm1_stdout = cmd!( sh, "{bck} libvirt run --name {vm1_name} --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - let vm1_stdout = String::from_utf8_lossy(&vm1_output.stdout); - let vm1_stderr = String::from_utf8_lossy(&vm1_output.stderr); + .read()?; println!("VM1 stdout: {}", vm1_stdout); - println!("VM1 stderr: {}", vm1_stderr); - - if !vm1_output.status.success() { - cleanup_domain(&vm1_name); - cleanup_domain(&vm2_name); - - panic!("Failed to create first VM: {}", vm1_stderr); - } // Verify base disk was created assert!( @@ -66,26 +61,11 @@ fn test_base_disk_creation_and_reuse() -> Result<()> { // Create second VM - this should reuse the base disk println!("Creating second VM (should reuse base disk)..."); - let vm2_output = cmd!( + let vm2_stdout = cmd!( sh, "{bck} libvirt run --name {vm2_name} --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - let vm2_stdout = String::from_utf8_lossy(&vm2_output.stdout); - let vm2_stderr = String::from_utf8_lossy(&vm2_output.stderr); - - println!("VM2 stdout: {}", vm2_stdout); - println!("VM2 stderr: {}", vm2_stderr); - - // Cleanup before assertions - cleanup_domain(&vm1_name); - cleanup_domain(&vm2_name); - - if !vm2_output.status.success() { - panic!("Failed to create second VM: {}", vm2_stderr); - } + .read()?; // Verify base disk was reused (should be faster and mention using existing) assert!( @@ -129,35 +109,21 @@ fn test_base_disks_list_command() -> Result<()> { println!("Testing base-disks list command"); - let output = cmd!(sh, "{bck} libvirt base-disks list") - .ignore_status() - .output()?; - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - - if output.status.success() { - println!("base-disks list output: {}", stdout); - - // Should show table header or empty message - assert!( - stdout.contains("NAME") - || stdout.contains("No base disk") - || stdout.contains("no base disk") - || stdout.is_empty(), - "Should show table format or empty message, got: {}", - stdout - ); - - println!("✓ base-disks list command works"); - } else { - println!("base-disks list failed (may be expected): {}", stderr); - - // Should fail gracefully - assert!( - stderr.contains("pool") || stderr.contains("libvirt") || stderr.contains("connect"), - "Should have meaningful error about libvirt connectivity" - ); - } + let stdout = cmd!(sh, "{bck} libvirt base-disks list").read()?; + + println!("base-disks list output: {}", stdout); + + // Should show table header or empty message + assert!( + stdout.contains("NAME") + || stdout.contains("No base disk") + || stdout.contains("no base disk") + || stdout.is_empty(), + "Should show table format or empty message, got: {}", + stdout + ); + + println!("✓ base-disks list command works"); Ok(()) } integration_test!(test_base_disks_list_command); @@ -169,31 +135,17 @@ fn test_base_disks_prune_dry_run() -> Result<()> { println!("Testing base-disks prune --dry-run command"); - let output = cmd!(sh, "{bck} libvirt base-disks prune --dry-run") - .ignore_status() - .output()?; - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - - if output.status.success() { - println!("base-disks prune --dry-run output: {}", stdout); - - // Should show what would be removed or indicate nothing to prune - assert!( - stdout.contains("Would remove") || stdout.contains("No") || stdout.is_empty(), - "Should show dry-run output" - ); - - println!("✓ base-disks prune --dry-run command works"); - } else { - println!("base-disks prune failed (may be expected): {}", stderr); - - // Should fail gracefully - assert!( - stderr.contains("pool") || stderr.contains("libvirt") || stderr.contains("connect"), - "Should have meaningful error about libvirt connectivity" - ); - } + let stdout = cmd!(sh, "{bck} libvirt base-disks prune --dry-run").read()?; + + println!("base-disks prune --dry-run output: {}", stdout); + + // Should show what would be removed or indicate nothing to prune + assert!( + stdout.contains("Would remove") || stdout.contains("No") || stdout.is_empty(), + "Should show dry-run output" + ); + + println!("✓ base-disks prune --dry-run command works"); Ok(()) } integration_test!(test_base_disks_prune_dry_run); @@ -214,20 +166,17 @@ fn test_vm_disk_references_base() -> Result<()> { cleanup_domain(&vm_name); + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&vm_name); + } + // Create VM - let output = cmd!( + cmd!( sh, "{bck} libvirt run --name {vm_name} --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - cleanup_domain(&vm_name); - - panic!("Failed to create VM: {}", stderr); - } + .run()?; // Get VM disk path from domain XML let sh = shell()?; @@ -247,8 +196,6 @@ fn test_vm_disk_references_base() -> Result<()> { .get("file") .expect("No file attribute found in source element"); - cleanup_domain(&vm_name); - println!("VM disk path: {}", disk_path); // Disk should be named after the VM, not a base disk diff --git a/crates/integration-tests/src/tests/libvirt_port_forward.rs b/crates/integration-tests/src/tests/libvirt_port_forward.rs index 787a848..ded9d82 100644 --- a/crates/integration-tests/src/tests/libvirt_port_forward.rs +++ b/crates/integration-tests/src/tests/libvirt_port_forward.rs @@ -7,6 +7,7 @@ use color_eyre::Result; use integration_tests::integration_test; +use scopeguard::defer; use xshell::cmd; use crate::{get_bck_command, get_test_image, shell, LIBVIRT_INTEGRATION_TEST_LABEL}; @@ -25,25 +26,13 @@ fn test_libvirt_port_forward_parsing() -> Result<()> { ]; for ports in valid_port_tests { - let output = cmd!(sh, "{bck} libvirt run {ports...} --help") - .ignore_status() - .output()?; + let output = cmd!(sh, "{bck} libvirt run {ports...} --help").output()?; let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); - if !output.status.success() { - assert!( - !stderr.contains("invalid") && !stderr.contains("parse"), - "Port forwarding options should be parsed correctly: {:?}, stderr: {}", - ports, - stderr - ); - } else { - assert!( - stdout.contains("Usage") || stdout.contains("USAGE"), - "Should show help output when using --help" - ); - } + assert!( + stdout.contains("Usage") || stdout.contains("USAGE"), + "Should show help output when using --help" + ); } println!("✓ Port forwarding argument parsing validated"); @@ -132,29 +121,20 @@ fn test_libvirt_port_forward_xml() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&domain_name); + } + // Create domain with port forwarding println!("Creating libvirt domain with port forwarding..."); - let create_output = cmd!( + let create_stdout = cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --port 8080:80 --port 9090:8080 --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - let create_stdout = String::from_utf8_lossy(&create_output.stdout); - let create_stderr = String::from_utf8_lossy(&create_output.stderr); + .read()?; println!("Create stdout: {}", create_stdout); - println!("Create stderr: {}", create_stderr); - - if !create_output.status.success() { - cleanup_domain(&domain_name); - panic!( - "Failed to create domain with port forwarding: {}", - create_stderr - ); - } - println!("Successfully created domain: {}", domain_name); // Verify port forwarding in output @@ -193,10 +173,6 @@ fn test_libvirt_port_forward_xml() -> Result<()> { ); println!("✓ Domain XML contains expected port forwarding configuration"); - - // Cleanup domain - cleanup_domain(&domain_name); - println!("✓ Port forwarding XML configuration test passed"); Ok(()) } @@ -230,31 +206,21 @@ fn test_libvirt_port_forward_connectivity() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&domain_name); + } + // Create domain with port forwarding (forward host_port to guest port 8080) println!( "Creating libvirt domain with port forwarding {}:8080...", host_port ); - let create_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --port {port_mapping} --filesystem ext4 --ssh-wait {test_image}" ) - .ignore_status() - .output()?; - - let create_stdout = String::from_utf8_lossy(&create_output.stdout); - let create_stderr = String::from_utf8_lossy(&create_output.stderr); - - println!("Create stdout: {}", create_stdout); - println!("Create stderr: {}", create_stderr); - - if !create_output.status.success() { - cleanup_domain(&domain_name); - panic!( - "Failed to create domain with port forwarding: {}", - create_stderr - ); - } + .run()?; println!("Successfully created domain: {}", domain_name); @@ -267,18 +233,11 @@ fn test_libvirt_port_forward_connectivity() -> Result<()> { // Create a test file to serve println!("Creating test file in VM..."); let create_file_cmd = "echo 'port-forward-test-success' > /tmp/test.txt"; - let create_file_output = cmd!( + cmd!( sh, "{bck} libvirt ssh --timeout 10 {domain_name} -- sh -c {create_file_cmd}" ) - .ignore_status() - .output()?; - - if !create_file_output.status.success() { - let stderr = String::from_utf8_lossy(&create_file_output.stderr); - cleanup_domain(&domain_name); - panic!("Failed to create test file in VM: {}", stderr); - } + .run()?; println!("✓ Test file created successfully"); // Start HTTP server in background @@ -290,18 +249,11 @@ fn test_libvirt_port_forward_connectivity() -> Result<()> { println!("Starting background HTTP server..."); let start_server_cmd = "(cd /tmp && exec python3 -m http.server 8080 > /tmp/http.log 2>&1 < /dev/null &) && sleep 0.1"; - let start_server_output = cmd!( + cmd!( sh, "{bck} libvirt ssh --timeout 10 {domain_name} -- bash -c {start_server_cmd}" ) - .ignore_status() - .output()?; - - if !start_server_output.status.success() { - let stderr = String::from_utf8_lossy(&start_server_output.stderr); - cleanup_domain(&domain_name); - panic!("Failed to start HTTP server in VM: {}", stderr); - } + .run()?; println!("✓ HTTP server command executed"); // Wait a bit for the server to start @@ -354,9 +306,6 @@ fn test_libvirt_port_forward_connectivity() -> Result<()> { } } - // Cleanup domain before assertions - cleanup_domain(&domain_name); - assert!( connection_success, "Failed to connect to forwarded port after {} attempts", diff --git a/crates/integration-tests/src/tests/libvirt_verb.rs b/crates/integration-tests/src/tests/libvirt_verb.rs index f7b418f..db2c56f 100644 --- a/crates/integration-tests/src/tests/libvirt_verb.rs +++ b/crates/integration-tests/src/tests/libvirt_verb.rs @@ -9,6 +9,7 @@ use color_eyre::Result; use integration_tests::integration_test; +use scopeguard::defer; use xshell::cmd; use crate::{get_bck_command, get_test_image, shell, LIBVIRT_INTEGRATION_TEST_LABEL}; @@ -29,28 +30,15 @@ fn test_libvirt_list_functionality() -> Result<()> { let sh = shell()?; let bck = get_bck_command()?; - let output = cmd!(sh, "{bck} libvirt list").ignore_status().output()?; - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = cmd!(sh, "{bck} libvirt list").read()?; - // May succeed or fail depending on libvirt availability, but should not crash - if output.status.success() { - println!("libvirt list succeeded: {}", stdout); - // Should show domain listing format - assert!( - stdout.contains("NAME") - || stdout.contains("No VMs found") - || stdout.contains("No running VMs found"), - "Should show domain listing format or empty message" - ); - } else { - println!("libvirt list failed (expected in CI): {}", stderr); - // Verify it fails with proper error message about libvirt connectivity - assert!( - stderr.contains("libvirt") || stderr.contains("connect") || stderr.contains("virsh"), - "Should have meaningful error about libvirt connectivity" - ); - } + // Should show domain listing format + assert!( + stdout.contains("NAME") + || stdout.contains("No VMs found") + || stdout.contains("No running VMs found"), + "Should show domain listing format or empty message" + ); println!("libvirt list functionality tested"); Ok(()) @@ -62,28 +50,16 @@ fn test_libvirt_list_json_output() -> Result<()> { let sh = shell()?; let bck = get_bck_command()?; - let output = cmd!(sh, "{bck} libvirt list --format json") - .ignore_status() - .output()?; - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = cmd!(sh, "{bck} libvirt list --format json").read()?; - if output.status.success() { - // If successful, should be valid JSON - let json_result: std::result::Result = serde_json::from_str(&stdout); - assert!( - json_result.is_ok(), - "libvirt list --format json should produce valid JSON: {}", - stdout - ); - println!("libvirt list --format json produced valid JSON"); - } else { - // May fail in CI without libvirt, but should mention error handling - println!( - "libvirt list --format json failed (expected in CI): {}", - stderr - ); - } + // Should be valid JSON + let json_result: std::result::Result = serde_json::from_str(&stdout); + assert!( + json_result.is_ok(), + "libvirt list --format json should produce valid JSON: {}", + stdout + ); + println!("libvirt list --format json produced valid JSON"); println!("libvirt list JSON output tested"); Ok(()) @@ -103,26 +79,12 @@ fn test_libvirt_run_resource_options() -> Result<()> { ]; for resources in resource_tests { - let output = cmd!(sh, "{bck} libvirt run {resources...} --help") - .ignore_status() - .output()?; - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = cmd!(sh, "{bck} libvirt run {resources...} --help").read()?; - // Should show help and not fail on resource parsing - if !output.status.success() { - assert!( - !stderr.contains("invalid") && !stderr.contains("parse"), - "Resource options should be parsed correctly: {:?}, stderr: {}", - resources, - stderr - ); - } else { - assert!( - stdout.contains("Usage") || stdout.contains("USAGE"), - "Should show help output when using --help" - ); - } + assert!( + stdout.contains("Usage") || stdout.contains("USAGE"), + "Should show help output when using --help" + ); } println!("libvirt run resource options validated"); @@ -142,26 +104,12 @@ fn test_libvirt_run_networking() -> Result<()> { ]; for network in network_configs { - let output = cmd!(sh, "{bck} libvirt run {network...} --help") - .ignore_status() - .output()?; - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let stdout = cmd!(sh, "{bck} libvirt run {network...} --help").read()?; - if !output.status.success() { - // Should not fail on network option parsing - assert!( - !stderr.contains("invalid") && !stderr.contains("parse"), - "Network options should be parsed correctly: {:?}, stderr: {}", - network, - stderr - ); - } else { - assert!( - stdout.contains("Usage") || stdout.contains("USAGE"), - "Should show help output when using --help" - ); - } + assert!( + stdout.contains("Usage") || stdout.contains("USAGE"), + "Should show help output when using --help" + ); } println!("libvirt run networking options validated"); @@ -214,25 +162,18 @@ fn test_libvirt_comprehensive_workflow() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&domain_name); + } + // Create domain with multiple features: instancetype, labels, SSH println!("Creating libvirt domain with instancetype and labels..."); - let create_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --label test-workflow --itype u1.small --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - let create_stdout = String::from_utf8_lossy(&create_output.stdout); - let create_stderr = String::from_utf8_lossy(&create_output.stderr); - - println!("Create stdout: {}", create_stdout); - println!("Create stderr: {}", create_stderr); - - if !create_output.status.success() { - cleanup_domain(&domain_name); - panic!("Failed to create domain: {}", create_stderr); - } + .run()?; println!("Successfully created domain: {}", domain_name); @@ -333,7 +274,7 @@ fn test_libvirt_comprehensive_workflow() -> Result<()> { // Test 6: Test `rm -f` stops running VMs by default println!("Test 6: Testing `rm -f` stops running VMs without --stop flag..."); - let rm_test_domain = create_test_vm_and_assert("test-rm", &test_image, &domain_name)?; + let rm_test_domain = create_test_vm_and_assert("test-rm", &test_image)?; // Verify it's running let rm_info = cmd!(sh, "virsh dominfo {rm_test_domain}").read()?; @@ -361,7 +302,12 @@ fn test_libvirt_comprehensive_workflow() -> Result<()> { // Test 7: Test `run --replace` replaces existing VM println!("Test 7: Testing `run --replace` replaces existing VM..."); - let replace_test_domain = create_test_vm_and_assert("test-replace", &test_image, &domain_name)?; + let replace_test_domain = create_test_vm_and_assert("test-replace", &test_image)?; + + // Set up cleanup guard for replace_test_domain + defer! { + cleanup_domain(&replace_test_domain); + } // Verify initial VM exists let initial_domain_list = cmd!(sh, "virsh list --all --name").read()?; @@ -376,22 +322,11 @@ fn test_libvirt_comprehensive_workflow() -> Result<()> { "Running `bcvk libvirt run --replace --name {}`...", replace_test_domain ); - let replace_output = cmd!( + cmd!( sh, "{bck} libvirt run --replace --name {replace_test_domain} --label {label} --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - if !replace_output.status.success() { - let replace_stderr = String::from_utf8_lossy(&replace_output.stderr); - cleanup_domain(&replace_test_domain); - cleanup_domain(&domain_name); - panic!( - "Failed to replace VM with --replace flag: {}", - replace_stderr - ); - } + .run()?; println!("✓ Successfully replaced VM with --replace flag"); // Verify VM still exists with same name @@ -410,13 +345,6 @@ fn test_libvirt_comprehensive_workflow() -> Result<()> { ); println!("✓ Replaced VM is running (fresh instance)"); - // Cleanup replace test VM - cleanup_domain(&replace_test_domain); - println!("✓ Cleaned up replace test VM"); - - // Cleanup original domain - cleanup_domain(&domain_name); - println!("✓ Comprehensive workflow test passed"); Ok(()) } @@ -461,31 +389,19 @@ fn cleanup_domain(domain_name: &str) { /// Helper function to create a test VM and assert success /// /// Creates a VM using cmd! with the given prefix and test image. -/// Cleans up the original domain on error and returns the created domain name. -fn create_test_vm_and_assert( - domain_prefix: &str, - test_image: &str, - original_domain: &str, -) -> Result { +/// Returns the created domain name on success. +fn create_test_vm_and_assert(domain_prefix: &str, test_image: &str) -> Result { let sh = shell()?; let bck = get_bck_command()?; let label = LIBVIRT_INTEGRATION_TEST_LABEL; let domain_name = format!("{}-{}", domain_prefix, random_suffix()); println!("Creating test VM: {}", domain_name); - let create_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - if !create_output.status.success() { - let stderr = String::from_utf8_lossy(&create_output.stderr); - cleanup_domain(&domain_name); - cleanup_domain(original_domain); - panic!("Failed to create test VM '{}': {}", domain_name, stderr); - } + .run()?; println!("✓ Test VM created: {}", domain_name); Ok(domain_name) @@ -615,28 +531,18 @@ fn test_libvirt_run_bind_storage_ro() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); - // Create domain with --bind-storage-ro flag + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&domain_name); + } + + // Create domain with --bind-storage-ro flag and wait for SSH println!("Creating libvirt domain with --bind-storage-ro..."); - let create_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --bind-storage-ro --filesystem ext4 --ssh-wait {test_image}" ) - .ignore_status() - .output()?; - - let create_stdout = String::from_utf8_lossy(&create_output.stdout); - let create_stderr = String::from_utf8_lossy(&create_output.stderr); - - println!("Create stdout: {}", create_stdout); - println!("Create stderr: {}", create_stderr); - - if !create_output.status.success() { - cleanup_domain(&domain_name); - panic!( - "Failed to create domain with --bind-storage-ro: {}", - create_stderr - ); - } + .run()?; println!("Successfully created domain: {}", domain_name); @@ -679,13 +585,9 @@ fn test_libvirt_run_bind_storage_ro() -> Result<()> { println!("✓ Container storage mount is configured as read-only"); println!("✓ hoststorage tag is present in filesystem configuration"); - // SSH is already available due to --ssh-wait flag + // SSH is already available due to --ssh-wait flag, VM is ready println!("✓ SSH is ready (via --ssh-wait)"); - // Wait for automatic mount to complete - println!("Waiting for VM to boot and automatic mount to complete..."); - std::thread::sleep(std::time::Duration::from_secs(10)); - // Test SSH connection and verify container storage is automatically mounted println!( "Verifying container storage is automatically mounted at /run/host-container-storage..." @@ -710,10 +612,6 @@ fn test_libvirt_run_bind_storage_ro() -> Result<()> { "Mount should be read-only, but write operation succeeded" ); println!("✓ Mount is correctly configured as read-only."); - - // Cleanup domain before completing test - cleanup_domain(&domain_name); - println!("✓ --bind-storage-ro end-to-end test passed"); Ok(()) } @@ -737,28 +635,18 @@ fn test_libvirt_run_no_storage_opts_without_bind_storage() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&domain_name); + } + // Create domain WITHOUT --bind-storage-ro flag println!("Creating libvirt domain without --bind-storage-ro..."); - let create_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - let create_stdout = String::from_utf8_lossy(&create_output.stdout); - let create_stderr = String::from_utf8_lossy(&create_output.stderr); - - println!("Create stdout: {}", create_stdout); - println!("Create stderr: {}", create_stderr); - - if !create_output.status.success() { - cleanup_domain(&domain_name); - panic!( - "Failed to create domain without --bind-storage-ro: {}", - create_stderr - ); - } + .run()?; println!("Successfully created domain: {}", domain_name); @@ -798,10 +686,6 @@ fn test_libvirt_run_no_storage_opts_without_bind_storage() -> Result<()> { "Domain XML should NOT contain bind-storage-ro metadata when flag is not used. Found in XML." ); println!("✓ Domain XML does not contain bind-storage-ro metadata"); - - // Cleanup domain - cleanup_domain(&domain_name); - println!("✓ Test passed: STORAGE_OPTS credentials are correctly excluded when --bind-storage-ro is not used"); Ok(()) } @@ -897,25 +781,19 @@ fn test_libvirt_run_transient_vm() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); + // For transient VMs, the domain is automatically removed when stopped, + // so we use a defer guard only for cleanup on early error + defer! { + cleanup_domain(&domain_name); + } + // Create transient domain println!("Creating transient libvirt domain..."); - let create_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --transient --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - let create_stdout = String::from_utf8_lossy(&create_output.stdout); - let create_stderr = String::from_utf8_lossy(&create_output.stderr); - - println!("Create stdout: {}", create_stdout); - println!("Create stderr: {}", create_stderr); - - if !create_output.status.success() { - cleanup_domain(&domain_name); - panic!("Failed to create transient domain: {}", create_stderr); - } + .run()?; println!("Successfully created transient domain: {}", domain_name); @@ -1024,20 +902,18 @@ fn test_libvirt_run_transient_replace() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&domain_name); + } + // Create initial transient domain println!("Creating initial transient domain..."); - let create_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --transient --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - if !create_output.status.success() { - let stderr = String::from_utf8_lossy(&create_output.stderr); - cleanup_domain(&domain_name); - panic!("Failed to create initial transient domain: {}", stderr); - } + .run()?; println!("✓ Initial transient domain created: {}", domain_name); let sh = shell()?; @@ -1053,23 +929,11 @@ fn test_libvirt_run_transient_replace() -> Result<()> { // Now replace the transient domain with another transient domain println!("Replacing transient domain with --transient --replace..."); - let replace_output = cmd!( + cmd!( sh, "{bck} libvirt run --name {domain_name} --label {label} --transient --replace --filesystem ext4 {test_image}" ) - .ignore_status() - .output()?; - - let replace_stdout = String::from_utf8_lossy(&replace_output.stdout); - let replace_stderr = String::from_utf8_lossy(&replace_output.stderr); - - println!("Replace stdout: {}", replace_stdout); - println!("Replace stderr: {}", replace_stderr); - - if !replace_output.status.success() { - cleanup_domain(&domain_name); - panic!("Failed to replace transient domain: {}", replace_stderr); - } + .run()?; println!("✓ Successfully replaced transient domain"); // Verify the new domain exists and is transient @@ -1088,9 +952,6 @@ fn test_libvirt_run_transient_replace() -> Result<()> { dominfo ); println!("✓ Replaced transient domain is running"); - - // Cleanup - cleanup_domain(&domain_name); println!("✓ Transient --replace test passed"); Ok(()) } @@ -1135,31 +996,22 @@ fn test_libvirt_run_bind_mounts() -> Result<()> { // Cleanup any existing domain with this name cleanup_domain(&domain_name); + // Set up cleanup guard that will run on scope exit + defer! { + cleanup_domain(&domain_name); + } + // Create domain with bind mounts and test karg println!("Creating libvirt domain with bind mounts and karg..."); let rw_bind = format!("{}:/var/mnt/test-rw", rw_dir_path); let ro_bind = format!("{}:/var/mnt/test-ro", ro_dir_path); - let create_output = cmd!( + // Use --ssh-wait to properly wait for VM to be ready instead of arbitrary sleep + cmd!( sh, - "{bck} libvirt run --name {domain_name} --label {label} --filesystem ext4 --karg bcvk.test-install-karg=1 --bind {rw_bind} --bind-ro {ro_bind} {test_image}" + "{bck} libvirt run --name {domain_name} --label {label} --filesystem ext4 --ssh-wait --karg bcvk.test-install-karg=1 --bind {rw_bind} --bind-ro {ro_bind} {test_image}" ) - .ignore_status() - .output()?; - - let create_stdout = String::from_utf8_lossy(&create_output.stdout); - let create_stderr = String::from_utf8_lossy(&create_output.stderr); - - println!("Create stdout: {}", create_stdout); - println!("Create stderr: {}", create_stderr); - - if !create_output.status.success() { - cleanup_domain(&domain_name); - panic!( - "Failed to create domain with bind mounts: {}", - create_stderr - ); - } + .run()?; println!("Successfully created domain: {}", domain_name); @@ -1182,33 +1034,7 @@ fn test_libvirt_run_bind_mounts() -> Result<()> { println!("✓ Domain XML contains virtiofs and SMBIOS credentials"); - // Wait for VM to boot and mounts to be ready - println!("Waiting for VM to boot and mounts to be ready..."); - std::thread::sleep(std::time::Duration::from_secs(15)); - - // Debug: Check systemd credentials - println!("Debugging: Checking systemd credentials..."); - let _ = cmd!( - sh, - "{bck} libvirt ssh {domain_name} -- ls -la /run/credentials" - ) - .ignore_status() - .run(); - - // Debug: Check mount units - println!("Debugging: Checking mount units..."); - let _ = cmd!( - sh, - "{bck} libvirt ssh {domain_name} -- systemctl list-units *.mount" - ) - .ignore_status() - .run(); - - // Debug: Check mount status - println!("Debugging: Checking if mounts exist..."); - let _ = cmd!(sh, "{bck} libvirt ssh {domain_name} -- mount") - .ignore_status() - .run(); + // VM is ready (--ssh-wait ensures this), now test the mounts // Test read-write bind mount - verify file exists and is readable println!("Testing read-write bind mount..."); @@ -1278,10 +1104,6 @@ fn test_libvirt_run_bind_mounts() -> Result<()> { cmdline_stdout ); println!("✓ Kernel argument validated"); - - // Cleanup domain - cleanup_domain(&domain_name); - println!("✓ Bind mounts and karg test passed"); Ok(()) } diff --git a/crates/integration-tests/src/tests/to_disk.rs b/crates/integration-tests/src/tests/to_disk.rs index ea6fe8f..0a5c08b 100644 --- a/crates/integration-tests/src/tests/to_disk.rs +++ b/crates/integration-tests/src/tests/to_disk.rs @@ -14,6 +14,8 @@ //! - "This is acceptable in CI/testing environments" //! - Warning and continuing on failures +use std::process::Output; + use camino::Utf8PathBuf; use color_eyre::Result; use integration_tests::{integration_test, parameterized_integration_test}; @@ -21,7 +23,7 @@ use xshell::cmd; use tempfile::TempDir; -use crate::{get_bck_command, get_test_image, shell, CapturedOutput, INTEGRATION_TEST_LABEL}; +use crate::{get_bck_command, get_test_image, shell, INTEGRATION_TEST_LABEL}; /// Validate that a disk image was created successfully with proper bootc installation /// @@ -32,11 +34,7 @@ use crate::{get_bck_command, get_test_image, shell, CapturedOutput, INTEGRATION_ /// /// Note: sfdisk can only read partition tables from raw disk images, not qcow2. /// For qcow2 images, partition validation is skipped. -fn validate_disk_image( - disk_path: &Utf8PathBuf, - output: &CapturedOutput, - context: &str, -) -> Result<()> { +fn validate_disk_image(disk_path: &Utf8PathBuf, output: &Output, context: &str) -> Result<()> { let metadata = std::fs::metadata(disk_path).expect("Failed to get disk metadata"); assert!(metadata.len() > 0, "{}: Disk image is empty", context); @@ -65,11 +63,13 @@ fn validate_disk_image( ); } + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); assert!( - output.stdout.contains("Installation complete") || output.stderr.contains("Installation complete"), + stdout.contains("Installation complete") || stderr.contains("Installation complete"), "{}: No 'Installation complete' message found in output. This indicates bootc install did not complete successfully. stdout: {}, stderr: {}", context, - output.stdout, output.stderr + stdout, stderr ); Ok(()) @@ -86,23 +86,7 @@ fn test_to_disk() -> Result<()> { let disk_path = Utf8PathBuf::try_from(temp_dir.path().join("test-disk.img")) .expect("temp path is not UTF-8"); - let raw_output = cmd!(sh, "{bck} to-disk --label {label} {image} {disk_path}") - .ignore_status() - .output()?; - let output = CapturedOutput::new(std::process::Output { - status: raw_output.status, - stdout: raw_output.stdout, - stderr: raw_output.stderr, - }); - - assert!( - output.success(), - "to-disk failed with exit code: {:?}. stdout: {}, stderr: {}", - output.exit_code(), - output.stdout, - output.stderr - ); - + let output = cmd!(sh, "{bck} to-disk --label {label} {image} {disk_path}").output()?; validate_disk_image(&disk_path, &output, "test_to_disk")?; Ok(()) } @@ -119,25 +103,11 @@ fn test_to_disk_qcow2() -> Result<()> { let disk_path = Utf8PathBuf::try_from(temp_dir.path().join("test-disk.qcow2")) .expect("temp path is not UTF-8"); - let raw_output = cmd!( + let output = cmd!( sh, "{bck} to-disk --format=qcow2 --label {label} {image} {disk_path}" ) - .ignore_status() .output()?; - let output = CapturedOutput::new(std::process::Output { - status: raw_output.status, - stdout: raw_output.stdout, - stderr: raw_output.stderr, - }); - - assert!( - output.success(), - "to-disk with qcow2 failed with exit code: {:?}. stdout: {}, stderr: {}", - output.exit_code(), - output.stdout, - output.stderr - ); // Verify the file is actually qcow2 format using qemu-img info let qemu_img_stdout = cmd!(sh, "qemu-img info {disk_path}").read()?; @@ -165,55 +135,28 @@ fn test_to_disk_caching() -> Result<()> { .expect("temp path is not UTF-8"); // First run: Create the disk image - let raw_output1 = cmd!(sh, "{bck} to-disk --label {label} {image} {disk_path}") - .ignore_status() - .output()?; - let output1 = CapturedOutput::new(std::process::Output { - status: raw_output1.status, - stdout: raw_output1.stdout, - stderr: raw_output1.stderr, - }); - - assert!( - output1.success(), - "First to-disk run failed with exit code: {:?}. stdout: {}, stderr: {}", - output1.exit_code(), - output1.stdout, - output1.stderr - ); + let output1 = cmd!(sh, "{bck} to-disk --label {label} {image} {disk_path}").output()?; + let stdout1 = String::from_utf8_lossy(&output1.stdout); + let stderr1 = String::from_utf8_lossy(&output1.stderr); let metadata1 = std::fs::metadata(&disk_path).expect("Failed to get disk metadata after first run"); assert!(metadata1.len() > 0, "Disk image is empty after first run"); assert!( - output1.stdout.contains("Installation complete") - || output1.stderr.contains("Installation complete"), + stdout1.contains("Installation complete") || stderr1.contains("Installation complete"), "No 'Installation complete' message found in first run output" ); // Second run: Should reuse the cached disk - let raw_output2 = cmd!(sh, "{bck} to-disk --label {label} {image} {disk_path}") - .ignore_status() - .output()?; - let output2 = CapturedOutput::new(std::process::Output { - status: raw_output2.status, - stdout: raw_output2.stdout, - stderr: raw_output2.stderr, - }); + let output2 = cmd!(sh, "{bck} to-disk --label {label} {image} {disk_path}").output()?; + let stdout2 = String::from_utf8_lossy(&output2.stdout); + let stderr2 = String::from_utf8_lossy(&output2.stderr); assert!( - output2.success(), - "Second to-disk run failed with exit code: {:?}. stdout: {}, stderr: {}", - output2.exit_code(), - output2.stdout, - output2.stderr - ); - - assert!( - output2.stdout.contains("Reusing existing cached disk image"), + stdout2.contains("Reusing existing cached disk image"), "Second run should have reused cached disk, but cache reuse message not found. stdout: {}, stderr: {}", - output2.stdout, output2.stderr + stdout2, stderr2 ); let metadata2 = @@ -225,7 +168,7 @@ fn test_to_disk_caching() -> Result<()> { ); assert!( - !output2.stdout.contains("Installation complete") && !output2.stderr.contains("Installation complete"), + !stdout2.contains("Installation complete") && !stderr2.contains("Installation complete"), "Second run should not have performed installation, but found 'Installation complete' message" ); Ok(()) @@ -251,22 +194,7 @@ fn test_to_disk_different_imgref_same_digest() -> Result<()> { let disk_path = Utf8PathBuf::try_from(temp_dir.path().join("test-disk.img")) .expect("temp path is not UTF-8"); - let raw_output1 = cmd!(sh, "{bck} to-disk --label {label} {test_image} {disk_path}") - .ignore_status() - .output()?; - let output1 = CapturedOutput::new(std::process::Output { - status: raw_output1.status, - stdout: raw_output1.stdout, - stderr: raw_output1.stderr, - }); - - assert!( - output1.success(), - "First to-disk run failed with exit code: {:?}. stdout: {}, stderr: {}", - output1.exit_code(), - output1.stdout, - output1.stderr - ); + cmd!(sh, "{bck} to-disk --label {label} {test_image} {disk_path}").run()?; let metadata1 = std::fs::metadata(&disk_path).expect("Failed to get disk metadata after first run"); @@ -274,32 +202,20 @@ fn test_to_disk_different_imgref_same_digest() -> Result<()> { // Use --dry-run with the aliased image reference (same digest, different imgref) // to verify it would regenerate instead of reusing the cache - let raw_output2 = cmd!( + let output2 = cmd!( sh, "{bck} to-disk --dry-run --label {label} {second_tag} {disk_path}" ) - .ignore_status() .output()?; - let output2 = CapturedOutput::new(std::process::Output { - status: raw_output2.status, - stdout: raw_output2.stdout, - stderr: raw_output2.stderr, - }); - - assert!( - output2.success(), - "Dry-run with different imgref failed with exit code: {:?}. stdout: {}, stderr: {}", - output2.exit_code(), - output2.stdout, - output2.stderr - ); + let stdout2 = String::from_utf8_lossy(&output2.stdout); + let stderr2 = String::from_utf8_lossy(&output2.stderr); // The dry-run should report it would regenerate because the source imgref is different assert!( - output2.stdout.contains("would-regenerate"), + stdout2.contains("would-regenerate"), "Dry-run should report 'would-regenerate' for different imgref. stdout: {}, stderr: {}", - output2.stdout, - output2.stderr + stdout2, + stderr2 ); // Clean up: remove the aliased tag @@ -326,26 +242,11 @@ fn test_to_disk_for_image(image: &str) -> Result<()> { .expect("temp path is not UTF-8"); // Not all images have a default filesystem, so explicitly specify ext4 - let raw_output = cmd!( + let output = cmd!( sh, "{bck} to-disk --label {label} --filesystem=ext4 {image} {disk_path}" ) - .ignore_status() .output()?; - let output = CapturedOutput::new(std::process::Output { - status: raw_output.status, - stdout: raw_output.stdout, - stderr: raw_output.stderr, - }); - - assert!( - output.success(), - "to-disk with image {} failed with exit code: {:?}. stdout: {}, stderr: {}", - image, - output.exit_code(), - output.stdout, - output.stderr - ); validate_disk_image( &disk_path,