psake / psake/PowerShellBuild

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

Ouverte
#220 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub

Personne n'a encore pris cette issue.

bug
Langage dominant
PowerShell
Étoiles
145
Forks
27
Merge moyen
10 h 16 min
PR mergées (30 j)
34

Description

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

Guide de contribution

Ouvrir le guide de contribution

Par où commencer

  1. Lisez l'issue en entier, puis le guide de contribution du projet.
  2. Signalez en commentaire que vous la prenez — cela évite que deux personnes fassent le même travail.
  3. Forkez le dépôt et travaillez sur une branche.
  4. Ouvrez une pull request qui référence le numéro de l'issue.

Piste de recherche

Commencez dans tests/Test-PSBuildPester.tests.ps1, autour de la configuration de BeforeDiscovery et du test "honors the Pester version that is already loaded" à la ligne 305. Suivez la manière dont $innerPesterVersions, $oldestInnerVersion et $newestInnerVersion sont renseignées, puis exécutez les tests Test-PSBuildPester ciblés. La tâche est terminée lorsque le test et son commentaire reflètent correctement quand une couverture pertinente est possible.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Stack technique
powershell
Domaine
testing-qa
Type d'issue
Bug
Difficulté
3/5
Temps estimé
1-2 jours
Activité
Active
Clarté
Plutôt claire
Accessibilité débutants
55/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.