Refactor Script Analyzer to be used as a .NET library

[daviwil]

Hi everyone,

This is a refactoring of Script Analyzer to enable it to be used as a library in a .NET application. The bulk of the work was moving existing logic from the Invoke-ScriptAnalyzer cmdlet into the ScriptAnalyzer class. The means by which I achieved this move are not ideal; further refactoring will be needed to remove reliance on singleton instances of the ScriptAnalyzer and Helper classes.

When we spoke at the meeting on Friday, we had agreed that using a PSHostUserInterface instance would be a good way to factor out the usage of Cmdlet.WriteX calls and avoid Console.WriteLine. After further inspection, I decided to create an IOutputWriter interface because PSHostUserInterface doesn't provide methods like WriteError(ErrorRecord) and ThrowTerminatingError. Let me know whether you think this is the right approach.

Please let me know if there are any coding or style guidelines I might have missed. I have made sure that all of the existing Pester tests run after my changes.

Thanks!

[msftclas]

Hi @daviwil, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (David Wilson (WSSC)). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

[quoctruong]

Hi David, have you updated the pull requests so that all the tests will pass?

[daviwil]

Yep, I pushed an updated commit that fixed everything. The AppVeyor results show green for me on this page: https://ci.appveyor.com/project/PowerShell/psscriptanalyzer/build/1.0.249

[quoctruong]

Should be !excludeRegexMatch

[daviwil]

Yep, I saw your bug fix after I sent the pull request :) I'll fix that.

[daviwil]

Fixed.

[raghushantha]

David. Thanks for the changes.

Can you add Pester tests to validate the ScriptAnalyzer C# library?

[daviwil]

Sure, what kind of tests would you like to see? Here are a couple of ideas:

• Create ScriptAnalyzer instance, call AnalyzeFile with a simple script that should raise the AvoidGlobalVars rule, verify that rule was raised
• Create ScriptAnalyzer instance, use PowerShell parsing API to get a ScriptBlockAst from the same simple script, pass it into AnalyzeSyntaxTree method and verify that AvoidGlobalVars rule was raised

Anything else?

[raghushantha]

Sounds good.

One more E2E test:
Validate and compare results when using Invoke-ScriptAnalyzer vs C# library for:

• At least two kinds of severity (Error, Warning)
• At least two rules (one in PS namespace, one in PSDSC namespace[use wildcard here])
• when using recursively

[daviwil]

Quick progress update: I think I've found a way to reuse all of the existing Invoke-ScriptAnalyzer tests to exercise the same functionality against the ScriptAnalyzer class. I'll have an update to the pull request pushed out tonight.

[daviwil]

I've pushed an update to the pull request, but a couple of the tests will most likely fail. I'd like to get some feedback on the approach I'm trying here, which is to mock Invoke-ScriptAnalyzer cmdlet in the LibraryUsage.tests.ps1 file so that it uses ScriptAnalyzer with a separate runspace, mimicing how it would be used in a .NET application. This approach helped me find a couple of issues with my implementation, most notably the fact that I wasn't handling path recursion correctly.

There are a couple of problems that I'd like to get feedback on:

Problem 1: Using CommandInvocationIntrinsics from a Runspace fails in some cases

In the case where the ScriptAnalyzer class is given an existing Runspace to use, I'm pulling the CommandInvocationIntrinsics class from the Runspace's SessionStateProxy object. For some reason, calling GetCommand on this CommandInvocationIntrinsics instance is raising an InvalidOperationException when trying to get information about the "Write-Verbose" cmdlet. This failure causes the evaluation of the PSAvoidPositionalParameters to appear to complete successfully on analysis of the BadCmdlet.ps1 file. On my local machine, this problem causes the IncludeRule wildcard tests to fail.

This problem doesn't show up in the current AppVeyor results because it is dependent on the initialization of the test environment. If you run InvokeScriptAnalyzer.tests.ps1 before LibraryUsage.tests.ps1, the problem doesn't appear. However, if you run LibraryUsage.tests.ps1 first, the problem appears for both scripts. You may have to clone my branch and run the test locally to see the problem.

It's worth mentioning that GetCommand doesn't fail on all cmdlets, it works fine with "Export-ModuleMember" and "Import-Module." For some reason all the "Write-*" cmdlets and anything else in the Microsoft.PowerShell.Utility module cause it to throw an exception. Is it possble that I missed a step in setting up the test runspace?

Problem 2: Redirecting warning output isn't working correctly in the library usage tests

In InvokeScriptAnalyzer.tests.ps1, the very last test attempts to use a rule path that is invalid. The test verification attempts to redirect warning output to a string to make sure the right message is being written out in this case. For some reason, my mocked Invoke-ScriptAnalyzer cmdlet and PesterTestOutputWriter implementation write out the warning text in a way that doesn't get intercepted by this redirection. The verification check always ends up comparing a DiagnosticResult to a string. Any ideas why redirection wouldn't be working when calling PSHost.UI.WriteWarningLine() as opposed to PSCmdlet.WriteWarning() (which is used in the real Invoke-ScriptAnalyzer)?

Please let me know if you have any ideas how to solve these issues.

Thanks!

[raghushantha]

David. For issue #2, when using $host.ui.writewarningline(), this does not redirect the warning stream. But pscmdlet.Writewarningline() does. Hence the test fails.

I am not sure why the behaviour for console api is diff compared to cmdlet api. Will try to get an answer from the PowerShell team.

[daviwil]

Hi Raghu,

How would you recommend that I change the "Test CustomizedRulePath" \ "When used incorrectly" test to account for this? Should I check to see whether library tests are running and then use a different means to look for the warning message? I know of a way to do this, but I wasn't sure if you wanted me changing the InvokeScriptAnalyzer tests to have special cases for library testing 😄

Thanks!

[raghushantha]

Hi David. I think it is fine to add a separate condition for the library functionality. going forward we will end up adding library specific tests anyway. Agree with your approach.

[daviwil]

Great, thanks! I'll fix that up and push an update tonight.

Regarding issue #1, it's probably OK to move forward without worrying about it for now since it doesn't affect the usage of Script Analyzer as a module. When using it as a library, it just causes some rules to not be applied correctly. Since it's not an error-creating or fatal issue, I think that it's acceptable for the next couple of weeks until we figure out how to solve the issue. I'll help with getting that resolved once we get more information from the PowerShell team. What do you think?

[daviwil]

The warning test workaround solved the problem, AppVeyor results are green again 🎉

Before you sign off and merge back into your repo, would you like for me to squash the two additional commits back into the original for the sake of cleanliness? If not, that's fine too :)

[raghushantha]

Great!.
Issue#1 can be solved later. Pls commit your changes and I will merge the PR.

Thanks..

[raghushantha]

Changes look good.
I will merge the PR once Appveyor is clean.

[daviwil]

Looks like the AppVeyor run has passed. Thanks a lot for your help Raghu!

[raghushantha]

David. Thank you for adding the library support to ScriptAnalyzer. We must continue the partnership for future EditorServices features.

[daviwil]

We most definitely will :) I am excited to contribute to this great project!