psake / psake/PowerShellBuild

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

オープン
#220 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

まだ誰も着手していません。

bug
主要言語
PowerShell
スター
145
フォーク
27
平均マージ
10時間 16分
マージ済み PR(30日)
34

説明

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

コントリビューションガイド

コントリビューションガイドを開く

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

調査の方向性

tests/Test-PSBuildPester.tests.ps1 の BeforeDiscovery セットアップ付近と、305 行目の "honors the Pester version that is already loaded" テストから始めます。$innerPesterVersions、$oldestInnerVersion、$newestInnerVersion がどのように設定されるかを追跡し、その後、対象の Test-PSBuildPester テストを実行します。テストとそのコメントが、意味のあるカバレッジがいつ可能なのかを正確に反映していれば完了です。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
powershell
領域
testing-qa
issue の種類
バグ
難易度
3/5
見積もり時間
1〜2日
活発さ
活発
明瞭さ
おおむね明確
初心者へのやさしさ
55/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。