diff --git a/CHANGELOG.md b/CHANGELOG.md index cd8089f..dd54893 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed +- [**#144**](https://github.com/psake/PowerShellBuild/issues/144) + **Breaking:** `Test-PSBuildScriptAnalysis` now counts PSScriptAnalyzer + `ParseError` records alongside `Error`. A file that does not parse at all + previously satisfied no threshold — not even the strictest validated value, + `Information` — so it was reported and the build passed anyway. It now fails + every threshold except `None`. See the + [v0.8 → v1.0 migration guide](docs/migration-v0.8-to-v1.0.md) — a build that + passed before may now correctly fail. + - [**#120**](https://github.com/psake/PowerShellBuild/issues/120) **Breaking:** the module manifest now requires PowerShell 5.1 or newer (`PowerShellVersion = '5.1'`, previously `'3.0'`) and declares @@ -18,8 +27,27 @@ and this project adheres to [Semantic Versioning](http://semver.org/). [v0.8 → v1.0 migration guide](docs/migration-v0.8-to-v1.0.md) for details. +### Added + +- [**#144**](https://github.com/psake/PowerShellBuild/issues/144) + `Test-PSBuildScriptAnalysis` accepts `Any` as a `SeverityThreshold`, failing + the build on any diagnostic record regardless of severity. `Any` was already + documented in `build.properties.ps1` but was missing from the parameter's + `ValidateSet`, so setting + `$PSBPreference.Test.ScriptAnalysis.FailBuildOnSeverityLevel = 'Any'` failed + parameter binding instead of working as documented. + ### Fixed +- [**#147**](https://github.com/psake/PowerShellBuild/issues/147) + `Test-PSBuildScriptAnalysis` retries the analysis when a PSScriptAnalyzer rule + crashes on an internal race + ([PSScriptAnalyzer#1538](https://github.com/PowerShell/PSScriptAnalyzer/issues/1538)), + which is unrelated to the code being analyzed and succeeds on a re-run. + Consumers who set `$ErrorActionPreference = 'Stop'` — common in a build script — + previously got a randomly red build. A crash that survives every attempt is + still surfaced, so a persistent failure behaves as it did before. + - [**#96**](https://github.com/psake/PowerShellBuild/issues/96) `Test-PSBuildScriptAnalysis` now fails the build when PSScriptAnalyzer reports findings at or above the configured severity threshold. The diff --git a/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 b/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 index deff786..62e359a 100644 --- a/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 +++ b/PowerShellBuild/Public/Test-PSBuildScriptAnalysis.ps1 @@ -8,19 +8,32 @@ function Test-PSBuildScriptAnalysis { Path to PowerShell module directory to run ScriptAnalyzer on. .PARAMETER SeverityThreshold Fail ScriptAnalyzer test if any issues are found with this threshold or higher. + + 'None' reports findings without ever failing the build. 'Information', 'Warning', and + 'Error' fail on a finding at that severity or higher. 'Any' fails on any diagnostic + record at all, regardless of severity. + + PSScriptAnalyzer also emits ParseError records for files that do not parse. Those are + counted alongside Error, so a file that cannot be parsed fails every threshold except + 'None'. .PARAMETER SettingsPath Path to ScriptAnalyzer settings to use. .EXAMPLE PS> Test-PSBuildScriptAnalysis -Path ./Output/MyModule/0.1.0 -SeverityThreshold Error Run ScriptAnalyzer on built module in ./Output/MyModule/0.1.0. Throw error if any errors are found. + .EXAMPLE + PS> Test-PSBuildScriptAnalysis -Path ./Output/MyModule/0.1.0 -SeverityThreshold Any + + Run ScriptAnalyzer on built module in ./Output/MyModule/0.1.0. Throw error if any + diagnostic record is returned, regardless of its severity. #> [CmdletBinding()] param( [parameter(Mandatory)] [string]$Path, - [ValidateSet('None', 'Error', 'Warning', 'Information')] + [ValidateSet('None', 'Error', 'Warning', 'Information', 'Any')] [string]$SeverityThreshold, [string]$SettingsPath @@ -38,13 +51,46 @@ function Test-PSBuildScriptAnalysis { $invokeScriptAnalyzerParameters.Settings = $SettingsPath } - $analysisResult = Invoke-ScriptAnalyzer @invokeScriptAnalyzerParameters -Verbose:$VerbosePreference + # PSScriptAnalyzer runs its script rules in parallel against a process-wide, unsynchronised + # singleton, so a rule can crash on an internal race that has nothing to do with the code + # under analysis (PSScriptAnalyzer#1538, #1351 -- both open, deferred to 2.0). A re-run + # succeeds, so retry rather than letting a random red build reach the consumer. The analyzer + # isolates a failed rule and still returns every other rule's findings, so a crash is not by + # itself a reason to fail. See psake/PowerShellBuild#147. + # + # Errors are captured rather than allowed to surface during the retries, because a consumer + # with $ErrorActionPreference = 'Stop' would otherwise terminate on the first crash and never + # reach the second attempt. Whatever remains after the final attempt is re-emitted below, so + # a persistent failure behaves exactly as it did before this retry existed. + $maximumAttempt = 3 + for ($attempt = 1; $attempt -le $maximumAttempt; $attempt++) { + $analysisErrors = @() + $analysisResult = Invoke-ScriptAnalyzer @invokeScriptAnalyzerParameters ` + -Verbose:$VerbosePreference -ErrorAction SilentlyContinue -ErrorVariable analysisErrors + + # Only the analyzer's own rule crashes are worth retrying. Anything else is a real + # failure that a second attempt will not change. + $ruleErrors = @($analysisErrors).Where({ $_.FullyQualifiedErrorId -like 'RULE_ERROR*' }) + if ($ruleErrors.Count -eq 0 -or $attempt -eq $maximumAttempt) { + break + } + + Write-Warning ($LocalizedData.ScriptAnalyzerRuleErrorRetry -f $attempt, $maximumAttempt, $ruleErrors[0].Exception.Message) + } + + # Surface anything the final attempt still reported, honouring the caller's error preference. + foreach ($analysisError in @($analysisErrors)) { + Write-Error -ErrorRecord $analysisError + } # A single diagnostic record comes back as a scalar rather than a collection, and Windows # PowerShell 5.1 does not expose .Where() or .Count on every scalar type. Wrapping in @() # guarantees collection semantics on both engines. $analysisRecords = @($analysisResult) - $errorCount = ($analysisRecords.Where({ $_.Severity -eq 'Error' })).Count + # ParseError is a fourth PSScriptAnalyzer severity, reported for a file that does not parse + # at all. It is counted with Error: a file the engine cannot read is at least as severe as + # an analyzer error, and leaving it out let it escape every threshold. + $errorCount = ($analysisRecords.Where({ $_.Severity -in @('Error', 'ParseError') })).Count $warningCount = ($analysisRecords.Where({ $_.Severity -eq 'Warning' })).Count $informationCount = ($analysisRecords.Where({ $_.Severity -eq 'Information' })).Count @@ -72,6 +118,11 @@ function Test-PSBuildScriptAnalysis { throw $LocalizedData.ScriptAnalyzerWarnings } } + 'Any' { + if ($analysisRecords.Count -gt 0) { + throw $LocalizedData.ScriptAnalyzerIssues + } + } default { if ($analysisRecords.Count -ne 0) { throw $LocalizedData.ScriptAnalyzerIssues diff --git a/PowerShellBuild/build.properties.ps1 b/PowerShellBuild/build.properties.ps1 index 2f1c0b0..541cda9 100644 --- a/PowerShellBuild/build.properties.ps1 +++ b/PowerShellBuild/build.properties.ps1 @@ -66,10 +66,14 @@ $moduleVersion = (Import-PowerShellDataFile -Path $env:BHPSModuleManifest).Modul Enabled = $true # When PSScriptAnalyzer is enabled, control which severity level will generate a build failure. - # Valid values are Error, Warning, Information and None. "None" will report errors but will not - # cause a build failure. "Error" will fail the build only on diagnostic records that are of - # severity error. "Warning" will fail the build on Warning and Error diagnostic records. - # "Any" will fail the build on any diagnostic record, regardless of severity. + # Valid values are None, Information, Warning, Error, and Any. + # "None" reports findings but never fails the build. + # "Information" fails the build on Information, Warning, and Error records. + # "Warning" fails the build on Warning and Error records. + # "Error" fails the build on Error records. + # "Any" fails the build on any diagnostic record, regardless of severity. + # PSScriptAnalyzer also reports ParseError records for files that do not parse at all. + # Those are counted with Error, so an unparsable file fails every level except "None". FailBuildOnSeverityLevel = 'Error' # Path to the PSScriptAnalyzer settings file. diff --git a/PowerShellBuild/en-US/Messages.psd1 b/PowerShellBuild/en-US/Messages.psd1 index 22f370d..9149e18 100644 --- a/PowerShellBuild/en-US/Messages.psd1 +++ b/PowerShellBuild/en-US/Messages.psd1 @@ -24,6 +24,7 @@ PSScriptAnalyzerResults=PSScriptAnalyzer results: ScriptAnalyzerErrors=One or more ScriptAnalyzer errors were found! ScriptAnalyzerWarnings=One or more ScriptAnalyzer warnings were found! ScriptAnalyzerIssues=One or more ScriptAnalyzer issues were found! +ScriptAnalyzerRuleErrorRetry=A PSScriptAnalyzer rule failed on attempt {0} of {1} and the analysis will be retried. This is an analyzer race, not a problem with the code being analyzed: {2} NoCertificateFound=No valid code signing certificate was found. Verify the configured CertificateSource and that a certificate with a private key is available. CertificateResolvedFromStore=Resolved code signing certificate from store [{0}]: Subject=[{1}] CertificateResolvedFromThumbprint=Resolved code signing certificate by thumbprint [{0}]: Subject=[{1}] diff --git a/docs/migration-v0.8-to-v1.0.md b/docs/migration-v0.8-to-v1.0.md index 04b0b6e..6268436 100644 --- a/docs/migration-v0.8-to-v1.0.md +++ b/docs/migration-v0.8-to-v1.0.md @@ -25,6 +25,9 @@ One line per break; follow the link for details and migration steps. - [Script analysis now actually fails the build](#script-analysis-now-actually-fails-the-build) — the `Analyze` task's severity threshold never fired in 0.8.x; a build that passed before may now correctly fail. +- [Unparsable files now fail the script analysis gate](#unparsable-files-now-fail-the-script-analysis-gate) + — `ParseError` findings are counted with `Error`, so a file that does not + parse fails every threshold except `None`. > More entries will follow as the Phase 2 migrations to > Microsoft.PowerShell.PlatyPS 1.x and psake 5.x land. @@ -156,6 +159,47 @@ now runs as documented instead of throwing. Tracked in issue #96. +### Unparsable files now fail the script analysis gate + +PSScriptAnalyzer's severity enum has four members — `Information`, +`Warning`, `Error`, and `ParseError`. In 0.8.x, +`Test-PSBuildScriptAnalysis` counted only the first three. A +`ParseError` record — a file that does not parse at all — therefore +satisfied no threshold, including the strictest one available +(`Information`): the record was printed in the results table and the +build passed. + +`ParseError` is now counted alongside `Error`, so a file the engine +cannot even read fails every threshold except `None`. A file that does +not parse cannot be meaningfully analyzed, so this closes a gap where +the most severe possible finding was the only one that could never fail +a build. + +**No configuration change is required.** If your build starts failing at +the `Analyze` task after upgrading and the reported record has severity +`ParseError`, the file genuinely does not parse — fix the syntax error. +It was being reported on 0.8.x too; it just never failed anything. + +To check before you upgrade: + + Invoke-ScriptAnalyzer -Path ./Output/MyModule/1.0.0 -Recurse | + Where-Object Severity -eq 'ParseError' + +Any output there is what will start failing your build. + +Alongside this, the severity threshold gains an **`Any`** value, which +fails the build on any diagnostic record regardless of severity: + + $PSBPreference.Test.ScriptAnalysis.FailBuildOnSeverityLevel = 'Any' + +`Any` was documented in `build.properties.ps1` on 0.8.x but was missing +from the parameter's `ValidateSet`, so setting it failed parameter +binding rather than doing what the documentation promised. It now works. +This is additive — existing values behave as before. + +Tracked in issue +[#144](https://github.com/psake/PowerShellBuild/issues/144). + ## Adding an entry (for PR contributors) Every breaking-change PR that lands in v1.0.0 must add an entry here for diff --git a/tests/Test-PSBuildScriptAnalysis.tests.ps1 b/tests/Test-PSBuildScriptAnalysis.tests.ps1 index 600a114..ee121c1 100644 --- a/tests/Test-PSBuildScriptAnalysis.tests.ps1 +++ b/tests/Test-PSBuildScriptAnalysis.tests.ps1 @@ -95,7 +95,7 @@ Describe 'Test-PSBuildScriptAnalysis' { $validateSet = $command.Parameters['SeverityThreshold'].Attributes.Where({ $_.TypeId.Name -eq 'ValidateSetAttribute' })[0] - ($validateSet.ValidValues | Sort-Object) -join ',' | Should -Be 'Error,Information,None,Warning' + ($validateSet.ValidValues | Sort-Object) -join ',' | Should -Be 'Any,Error,Information,None,Warning' } } @@ -249,6 +249,164 @@ Describe 'Test-PSBuildScriptAnalysis' { } } + Context 'ParseError severity' { + + # PSScriptAnalyzer's severity enum has four members; ParseError is reported for a file + # that does not parse at all. It was counted by none of the thresholds, so the strictest + # validated value (Information) still let an unparsable file through: the record was + # printed in the results table and the build passed. It is now counted with Error. + + It 'Fails at the threshold when a ParseError record is returned' -ForEach @( + @{ Threshold = 'Error' } + @{ Threshold = 'Warning' } + @{ Threshold = 'Information' } + @{ Threshold = 'Any' } + ) { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + [PSCustomObject]@{ + Severity = 'ParseError' + RuleName = 'FakeParseErrorRule' + ScriptName = 'Unparsable.ps1' + Message = 'A fake ParseError record' + } + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = $Threshold + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Throw + } + + It 'Still reports without failing at the None threshold' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + [PSCustomObject]@{ + Severity = 'ParseError' + RuleName = 'FakeParseErrorRule' + ScriptName = 'Unparsable.ps1' + Message = 'A fake ParseError record' + } + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'None' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Not -Throw + } + } + + Context 'Any threshold' { + + # 'Any' was documented in build.properties.ps1 but missing from the ValidateSet, so + # setting it failed parameter binding instead of doing what the documentation promised. + + It 'Fails on a finding' -ForEach @( + @{ FindingSeverity = 'Error' } + @{ FindingSeverity = 'Warning' } + @{ FindingSeverity = 'Information' } + ) { + $mockFindings = @( + [PSCustomObject]@{ + Severity = $FindingSeverity + RuleName = "Fake${FindingSeverity}Rule" + ScriptName = 'Fake.ps1' + Message = "A fake $FindingSeverity record" + } + ) + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $mockFindings + }.GetNewClosure() + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Any' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Throw + } + + It 'Passes when there are no findings at all' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Any' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters } | Should -Not -Throw + } + } + + Context 'Analyzer rule crash retry' { + + # PSScriptAnalyzer can crash a rule on an internal race unrelated to the code being + # analyzed (psake/PowerShellBuild#147). A re-run succeeds, so a RULE_ERROR is retried. + # A consumer with $ErrorActionPreference = 'Stop' would otherwise get a random red build. + + It 'Retries and succeeds when a rule crashes once' { + $script:analyzerAttempt = 0 + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $script:analyzerAttempt++ + if ($script:analyzerAttempt -eq 1) { + # Non-terminating, mirroring how the real cmdlet behaves under the + # -ErrorAction SilentlyContinue the function passes. The mock body would + # otherwise inherit $ErrorActionPreference = 'Stop' and throw on attempt one. + $ErrorActionPreference = 'SilentlyContinue' + Write-Error -Message 'Object reference not set to an instance of an object.' -ErrorId 'RULE_ERROR' + } + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Error' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters -WarningAction SilentlyContinue } | + Should -Not -Throw + + Should -Invoke -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -Times 2 -Exactly + } + + It 'Gives up after three attempts and surfaces the error' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $ErrorActionPreference = 'SilentlyContinue' + Write-Error -Message 'Object reference not set to an instance of an object.' -ErrorId 'RULE_ERROR' + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Error' + SettingsPath = $script:defaultSettingsPath + } + # A persistent failure must still reach the caller, so a consumer using + # ErrorActionPreference = 'Stop' behaves exactly as it did before the retry existed. + { Test-PSBuildScriptAnalysis @testParameters -WarningAction SilentlyContinue -ErrorAction Stop } | + Should -Throw + + Should -Invoke -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -Times 3 -Exactly + } + + It 'Does not retry an error that is not a rule crash' { + Mock -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -MockWith { + $ErrorActionPreference = 'SilentlyContinue' + Write-Error -Message 'Some other analyzer failure.' -ErrorId 'SOME_OTHER_ERROR' + } + + $testParameters = @{ + Path = $script:cleanPath + SeverityThreshold = 'Error' + SettingsPath = $script:defaultSettingsPath + } + { Test-PSBuildScriptAnalysis @testParameters -ErrorAction SilentlyContinue } | + Should -Not -Throw + + Should -Invoke -CommandName 'Invoke-ScriptAnalyzer' -ModuleName 'PowerShellBuild' -Times 1 -Exactly + } + } + Context 'End-to-end analysis' { It 'Fails a script with an Error-severity finding at the Error threshold' {