X Tutup
The Wayback Machine - https://web.archive.org/web/20200918095336/https://github.com/PowerShell/PSScriptAnalyzer/pull/1418
Skip to content

/ PSScriptAnalyzer

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #1417 modulehelp false positives #1418

Merged
merged 3 commits into from Feb 25, 2020
Merged

fix #1417 modulehelp false positives #1418

merged 3 commits into from Feb 25, 2020

Conversation

@Jawz84
Copy link
Contributor

Jawz84 commented Feb 17, 2020 •
edited

PR Summary

Fixes #1417

When looking up module version, if the module is not installed, also check if it is loaded and get module version from loaded module
Add 'AttachAndDebug' to paramBlackList, so it does not throw false positives for DEBUG builds.
Small bugfix for the actual blacklist check, the property 'name' of the $parameter variable needs to be used to check against the blacklist.

I have tested this by running:

# in pwsh.exe
.\PSScriptAnalyzer\build.ps1 -clean
.\PSScriptAnalyzer\build.ps1
.\PSScriptAnalyzer\build.ps1 -test
# no errors
# in powershell.exe
.\PSScriptAnalyzer\build.ps1 -clean
.\PSScriptAnalyzer\build.ps1
.\PSScriptAnalyzer\build.ps1 -test
# no errors

PR Checklist
Jos Koelewijn Jos Koelewijn
fix modulehelp false positives
Loading status checks…
b70e444 
When looking up module version, if the module is not installed, also check if it is loaded and get module version from loaded module
Add 'AttachAndDebug' to paramBlackList, so it does not throw false positives for DEBUG builds.
Small bugfix for the actual blacklist check, the property 'name' of the $parameter variable needs to be used to check against the blacklist.
@bergmeister
Copy link
Collaborator

bergmeister commented Feb 17, 2020 •
edited

I've re-run that failed job on Ubuntu 18.04 but it failed again with that 1 failure. I've re-run CI on master and had the same result. Sometimes it is upstream image changes that can cause that, therefore this 1 failures is not related to your change,
@Jawz84
Copy link
Contributor Author

Jawz84 commented Feb 17, 2020

Oh! I haven't tested on Linux, just pwsh on Windows. Could look into that later
@bergmeister
Copy link
Collaborator

bergmeister commented Feb 17, 2020

This failure started to happen in master on that specific image as well, therefore not something related to your change, so don't worry
@Jawz84
Copy link
Contributor Author

Jawz84 commented Feb 17, 2020

I just realized I made one unnecessary change. Because of the trick with temporarily setting $env:psmodulepath, the module will always appear 'installed'. I'll revert that part.
Jos Koelewijn Jos Koelewijn
undo check for loaded module, because $env:psmodulepath is set to .\o…
Loading status checks…
938696f 
…ut\ module during testing, the built module will always appear 'installed'.
@bergmeister
bergmeister reviewed Feb 17, 2020
View changes
Tests/Engine/ModuleHelp.Tests.ps1 Outdated
$commands = Get-Command -FullyQualifiedModule $ms -CommandType Cmdlet,Function,Workflow # Not alias
# Because on Windows Powershell we have workflow, we need to include it there, but in pwsh, we can't. This makes sure this works on both.
$commandTypes = ([System.Enum]::GetNames("System.Management.Automation.CommandTypes") -match "^Cmdlet$|^Function$|^Workflow$")
$commands = Get-Command -FullyQualifiedModule $ms -CommandType $commandTypes

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Feb 17, 2020 •
edited
Collaborator

$ms is just PSScriptAnalyzer in this case if I read correctly and PSSA only exports 3 cmdlets, no function/alias/workflow at all, therefore why not simplify it to just:

$commands = Get-Command -FullyQualifiedModule $ms

I could reproduce the error where it complains about workflows locally and just using the proposed line worked fine as well.

This comment has been minimized.

Sign in to view
@Jawz84

Jawz84 Feb 20, 2020 Author Contributor

yes, makes a lot of sense. will do that.
@bergmeister
bergmeister reviewed Feb 17, 2020
View changes
Tests/Engine/ModuleHelp.Tests.ps1
@@ -214,15 +214,19 @@ Get-Module $ModuleName | Remove-Module
Import-Module $ModuleName -RequiredVersion $RequiredVersion -ErrorAction Stop
$ms = $null
$commands =$null
$paramBlackList = @()
$paramBlackList = @(
'AttachAndDebug' # Reason: When building with DEGUG configuration, an additional parameter 'AttachAndDebug' will be added to Invoke-ScriptAnalyzer and Invoke-Formatter, but there is no Help for those, as they are not intended for production usage.

This comment has been minimized.

Sign in to view
@bergmeister

bergmeister Feb 17, 2020 Collaborator

This change absolutely makes sense, I could reproduce the failure locally using a debug build. Thanks for taking the time to fix it.
@bergmeister
bergmeister approved these changes Feb 17, 2020
View changes
Copy link
Collaborator

bergmeister left a comment

Thanks for taking the time. Good improvements! It's good to have everything work 'out of the box', no matter in which configuration it was built.
Just one small suggestion to minimize/simplify one change but otherwise looks good.

On an unrelated note: I saw your initial change to some of the Get-Module version logic and actually think most of it is not necessary and not even right in all cases and should rather be replaced by something like (Get-Command Invoke-ScriptAnalyzer).Module.Version because only this was will it load the right module into memory and return the correct version. But let's defer that to another PR.
Jos Koelewijn Jos Koelewijn
simplify command discovery
Loading status checks…
58fb6c6
@Jawz84
Copy link
Contributor Author

Jawz84 commented Feb 20, 2020

Could you have another look @bergmeister ?
@bergmeister
bergmeister approved these changes Feb 21, 2020
View changes
Copy link
Collaborator

bergmeister left a comment •
edited

Awesome, thanks, fully happy now :-)
@rjmholt There is new test failure in master (which is the one we see here) and it happens only on Ubuntu18 and it is another one of your compatibility tests, shall we disable it on Linux as well since we know that those sporadic failures in your compatibility tests come and go depending on image Ubuntu image updates?
@bergmeister bergmeister requested a review from rjmholt Feb 21, 2020
@bergmeister bergmeister merged commit 46e89de into PowerShell:master Feb 25, 2020
9 of 12 checks passed
9 of 12 checks passed
PSScriptAnalyzer-CI Build #20200220.3 failed
Details
PSScriptAnalyzer-CI (Test Ubuntu_18_04) Test Ubuntu_18_04 failed
Details
PSScriptAnalyzer-CI (Test macOS_10_15_Catalina) Test macOS_10_15_Catalina was canceled
Details
PSScriptAnalyzer-CI (Build Full_Build) Build Full_Build succeeded
Details
PSScriptAnalyzer-CI (Test Ubuntu_16_04) Test Ubuntu_16_04 succeeded
Details
PSScriptAnalyzer-CI (Test Windows_Server2016_PowerShell_5_1) Test Windows_Server2016_PowerShell_5_1 succeeded
Details
PSScriptAnalyzer-CI (Test Windows_Server2016_PowerShell_Core) Test Windows_Server2016_PowerShell_Core succeeded
Details
PSScriptAnalyzer-CI (Test Windows_Server2019_PowerShell_5_1) Test Windows_Server2019_PowerShell_5_1 succeeded
Details
PSScriptAnalyzer-CI (Test Windows_Server2019_PowerShell_Core) Test Windows_Server2019_PowerShell_Core succeeded
Details
PSScriptAnalyzer-CI (Test macOS_10_14_Mojave) Test macOS_10_14_Mojave succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bergmeister bergmeister

@rjmholt rjmholt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

ModuleHelp.Tests.ps1 throws false positives
2 participants
@Jawz84 @bergmeister
You can’t perform that action at this time.
X Tutup