WIP: General code cleanup

[fourpastmidnight]

This PR just contains some general code cleanups:

1. Ensuring that the $SafeCommand version of Where-Object is used in files where in other instances, the $SafeCommands instance was being used
2. Ensuring that when checking for null, that $null is on the LHS of the equality check
3. Ensuring that aliases are not used (e.g. ForEach-Object instead of foreach when on the RHS of a pipe)

I'm sure there are more things that could be cleaned up, but these were pointed out by PSScriptAnalyzer in a project I'm working on where I've taken a private dependency on a customized version of Pester.

NOTE: I ran the tests locally, and I'm getting a lot of failures related to not being able to convert a breakpoint to a boolean. Not sure if my changes are causing this (I can't see how) or whether these are the result of recent changes due to the Assert-MockCalled issue work that's been done over the past few days. I'll update this PR description when I get more information, but posting this PR for now.

[fourpastmidnight]

Hmm, I ran the tests locally. In the case of the Travis CI build, I ran the exact same command line being run by the CI build in PowerShell Core 6.1 and they all passed. No errors. Same with the other tests--ran them in PowerShell v5.1 (which, given Pester is written with PSv2 compatibility, should run fine even on PS3 even when run in PSv5.x)--all passing. So, not sure why the CI builds are failing.

Honestly, I'm not seeing the problem here. These tests simply don't fail locally.

[fourpastmidnight]

Spoke too soon. I found the offending change--the change is in Coverage.ps1 in Exit-CoverageAnalysis. While cleaning up some issues spotted by PSScriptAnalyzer, specifically, comparison with $null where $null was on the RHS of the -ne operator. Null comparisons should be performed with $null on the LHS. Is there something special about the following line?

- $breakpoints = @($PesterState.CommandCoverage.Breakpoint) -ne $null
+ $breakpoints = $null -ne @($PesterState.CommandCoverage.Breakpoint)
  if ($breakpoints.Count -gt 0)
  {
      & $SafeCommands['Remove-PSBreakpoint'] -Breakpoint $breakpoints
  }

Looking at both lines of the diff, it would appear the order of operations should be:

1. Compare the array @($PesterState.CommandCoverage.Breakpoint) to $null
2. Store the result in $breakpoints. This implies that $breakpoints is a Boolean.
3. When executing the if condition, since $breakpoints is a Boolean, it doesn't have a Count property, so that should be, at worst, an InvalidOperationException or NullReferenceException, and at best, False, which is 0, and the if block would never execute.

But when I executed the original line in Powershell where $null is on the RHS of the -ne operator, $breakpoints is, in fact, a System.Object[]! Looks like I need to review Powershell's operator precedence rules?? Wow, that's really unexpected.

[fourpastmidnight]

And one more question about this:

When I run the commands in a PowerShell Console, I can see why the tests are failing. But, when I run the tests locally, then why are they passing?? I mean, if $breakpoints is truly a System.Boolean when $null is on the LHS of the -ne operator, then I would expect my tests to fail locally, just as they would when I run the commands in manually in the PowerShell console and just as they do when run in the CI build.

Strange.

[fourpastmidnight]

OK, well, I found this gem in the about_Comparison_Operators help:

When the input to an operator is a scalar value, comparison operators return a Boolean value. When the input is a collection of values, the comparison operators return any matching values. If there are no matches in a collection, comparison operators do not return anything.

Wow.

That seems like a poor design choice given that you expect a comparison operator to behave in only one way--performing a comparison. Not sometimes performing a comparison and sometimes filtering a collection (for which we have Where-Object)! Worse, the PSScriptAnalyzer rule is misleading (or, at least its description should be updated to suggest that if you're doing a "filter" comparison, that the RHS usage of $null with -(n)eq is OK). And even worse yet, in the above referenced help topic, there is no example showing what happens when a collection object is used on the LHS of such an operator. That line I quoted above is very obscure and very unlike what happens when using comparison operators in almost any computer language out there, especially OO languages like C# and their like.

Wow.

I guess I'll be reverting that change to Coverage.ps1 and all should be good.

[fourpastmidnight]

Still, why do the tests pass locally? Ugh, that really bothers me.

[nohwnd]

Still, why do the tests pass locally? Ugh, that really bothers me.

Are you using the same PowerShell version? Are you running in strict mode? I usually run bin\pester.bat and that gives me consistent results.

That operators filter collections is imho pretty well known gotcha, but I am not sure I would remember it when seeing that piece of code that you commented. Frankly I don't care much about fixing the code for the sake of making PSScriptAnalyzer happy, because it brings little benefit vs. easily introduces bugs, a lot of the code is written that way because of some optimization or edge case. Ideally those things would be identified and commented but as you can see it's not always the case.

[fourpastmidnight]

So, I do use pester.bat now to run the tests since I had all kinds of problems with PSv2 and #1142. I ran the tests with powershell.exe -Version 2.0 and with Powershell v5.1 and PowerShell 6.1.

All the tests passed locally. Anyway, it's a moot point, as I identified the offending code that I changed, and once again, the errors were caused by my lack of knowledge about "corner cases" in PowerShell which is very atypical of normal OO languages such as C#. This is not the fault of PowerShell--it's just that I'm an OO language developer, so I have to constantly remember to "question everything" when it comes to PowerShell, which is what lead me to look up info on comparison operators, which one would assume should be pretty straightforward. Yeah, that may be a known gotcha to those who've used PS for a while--but I've only heavily used it now for about 6 months and never came across this until now. 😉

Anyway, what are you trying to say in your comment? Are you saying you are not inclined to merge this PR at all? Or just that one commit? I'm not sure I understand what your position is on this PR. I'm fine whatever you decide; I just thought I'd clean up some of these things. (Although, now that I think about it, you must have suppressions somewhere in this project, or you wouldn't be able to host this on PowerShell gallery since they run PSScriptAnalyzer on uploaded modules!)

[nohwnd]

I ran the tests with powershell.exe -Version 2.0

That does not work the same way as PowerShell 2 sadly.

Yeah, that may be a known gotcha to those who've used PS for a while--but I've only heavily used it now for about 6 months and never came across this until now.

I was not trying to pick on you. I would not notice even I know the gotcha.

Anyway, what are you trying to say in your comment?

I will merge it after I review it, I see value in using the RHS LHS consistently, but replacing foreach with Foreach-Object just because PSScriptAnalyzer rule says it's the way to do it has no benefit for me, because it will work the same (I won't revert it now though). I don't think there are any suppressions, imho you can upload anything to PSGallery, it's just best practice, not a restriction.

[fourpastmidnight]

@nohwnd Ah, I see. And I didn't think you were trying to pick on me. 😄 I'm just frustrated that it's so hard to come to PowerShell from a typical OO language, such as C#, and have so many "weird" gotchas that, frankly, I haven't seen anywhere else (in bash scripting, batch files, etc.). Maybe they borrowed that from Perl? But I doubt it (but then again, I haven't used Perl in about 10 or 15 years and hardly want to even go looking!! 😉 ) In any event, thanks for the clarification on your position on this PR.

[nohwnd]

@fourpastmidnight is this ready to merge?

[fourpastmidnight]

@nohwnd: Yes, this is ready if all the tests are passing.

…

Sent from my Windows 10 phone From: Jakub Jareš Sent: Saturday, January 5, 2019 8:19 To: pester/Pester Cc: Craig E. Shea; Mention Subject: Re: [pester/Pester] General code cleanup (#1172) @fourpastmidnight is this ready to merge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[nohwnd]

It so happens, the calls to foreach which I updated were
not making use of $SafeCommands--and for the life of me, I still don't
fully understand when one should be using the $SafeCommand instance of
the command versus just outright calling the command directly.

This is because a lot of the code is running in places where user might have mocked the called command , so instead of calling the command directly and relying on the PowerShell to resolve it, we save a reference to it and invoke that instead. The SafeCommands hashtable is pretty ugly way to do this, an alternative is to create a module that is internal to pester that publishes the commands, I remember summarizing it in an issue when we discussed SafeCommands (cannot find the issue now), but ultimately Dave used safe commands because re-publishing the commands did not work for snap-ins, and some of the commands needed were snap-ins.