Skip to content

A skipped Test-PSBuildPester test can never run, and its comment says it does #220

Description

@tablackburn

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 -lt 2) {

$innerPesterVersions is built in BeforeDiscovery as the newest installed Pester of each supported major:

$script:innerPesterVersions = @(
    foreach ($majorVersion in 6) {
        $newestOfMajor = Get-Module -Name 'Pester' -ListAvailable |
            Where-Object { $_.Version.Major -eq $majorVersion } |
            Sort-Object -Property 'Version' -Descending |
            Select-Object -First 1
        ...

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

  1. 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.
  2. 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.
  3. 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.
  4. Restore it in CI. Install a second Pester version on the runner purely so this test has something to pin against. Pester version floors do not match what is tested, and Test-PSBuildPester's guard admits versions that cannot work #172 deliberately removed that machinery; re-adding it for one test is probably not worth it, and the comment is right about that much.

(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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions