Allow paths to be pipelined to Invoke-ScriptAnalyzer #1040
Conversation
Merge changes of 1.17.1 (including 1.17.0) into Master
edyoung
changed the title
|
AppVeyor failure appears unrelated. |
| It "Can be piped from Get-ChildItem" { | ||
| $piped = ( Get-ChildItem -Path $directory -Filter TestTestPath*.ps1 | Invoke-ScriptAnalyzer) | ||
| $explicit = Invoke-ScriptAnalyzer -Path $directory\TestTestPath*.ps1 | ||
| $piped.Count -eq $explicit.Count | Should -BeTrue |
There was a problem hiding this comment.
This would result in a much better error message if the test fails:
$piped.Count | Should -Be $explicit.Count
| @@ -9,7 +9,7 @@ if (-not (Test-PSEditionCoreCLR)) | |||
| $null,"Wow6432Node" | ForEach-Object { | |||
| try | |||
| { | |||
| Set-ItemProperty -Name "DisablePromptToUpdateHelp" -Path "HKLM:\SOFTWARE\$($_)\Microsoft\PowerShell" -Value 1 -Force | |||
| Set-ItemProperty -Name "DisablePromptToUpdateHelp" -Path "HKLM:\SOFTWARE\$($_)\Microsoft\PowerShell" -Value 1 -Force -ErrorAction SilentlyContinue | |||
There was a problem hiding this comment.
Can you please elaborate on the error that you were getting that led you to apply this change? On which environment (platform and ps version) did you see it?
There was a problem hiding this comment.
running tests in non-elevated windows powershell on Windows 10. Despite the try/catch errors are shown when this fails to update HKLM key.
There was a problem hiding this comment.
Ok, thanks for the info. This does not happen on my machine, I am wondering if this could be caused by some group policy in your environment. Although it seems that this test code should reviewed again and possibly refactored, I think we can still accept your changes to it.
There was a problem hiding this comment.
Overall, the fix looks OK to me, thanks for improving PSSA. I pushed an empty commit to restart CI but you are right that this was probably a sporadic failure, the error message indicated though that the test failure was caused by a PSSA bug (I suspect it was one of the known multi-threading issues).
I left one minor comment for an improved test assertion and a question regarding the unrelated test improvement.
But the next time, please follow the GitHub flow model and create the branch in your fork.
| @@ -66,7 +66,7 @@ Describe "Test importing correct customized rules" { | |||
| $null,"Wow6432Node" | ForEach-Object { | |||
| try | |||
| { | |||
| Set-ItemProperty -Name "DisablePromptToUpdateHelp" -Path "HKLM:\SOFTWARE\$($_)\Microsoft\PowerShell" -Value 1 -Force | |||
| Set-ItemProperty -Name "DisablePromptToUpdateHelp" -Path "HKLM:\SOFTWARE\$($_)\Microsoft\PowerShell" -Value 1 -Force -EA SilentlyContinue | |||
There was a problem hiding this comment.
Minor nit: Use -ErrorAction SilentlyContinue for consistency, readability and to not rely on parameter aliases
There was a problem hiding this comment.
Looks ok, overall, only a minor nit. The first CI run that I re-triggered had a problem with Push-AppVeyorArtifact, I raised this issue with AppVeyor as it seems to be a (repeated) regression from their side: appveyor/ci#2183
The 2nd build hung and timed out after 3h, I therefore started it again. We have not seen such behaviour before, therefore it is probably due to environmental changes in AppVeyor....
|
Appveyor responded and said they fixed the issue mentioned above. Will close and re-open to trigger CI again |
PR Summary
Resolves issue #1039
Allow Get-ChildItem *.psm1 -Recurse -Exclude *.test.ps1 | Invoke-ScriptAnalyzer
Also fix some errors I was getting from test scripts. Unclear why others weren't seeing that.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready