Test-PSBuildPester unloads a module it never imported

[tablackburn]

Test-PSBuildPester removes the module named by -ModuleName whether or not it ever imported it. The import is conditional on -ImportModule; the removal is not.

What happens

try {
    if ($ImportModule) {                                              # line 82
        if (-not (Test-Path $ModuleManifest)) {
            Write-Error ($LocalizedData.UnableToFindModuleManifest -f $ModuleManifest)
        } else {
            Get-Module $ModuleName | Remove-Module -Force -ErrorAction SilentlyContinue
            Import-Module $ModuleManifest -Force
        }
    }
    ...
} finally {
    Pop-Location
    # ModuleName is optional; Remove-Module with an empty -Name raises a parameter-binding
    # error that -ErrorAction SilentlyContinue cannot suppress.
    if ($ModuleName) {
        Remove-Module -Name $ModuleName -ErrorAction SilentlyContinue  # line 159
    }
}

The finally guards on $ModuleName being non-empty. It does not guard on $ImportModule. When -ImportModule is not passed, the function imports nothing and still removes whatever the caller had loaded under that name.

Measured

A caller loads a module, then calls Test-PSBuildPester with -ModuleName but without -ImportModule:

BEFORE: Probe loaded = 1; Get-Alpha -> alpha
AFTER : Probe loaded = 0; Get-Alpha -> gone

Why it matters

This fires on the default path, not an opt-in one. ImportModule defaults to $false in build.properties.ps1 (line 85), and both task files forward $PSBPreference.Test.ImportModule verbatim:

# psakeFile.ps1:162 and IB.tasks.ps1:101
ImportModule = $PSBPreference.Test.ImportModule

So for every consumer who has not set $PSBPreference.Test.ImportModule = $true — which is every consumer who has not gone looking for it — the Pester task takes the branch where the function imports nothing, and then unloads their module anyway. Test depends on Pester, so ./build.ps1 and ./build.ps1 -Task Test both do this.

It is worse than the same defect in Build-PSBuildMarkdown (#221) in two ways. There, the function at least imported something before removing it, so the removal has an internal justification and only over-reaches by scope. Here there is nothing to clean up at all — the removal is pure side effect on a session the function did not touch. And the Pester task runs on far more builds than GenerateMarkdown does, since a consumer who disabled documentation generation still runs tests.

The consequence is the same either way: after a build, a command that worked beforehand is no longer recognised, with nothing in the build output explaining why.

Root cause is shared

Both functions treat Remove-Module -Name as an undo for an import. It is not — it removes every loaded module with that name, regardless of who loaded it, from where, or whether anything was loaded by this function at all. Whatever approach is chosen for Build-PSBuildMarkdown should be applied here as well, and the two are probably one change.

Options

1. Move the removal inside the if ($ImportModule) guard, and remove by ModuleInfo. Capture what the import produced, remove that, and restore anything the caller had loaded first. This is the minimum correct behaviour: clean up exactly what was created, and nothing else. Cheap, and it makes the finally guard say what it means.
2. Drop the removal entirely. If -ImportModule was passed, leaving the module imported is a defensible end state — the consumer explicitly asked for it to be loaded, and a test task that leaves the tested module loaded is not surprising. Simplest change, and it makes the function non-destructive at the cost of a session side effect that is at least additive.
3. Fix both functions together with a shared helper. A small private function that imports a module, records what was displaced, and restores it, used by both Test-PSBuildPester and Build-PSBuildMarkdown. More work, and it keeps the two from drifting apart the way the psake and Invoke-Build task files have.

(1) is the smallest change that is correct here. (3) is worth considering if Build-PSBuildMarkdown is being fixed at the same time, since the two would otherwise grow two different answers to the same question.

One caution for whichever is chosen: the comment above line 159 documents a real regression — Remove-Module with an empty -Name raises a parameter-binding error that -ErrorAction SilentlyContinue cannot suppress — and tests/Test-PSBuildPester.tests.ps1 has a test for it (does not error when ModuleName is not provided). Whatever replaces this block needs to keep behaving correctly when ModuleName is absent.

Related: #221, Build-PSBuildMarkdown, which has the same root cause and the same options.