Added -ReportSummary switch

[StingyJack]

After this discussion I tinkered with it some more and found I could not use the output variable methods as the would require multiline statements. Multiline statements require script blocks, and running a script block using powershell.exe from a non-powershell session does not work. My non-powershell session in this case is the Visual Studio External Tools tool.

This change adds a -ReportSummary switch to emit an additional single line count of error, warning, and info results that looks like this (if you had no errors, five warnings, and seven info messages).

Error : 0        Warning : 5     Information : 7

I needed this so i could tell if the script analysis had actually completed, was in progress, or had failed to execute in some way. Without it, the output is blank.

With the -ReportSummary switch , I know it ran...

[msftclas]

All CLA requirements met.

[bergmeister]

@StingyJack Thanks for the contribution first of all. However, after more thinking and discussing with others I am not convinced if it is right to write to the output stream because this will make it return an object. Would it work for you in Visual Studio to write to the host? I checked with the PowerShellEditor services guys here and this should not be a problem (although we would need to test it first). Because you defined it as a switch statement, it is not breaking at the moment but it would be nice to include this feature by default without breaking changes, which would be possible by writing to the host. I know, I was originally concerned about writing to the host but it seems to be the better option (and Pester does it as well).
If we continue with the -Switch parameter then in order to fix the failing CI tests due to the new parameter, you will need to update this file that creates a mock of Invoke-ScriptAnalyzer and the markdown file here. For an example of the required adaptions in those 2 files have a look at PR 817

[StingyJack]

Write-Host seemed appropriate to me, but I figured that the tool that complains about my write-host usage probably shouldn't be guilty of it.

Are those CI tests something I can run locally? VS didnt discover anything and there are only the two projects in the solution.

Edit: I also tried running pester but it fails on 63 tests if I just run it on the non-changed development branch. It then took another 20 minutes to realize pester probably has to be run from inside the Engine and the Rules folders, but still 46 failures not 3 or 6. Whats the secret?

[bergmeister]

Let's wait what James thinks about either writing to host by default without a switch statement (to keep the cmdlet simpler because it already has so many arguments) or writing to the output only when using the new switch. Thanks for the change in the meantime. I will try to test in the next days if this really works fine with other tools such as VSCode.

Therefore I would wait with fixing the CI tests in the meantime but I already pointed you to the files that you need to correct. You can run all tests locally via command line as follows:

.\buildCoreClr.ps1 -Framework net451 -Configuration Release -Build
.\buildCoreClr.ps1 -Framework net451 -Configuration PSV3Release -Build
.\buildCoreClr.ps1 -Framework netstandard1.6 -Configuration Release -Build
# install platyps for building docs if not already installed: install-module platyPS -RequiredVersion 0.5.0
.\build.ps1 -BuildDocs
# Copy pssa to one of the $env:PSModulePath folders and make sure to uninstall PSSA before. You need to open PS as admin for that!
new-item -ItemType Directory 'C:\Program Files\WindowsPowerShell\Modules\PSScriptAnalyzer/1.16.1'
copy-item .\out\PSScriptAnalyzer\* -Recurse -Destination 'C:\Program Files\WindowsPowerShell\Modules\PSScriptAnalyzer/1.16.1' -force
# tests are using pester 3.4
invoke-pester -Script .\Tests\Engine\
invoke-pester -Script .\Tests\Rules\

[bergmeister]

@StingyJack Just to keep you up to date. Today, I managed to get it working to integrate a custom build of PSSA into VSCode for testing purposes. I then tried this with your branch and it seems that the missing documentation of the new parameter is enough to break your version of PSSA (there is tight coupling with the documentation build and the help of the cmdlet). I then tried removing the switch and it did not work either so I am currently assuming that writing to the host by default breaks VSCode. Just give me a bit more time in the next days to experiment more please. In the next days, I'll cherry-pick your branch (GitHub does not allow me to have more than 1 fork) and use this then for further experiments. It is just easier to do it this way rather than having to always come back to you and asking for changes. Therefore please be patient for the moment whilst I continue testing and experiments. I will do my best to get this feature into the next (pre-)release (although we do not know a date for it yet). The good news is at least that the current state of the development branch has not broken the VSCode integration (which is good because there were substantial changes)

[StingyJack]

thats all fine, i have what i need locally and shared it with my team so time is not a pressure. i did see that the missing doc stubs cause breakage, and i am still curious as to why i get very different results from running the tests when compared to the ci.

i know what pester is for, but havent written a single pester test or attempted to run them outside of this exercise and that knowlegde gap (or the usual personal idiocy) is probably the difference.

[JamesWTruher]

@StingyJack, @bergmeister I'm working on a script to allow tests to be run on your local dev box which mimics what happens in CI. Stay tuned!

[bergmeister]

@StingyJack I proceeded with testing today and can now say that writing to the host by default does not hurt the VSCode integration and it does not even seem to show up in its output pane (which is good, because the PowerShell extension of VSCode has now even its own pane listing the PSSA violations). But I would like to let @JamesWTruher make the final call whether it is better to write to the host by default or introduce the new -ReportSummary switch. The advantage of enabling it by default it that every user wills benefit from it and since we already have a lot of parameters to the cmdlet, it does not make its usage more complicated. The obvious downside is that writing to the host is forced and we all know that we cannot pipe it away but given the use case, it is acceptable and Pester does the same without any problem. If we want to keep the switch, then @StingyJack would need to cherry-pick my commit here to fix the build problems.

[StingyJack]

Now I'm curious... why would anyone want to pipe away host output? Does that constitute "the big hate" toward Write-Host?

[StingyJack]

I'll take a look at that commit later. I think I know what you mean by cherry pick, I just don't use git that often.

[bergmeister]

@StingyJack because the commit is quite small you can also just copy paste the diff that GitHub shows you. But please wait with it as the question about the switch itself is not answered yet (your technical implementation is OK by the way).
@JamesWTruher Can you please approve if it is OK to write the summary to the host by default or only using this new switch.

[JamesWTruher]

@bergmeister @StingyJack this isn't passing CI yet - I think before this can be merged, we need to be sure that it has a clean CI run.

[StingyJack]

@JamesWTruher - I flailed for an hour or so trying to get tests to run and could get no better than 46 errors, which is different from the CI's reported failure count of 3, so I'm guessing that there is some other piece of sw required to run these tests that I dont have. Ideas welcome.

[bergmeister]

@JamesWTruher I already know what needs fixing in CI but before applying it we need to get an answer/approval on the high level design question because the fix would be different depending on what the answer is. Maybe it's easier if I open 2 new PRs with the fixes and you can decide then which PR you prefer?

[JamesWTruher]

@StingyJack - running the tests on your local dev box was pretty hard but should be better now with #882 merged. try a pull/rebase and let me know what you see. Be sure that you install platyPS before building and be sure to build the docs. I'm working on the build script so all of that happens automatically. It's my intention that running tests on the local dev box should be easy!