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
- 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.
- 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.
- 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.
- 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
Found while adding tests for
Test-PSBuildScriptAnalysisin #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 bindingPowerShellBuild/build.properties.ps1documents an'Any'value:But
Test-PSBuildScriptAnalysisdeclares: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 whatInformationactually does, even though that is a real accepted value.3.
ParseErrorfindings escape every thresholdThis is the substantive one. PSScriptAnalyzer's severity enum has four members:
Test-PSBuildScriptAnalysisonly countsError,Warning, andInformation. AParseErrorrecord — 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 noSeverityThresholdis supplied at all. The consumer-facingAnalyzetask 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
SeverityThresholdis 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
'Any'to theValidateSetand wire it to the catch-all branch, making the documented behavior real and giving consumers a way to catchParseError.ParseErroralongsideErrorin 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 existingErrorthreshold.ParseErrorfolded intoErrorfor sane defaults, plus'Any'for consumers who want to fail on absolutely everything.'Any'sentence and describeInformationproperly. Cheapest, but leavesParseErrorsilently escaping the gate.Whichever is chosen,
build.properties.ps1needs its comment corrected to match, and the change needs test coverage intests/Test-PSBuildScriptAnalysis.tests.ps1(the severity-gating context there is already table-driven and takes a new severity easily).Related