From 94bc1a9d73873e4350ab7d48d1a23cf81e1469d2 Mon Sep 17 00:00:00 2001 From: Clint Branham Date: Wed, 25 Mar 2026 16:27:04 -0500 Subject: [PATCH] fix: address Copilot review round 3 - Fix bare Skip-IfNoTarget calls in 13_Firewall and 14_Backup (missing if/return pattern caused tests to run when they should skip) - Validate modifier-only switches in dev.ps1 (-Force/-Reprovision without an action switch now errors instead of defaulting to -Shell) - Add force-cleanup to usage text in run-integration.sh - Add --connect-timeout/--max-time to guest agent curl in wait-for-pve.sh Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Integration/13_Firewall.Tests.ps1 | 26 +++++++++---------- .../Integration/14_Backup.Tests.ps1 | 10 +++---- tests/dev.ps1 | 5 +++- .../infrastructure/scripts/run-integration.sh | 3 ++- tests/infrastructure/scripts/wait-for-pve.sh | 2 +- 5 files changed, 25 insertions(+), 21 deletions(-) diff --git a/tests/PSProxmoxVE.Tests/Integration/13_Firewall.Tests.ps1 b/tests/PSProxmoxVE.Tests/Integration/13_Firewall.Tests.ps1 index 14c052f..64d41bd 100644 --- a/tests/PSProxmoxVE.Tests/Integration/13_Firewall.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Integration/13_Firewall.Tests.ps1 @@ -35,12 +35,12 @@ Describe 'Firewall — Integration' -Tag 'Integration' { Context 'Firewall — Cluster Rules' { It 'Should create a firewall rule' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { New-PveFirewallRule -Level Cluster -Type in -Action ACCEPT -Proto tcp -Dport '8080' -Comment 'pester-test-rule' -Enable -ErrorAction Stop } | Should -Not -Throw } It 'Should list firewall rules and find the test rule' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } $rules = Get-PveFirewallRule -Level Cluster $rules | Should -Not -BeNullOrEmpty $testRule = $rules | Where-Object { $_.Comment -eq 'pester-test-rule' } @@ -49,7 +49,7 @@ Describe 'Firewall — Integration' -Tag 'Integration' { } It 'Should update the firewall rule' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } if ($null -eq $script:FirewallTestRulePos) { Set-ItResult -Skipped -Because 'No test rule was created' } @@ -57,7 +57,7 @@ Describe 'Firewall — Integration' -Tag 'Integration' { } It 'Should remove the firewall rule' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } if ($null -eq $script:FirewallTestRulePos) { Set-ItResult -Skipped -Because 'No test rule was created' } @@ -65,7 +65,7 @@ Describe 'Firewall — Integration' -Tag 'Integration' { } It 'Should get firewall options' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } $opts = Get-PveFirewallOptions -Level Cluster $opts | Should -Not -BeNullOrEmpty } @@ -73,45 +73,45 @@ Describe 'Firewall — Integration' -Tag 'Integration' { Context 'Firewall — Aliases and IP Sets' { It 'Should create a firewall alias' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { New-PveFirewallAlias -Level Cluster -Name 'pester-alias' -Cidr '192.168.99.0/24' -Comment 'test alias' -ErrorAction Stop } | Should -Not -Throw } It 'Should list and find the alias' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } $aliases = Get-PveFirewallAlias -Level Cluster $aliases | Where-Object { $_.Name -eq 'pester-alias' } | Should -Not -BeNullOrEmpty } It 'Should remove the alias' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { Remove-PveFirewallAlias -Level Cluster -Name 'pester-alias' -Confirm:$false -ErrorAction Stop } | Should -Not -Throw } It 'Should create an IP set' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { New-PveFirewallIpSet -Level Cluster -Name 'pester-ipset' -Comment 'test ipset' -ErrorAction Stop } | Should -Not -Throw } It 'Should add an entry to the IP set' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { New-PveFirewallIpSetEntry -Level Cluster -Name 'pester-ipset' -Cidr '10.99.0.0/16' -Comment 'test entry' -ErrorAction Stop } | Should -Not -Throw } It 'Should list IP set entries' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } $entries = Get-PveFirewallIpSetEntry -Level Cluster -Name 'pester-ipset' $entries | Should -Not -BeNullOrEmpty ($entries | Where-Object { $_.Cidr -like '10.99.0.0*' }) | Should -Not -BeNullOrEmpty } It 'Should remove the IP set entry' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { Remove-PveFirewallIpSetEntry -Level Cluster -Name 'pester-ipset' -Cidr '10.99.0.0/16' -Confirm:$false -ErrorAction Stop } | Should -Not -Throw } It 'Should remove the IP set' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { Remove-PveFirewallIpSet -Level Cluster -Name 'pester-ipset' -Confirm:$false -ErrorAction Stop } | Should -Not -Throw } } diff --git a/tests/PSProxmoxVE.Tests/Integration/14_Backup.Tests.ps1 b/tests/PSProxmoxVE.Tests/Integration/14_Backup.Tests.ps1 index 26c7bcc..212ea6d 100644 --- a/tests/PSProxmoxVE.Tests/Integration/14_Backup.Tests.ps1 +++ b/tests/PSProxmoxVE.Tests/Integration/14_Backup.Tests.ps1 @@ -22,12 +22,12 @@ Describe 'Backup Jobs — Integration' -Tag 'Integration' { Context 'Backup Jobs' { It 'Should create a backup job' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } { New-PveBackupJob -Schedule 'sat 03:00' -Storage $script:Storage -Mode snapshot -All -Comment 'pester-test-backup' -ErrorAction Stop } | Should -Not -Throw } It 'Should list backup jobs and find the test job' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } $jobs = Get-PveBackupJob $testJob = $jobs | Where-Object { $_.Comment -eq 'pester-test-backup' } $testJob | Should -Not -BeNullOrEmpty @@ -35,7 +35,7 @@ Describe 'Backup Jobs — Integration' -Tag 'Integration' { } It 'Should update the backup job' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } if (-not $script:BackupTestJobId) { Set-ItResult -Skipped -Because 'No test backup job was created' } @@ -43,7 +43,7 @@ Describe 'Backup Jobs — Integration' -Tag 'Integration' { } It 'Should remove the backup job' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } if (-not $script:BackupTestJobId) { Set-ItResult -Skipped -Because 'No test backup job was created' } @@ -51,7 +51,7 @@ Describe 'Backup Jobs — Integration' -Tag 'Integration' { } It 'Should verify the backup job was removed' { - Skip-IfNoTarget + if (Skip-IfNoTarget) { return } if (-not $script:BackupTestJobId) { Set-ItResult -Skipped -Because 'No test backup job was created' } diff --git a/tests/dev.ps1 b/tests/dev.ps1 index 075fd12..de004df 100644 --- a/tests/dev.ps1 +++ b/tests/dev.ps1 @@ -114,9 +114,12 @@ param( $ErrorActionPreference = 'Stop' -# If no switches specified, default to -Shell +# If no action switches specified, default to -Shell $anySwitchSet = $Shell -or $Build -or $Test -or $Provision -or $Integration -or $Cleanup -or $Stop -or $Rebuild if (-not $anySwitchSet) { + if ($Force -or $Reprovision) { + throw "-Force and -Reprovision are modifiers — combine with an action switch (e.g. -Cleanup -Force, -Provision -Reprovision)." + } $Shell = $true } diff --git a/tests/infrastructure/scripts/run-integration.sh b/tests/infrastructure/scripts/run-integration.sh index 837948f..9d84554 100755 --- a/tests/infrastructure/scripts/run-integration.sh +++ b/tests/infrastructure/scripts/run-integration.sh @@ -707,12 +707,13 @@ main() { taint) cmd_taint "$@" ;; all) cmd_all "$@" ;; *) - echo "Usage: $(basename "$0") {provision|test|cleanup|taint|all} [8|9|all] [test-filter]" + echo "Usage: $(basename "$0") {provision|test|cleanup|force-cleanup|taint|all} [8|9|all] [test-filter]" echo "" echo "Subcommands:" echo " provision [8|9|all] Provision nested PVE VMs + storage containers" echo " test [8|9|all] [filter] Run integration tests (default: all versions, no filter)" echo " cleanup [8|9|all] Destroy resources via terraform destroy (default: all)" + echo " force-cleanup [8|9|all] Bypass Terraform — destroy via API + wipe state (recovery)" echo " taint [8|9|all] Mark VMs for recreation on next provision" echo " all [8|9|all] Full lifecycle: provision → test → cleanup" echo "" diff --git a/tests/infrastructure/scripts/wait-for-pve.sh b/tests/infrastructure/scripts/wait-for-pve.sh index 288c86b..e8e4078 100755 --- a/tests/infrastructure/scripts/wait-for-pve.sh +++ b/tests/infrastructure/scripts/wait-for-pve.sh @@ -22,7 +22,7 @@ echo "Waiting for guest agent on VM ${VM_ID} (node: ${PARENT_NODE})..." VM_IP="" elapsed=0 while [ $elapsed -lt $MAX_WAIT ]; do - AGENT_RESPONSE=$(curl -sk \ + AGENT_RESPONSE=$(curl -sk --connect-timeout 5 --max-time 10 \ -H "Authorization: PVEAPIToken=${PARENT_TOKEN}" \ "${PARENT_API}/nodes/${PARENT_NODE}/qemu/${VM_ID}/agent/network-get-interfaces" 2>/dev/null || true)