Fix $PSDefaultParameterValues leak causing tests to skip unexpectedly#26602
Fix $PSDefaultParameterValues leak causing tests to skip unexpectedly#26602
Conversation
- AclCmdlets.Tests.ps1: Clone and restore original values - SessionOption.Tests.ps1: Clone and restore original values - CimAdapter.Tests.ps1: Clone and restore original values - Rename-Computer.Tests.ps1: Clone and restore original values All files now properly save the original $PSDefaultParameterValues before modification and restore it to $global:PSDefaultParameterValues in their cleanup blocks, preventing the It:Skip setting from leaking to other tests. Co-authored-by: iSazonov <22290914+iSazonov@users.noreply.github.com>
|
@copilot The issue is not still fixed. See log https://github.com/PowerShell/PowerShell/actions/runs/20123556684/job/57749053868?pr=26602 - all tests starting with "Describing Resources strings in Microsoft.Management.Infrastructure.CimCmdlets (was -ResGen used with Start-PSBuild)" are still skipped. So a root of the issue is in a test before the test.
|
- RemoteSession.Basic.Tests.ps1: Remove unconditional Pop that was executed when test wasn't skipped - FileSignature.Tests.ps1: Add missing Pop when test is skipped These tests use Push-DefaultParameterValueStack in BeforeAll when skipping, but were not properly calling Pop-DefaultParameterValueStack in AfterAll, causing the skip setting to leak to subsequent tests like CimCmdletsResources.Tests.ps1. Co-authored-by: iSazonov <22290914+iSazonov@users.noreply.github.com>
Found and fixed the root cause. Two additional test files had
The RemoteSession.Basic.Tests.ps1 file runs before CimCmdletsResources.Tests.ps1 (alphabetically under engine/Remoting vs engine/ResourceValidation), so its leak was causing the CimCmdlets tests to be skipped. Fixed in commit 2665e2e.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes test isolation issues where six test files were leaking $PSDefaultParameterValues["It:Skip"] settings to subsequent tests, causing widespread test skipping on Linux. The fixes address two distinct patterns: using .Remove() which doesn't restore the original state (4 files), and imbalanced Push-DefaultParameterValueStack/Pop-DefaultParameterValueStack calls (2 files).
Key changes:
- Replace
.Remove()pattern with proper state cloning and restoration in 4 test files - Fix imbalanced Push/Pop calls in 2 test files to ensure proper cleanup even when tests are skipped
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/powershell/engine/Security/FileSignature.Tests.ps1 | Adds missing Pop-DefaultParameterValueStack call in AfterAll when test is skipped |
| test/powershell/engine/Remoting/SessionOption.Tests.ps1 | Replaces .Remove() with clone/restore pattern to properly restore state |
| test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1 | Removes unconditional Pop that had no matching Push, fixing stack balance |
| test/powershell/engine/ETS/CimAdapter.Tests.ps1 | Replaces .Remove() with clone/restore pattern to properly restore state |
| test/powershell/Modules/Microsoft.PowerShell.Security/AclCmdlets.Tests.ps1 | Replaces .Remove() with clone/restore pattern to properly restore state |
| test/powershell/Modules/Microsoft.PowerShell.Management/Rename-Computer.Tests.ps1 | Replaces .Remove() with clone/restore pattern to properly restore state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| AfterAll { | ||
| if ($shouldSkip) { | ||
| Pop-DefaultParameterValueStack |
There was a problem hiding this comment.
This is the root cause that results in tests get skipped on Linux.
|
From the copilot session:
Practically, I don't think the changes regarding Comparison before and after this fix for "Linux Unelevated Others":
1170 tests were incorrectly skipped due to the leaking |
I don't want :-) |
|
@daxian-dbw I believe it should be backported. |
…PowerShell#26602) This fixes test isolation issues where six test files were leaking $PSDefaultParameterValues["It:Skip"] settings to subsequent tests, causing widespread test skipping on Linux. The fixes address two distinct patterns: using .Remove() which doesn't restore the original state (4 files), and imbalanced Push-DefaultParameterValueStack/Pop-DefaultParameterValueStack calls (2 files).
…PowerShell#26602) This fixes test isolation issues where six test files were leaking $PSDefaultParameterValues["It:Skip"] settings to subsequent tests, causing widespread test skipping on Linux. The fixes address two distinct patterns: using .Remove() which doesn't restore the original state (4 files), and imbalanced Push-DefaultParameterValueStack/Pop-DefaultParameterValueStack calls (2 files).
…PowerShell#26602) This fixes test isolation issues where six test files were leaking $PSDefaultParameterValues["It:Skip"] settings to subsequent tests, causing widespread test skipping on Linux. The fixes address two distinct patterns: using .Remove() which doesn't restore the original state (4 files), and imbalanced Push-DefaultParameterValueStack/Pop-DefaultParameterValueStack calls (2 files).
…PowerShell#26602) This fixes test isolation issues where six test files were leaking $PSDefaultParameterValues["It:Skip"] settings to subsequent tests, causing widespread test skipping on Linux. The fixes address two distinct patterns: using .Remove() which doesn't restore the original state (4 files), and imbalanced Push-DefaultParameterValueStack/Pop-DefaultParameterValueStack calls (2 files).
PR Summary
Fix #26601
Six test files were leaking
$PSDefaultParameterValues["It:Skip"]settings to subsequent tests, causing widespread test skipping on Linux. The leaks occurred through two different patterns: using.Remove()instead of properly restoring state, and imbalancedPush-DefaultParameterValueStack/Pop-DefaultParameterValueStackcalls.PR Context
When test files modify
$PSDefaultParameterValues["It:Skip"]and don't properly restore the original state, the modification persists in the PowerShell session and affects all subsequent test files. This caused tests that should run on Linux to be skipped, particularly affecting resource validation tests like CimCmdletsResources.Tests.ps1.Two patterns of leakage were identified:
Pattern 1: Using .Remove() (4 files fixed)
Files using
.Remove("It:Skip")which doesn't restore the original state:test/powershell/Modules/Microsoft.PowerShell.Security/AclCmdlets.Tests.ps1test/powershell/engine/Remoting/SessionOption.Tests.ps1test/powershell/engine/ETS/CimAdapter.Tests.ps1test/powershell/Modules/Microsoft.PowerShell.Management/Rename-Computer.Tests.ps1Before (leaked):
BeforeAll { $PSDefaultParameterValues["It:Skip"] = -not $IsWindows } AfterAll { $PSDefaultParameterValues.Remove("It:Skip") # Insufficient }After (isolated):
BeforeAll { $originalDefaultParameterValues = $PSDefaultParameterValues.Clone() $PSDefaultParameterValues["It:Skip"] = -not $IsWindows } AfterAll { $global:PSDefaultParameterValues = $originalDefaultParameterValues }Pattern 2: Imbalanced Push/Pop (2 files fixed)
Files with imbalanced
Push-DefaultParameterValueStack/Pop-DefaultParameterValueStackcalls:test/powershell/engine/Remoting/RemoteSession.Basic.Tests.ps1- Had unconditionalPopwithout correspondingPushtest/powershell/engine/Security/FileSignature.Tests.ps1- MissingPopwhen test was skippedRemoteSession.Basic.Tests.ps1 Before:
AfterAll { if ($skipTest) { Pop-DefaultParameterValueStack return } Pop-DefaultParameterValueStack # This Pop had no matching Push! }After:
AfterAll { if ($skipTest) { Pop-DefaultParameterValueStack } }FileSignature.Tests.ps1 Before:
BeforeAll { if ($shouldSkip) { Push-DefaultParameterValueStack @{ "it:skip" = $shouldSkip } return } } AfterAll { if ($shouldSkip) { return # Missing Pop! } }After:
AfterAll { if ($shouldSkip) { Pop-DefaultParameterValueStack return } }The RemoteSession.Basic.Tests.ps1 leak was particularly impactful as it runs before ResourceValidation tests in alphabetical order (engine/Remoting/ before engine/ResourceValidation/), causing CimCmdletsResources.Tests.ps1 and other subsequent tests to be skipped.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.