You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
tests/Test-PSBuildPester.tests.ps1, the honors the Pester version that is already loaded test at line 305, is skipped by a condition that can never be false. Its comment says otherwise, and that is the part most worth fixing.
What happens
It 'honors the Pester version that is already loaded'-Skip:($script:innerPesterVersions.Count-lt2) {
$innerPesterVersions is built in BeforeDiscovery as the newest installed Pester of each supported major:
The loop runs over a single major, and Select-Object -First 1 takes one version from it. The collection therefore holds at most one element, so Count -lt 2 is always true and the test is always skipped. Pester 6 has been the only supported major since #172, which removed the 5.x matrix.
The comment is wrong in a specific, misleading way
# This skips in CI as of #172. It needs two installed Pester versions to tell# "used the loaded one" apart from "imported the newest", and dropping the 5.x# matrix left exactly one. It still runs on a developer machine with more than one# Pester installed. Restoring it in CI would mean installing a second version that# is never imported -- the machinery #172 deleted -- so it is left skipped# deliberately rather than by oversight.
"It still runs on a developer machine with more than one Pester installed" is false. The collection is not "installed Pester versions"; it is "the newest installed version of each supported major". A machine can have any number of Pester versions installed and still produce a one-element collection.
Measured on a workstation with five Pester versions installed — 3.4.0, 5.7.1, 6.0.0, 6.0.1, and 6.1.0, comfortably "more than one Pester installed" — $innerPesterVersions resolves to a single entry, 6.1.0, and the test skips. There is no machine configuration, developer or otherwise, on which it runs.
Why it matters
The comment is the reason this went unnoticed and is the thing that will keep it unnoticed. It is written in the repository's usual careful register, it names the pull request that caused the situation, and it explicitly claims the skip is deliberate rather than oversight — so a reader auditing skipped tests has every reason to move on. A test that is permanently dead is a cost; a permanently dead test with a comment asserting it is alive is a trap.
The coverage that is actually lost is real but narrow. The regression it was written for was an unconditional Import-Module Pester -MinimumVersion 5.0.0 that loaded the newest installed Pester on top of an already-loaded older one, crashing on a Pester.dll version conflict. Test-PSBuildPester now checks the loaded version rather than forcing an import, and the assertion this test makes:
$result.LoadedModuleVersion['Pester'] | Should -Be @($script:oldestInnerVersion)
can only distinguish "used the loaded one" from "imported the newest" when the two differ — which needs two versions the harness is willing to pin. Everything else in that context uses $newestInnerVersion and passes.
Note also that the test's body would still be self-defeating if the skip were removed today, because $oldestInnerVersion and $newestInnerVersion resolve to the same single element, so the assertion would pass without distinguishing anything. That makes this the second defect in this list where two problems mask each other.
Options
Delete the test and its comment. Honest, and it removes the trap. It costs the only guard against the import regression coming back, which would show up as a hard crash rather than a silent wrong answer — so the cost is smaller than it looks.
Restore the coverage without a second Pester major. The inner job pins a version by importing it; a second minor version of Pester 6 would serve just as well as a second major for telling "used the loaded one" apart from "imported the newest", since the assertion compares exact version strings. Widening $innerPesterVersions to collect, say, the newest and the oldest installed 6.x would make the test run wherever two are present and skip cleanly where only one is — with a comment that says exactly that. It would run on the workstation measured above, and on CI only if a second 6.x were installed.
Keep it skipped but tell the truth. Change the condition to a plain -Skip with a comment saying it can never run as written and why, so the next reader is not misled. Cheapest, and it leaves a dead test in the file.
(2) is the option that keeps the coverage and matches what the comment currently promises. (1) is the right answer if the coverage is judged not worth the machinery — in which case the comment should go with it rather than being left to describe a test that is not there.
Related: #172 (removed the 5.x matrix and created this situation).
tests/Test-PSBuildPester.tests.ps1, thehonors the Pester version that is already loadedtest at line 305, is skipped by a condition that can never be false. Its comment says otherwise, and that is the part most worth fixing.What happens
$innerPesterVersionsis built inBeforeDiscoveryas the newest installed Pester of each supported major:The loop runs over a single major, and
Select-Object -First 1takes one version from it. The collection therefore holds at most one element, soCount -lt 2is always true and the test is always skipped. Pester 6 has been the only supported major since #172, which removed the 5.x matrix.The comment is wrong in a specific, misleading way
"It still runs on a developer machine with more than one Pester installed" is false. The collection is not "installed Pester versions"; it is "the newest installed version of each supported major". A machine can have any number of Pester versions installed and still produce a one-element collection.
Measured on a workstation with five Pester versions installed — 3.4.0, 5.7.1, 6.0.0, 6.0.1, and 6.1.0, comfortably "more than one Pester installed" —
$innerPesterVersionsresolves to a single entry,6.1.0, and the test skips. There is no machine configuration, developer or otherwise, on which it runs.Why it matters
The comment is the reason this went unnoticed and is the thing that will keep it unnoticed. It is written in the repository's usual careful register, it names the pull request that caused the situation, and it explicitly claims the skip is deliberate rather than oversight — so a reader auditing skipped tests has every reason to move on. A test that is permanently dead is a cost; a permanently dead test with a comment asserting it is alive is a trap.
The coverage that is actually lost is real but narrow. The regression it was written for was an unconditional
Import-Module Pester -MinimumVersion 5.0.0that loaded the newest installed Pester on top of an already-loaded older one, crashing on aPester.dllversion conflict.Test-PSBuildPesternow checks the loaded version rather than forcing an import, and the assertion this test makes:can only distinguish "used the loaded one" from "imported the newest" when the two differ — which needs two versions the harness is willing to pin. Everything else in that context uses
$newestInnerVersionand passes.Note also that the test's body would still be self-defeating if the skip were removed today, because
$oldestInnerVersionand$newestInnerVersionresolve to the same single element, so the assertion would pass without distinguishing anything. That makes this the second defect in this list where two problems mask each other.Options
$innerPesterVersionsto collect, say, the newest and the oldest installed 6.x would make the test run wherever two are present and skip cleanly where only one is — with a comment that says exactly that. It would run on the workstation measured above, and on CI only if a second 6.x were installed.-Skipwith a comment saying it can never run as written and why, so the next reader is not misled. Cheapest, and it leaves a dead test in the file.(2) is the option that keeps the coverage and matches what the comment currently promises. (1) is the right answer if the coverage is judged not worth the machinery — in which case the comment should go with it rather than being left to describe a test that is not there.
Related: #172 (removed the 5.x matrix and created this situation).