fix #1417 modulehelp false positives #1418
Conversation
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'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, |
|
Oh! I haven't tested on Linux, just pwsh on Windows. Could look into that later |
|
This failure started to happen in master on that specific image as well, therefore not something related to your change, so don't worry |
|
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. |
…ut\ module during testing, the built module will always appear 'installed'.
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 |
There was a problem hiding this comment.
$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 $msI could reproduce the error where it complains about workflows locally and just using the proposed line worked fine as well.
There was a problem hiding this comment.
yes, makes a lot of sense. will do that.
| @@ -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. | |||
There was a problem hiding this comment.
This change absolutely makes sense, I could reproduce the failure locally using a debug build. Thanks for taking the time to fix it.
There was a problem hiding this 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.
|
Could you have another look @bergmeister ? |
There was a problem hiding this comment.
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?
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:
PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.