Skip to content

Settle the FailBuildOnSeverityLevel contract: 'Any' is documented but rejected, and ParseError escapes every threshold #144

Description

@tablackburn

Found while adding tests for Test-PSBuildScriptAnalysis in #143. Three related problems in the severity-threshold contract, none of which belonged in that PR because resolving them means deciding on the public parameter surface.

1. 'Any' is documented but fails parameter binding

PowerShellBuild/build.properties.ps1 documents an 'Any' value:

"Any" will fail the build on any diagnostic record, regardless of severity.

But Test-PSBuildScriptAnalysis declares:

[ValidateSet('None', 'Error', 'Warning', 'Information')]
[string]$SeverityThreshold

So setting $PSBPreference.Test.ScriptAnalysis.FailBuildOnSeverityLevel = 'Any' fails parameter binding rather than doing what the comment promises.

2. The same comment contradicts itself

It opens with "Valid values are Error, Warning, Information and None", then goes on to describe 'Any' — which is not in that list. It also never explains what Information actually does, even though that is a real accepted value.

3. ParseError findings escape every threshold

This is the substantive one. PSScriptAnalyzer's severity enum has four members:

[enum]::GetNames([Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic.DiagnosticSeverity])
# Information Warning Error ParseError

Test-PSBuildScriptAnalysis only counts Error, Warning, and Information. A ParseError record — a file that does not parse at all — therefore fails no threshold, including the strictest validated value (Information). It is reported in the results table and the build passes.

The catch-all branch of the switch (if ($analysisRecords.Count -ne 0)) does have the desired behavior, but it is only reachable when no SeverityThreshold is supplied at all. The consumer-facing Analyze task always passes one, so consumers can never reach it. The documented-but-missing 'Any' was evidently meant to be the value that maps to it.

Why this matters for v1.0.0

SeverityThreshold is part of the public API being frozen at 1.0.0, so the accepted value set should be settled before the release rather than after. Adding a value is additive and non-breaking; removing or redefining one later would not be.

Options

  1. Add 'Any' to the ValidateSet and wire it to the catch-all branch, making the documented behavior real and giving consumers a way to catch ParseError.
  2. Count ParseError alongside Error in the existing thresholds, on the grounds that a file which does not parse is at least as severe as an analyzer error. This closes the gap without growing the parameter surface, but changes the meaning of the existing Error threshold.
  3. Both, which is likely the right answer: ParseError folded into Error for sane defaults, plus 'Any' for consumers who want to fail on absolutely everything.
  4. Documentation only — delete the 'Any' sentence and describe Information properly. Cheapest, but leaves ParseError silently escaping the gate.

Whichever is chosen, build.properties.ps1 needs its comment corrected to match, and the change needs test coverage in tests/Test-PSBuildScriptAnalysis.tests.ps1 (the severity-gating context there is already table-driven and takes a new severity easily).

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions