Should Contains for evaluating arrays

[tribou]

It would be nice if the following were possible:

$array | Should Contain $item

Instead of:

$array -contains $item | Should Be $true

The former would help if it responded with a more specific failure message for better troubleshooting. (Instead of the latter's "Expected: {True} but was {False}")

[dlidstrom]

Yep, I'd like this too.

[nohwnd]

It is a bit unfortunate that the Should Contain assertion performs a lookup of a string in a file when there is -containsoperator in PowerShell. Overriding the established behavior with something completely different is not very intuitive so frankly I am not sure how to proceed with this. Can you think of other name that would suit the assertion?

[dlidstrom]

Perhaps?

$array | Should Include $item

[nohwnd]

Good idea.

[dlwyatt]

With the way Should currently works, you'd also have to write your test like this:

,$array | Should Include $item

Note the unary comma before $array. This prevents PowerShell from enumerating your array and sending the elements to Should to each be tested individually.

[ghostsquad]

Contain(s) should do either substring lookup if the input is a string, OR collection enumeration if the input is a collection. After all, a string is a collection of characters. Why would that not make sense? That's how it's done with Fluent Assertions (http://bit.ly/1lccNNU & http://bit.ly/1lccR08) as well as XUnit asserts (http://bit.ly/1lccS4f & http://bit.ly/1lccSRw).

[dlwyatt]

There are a few problems at the moment. First, the Should command receives pipeline input one element at a time, and calls the assertion function for each element. Unless you do something like ,@($someData) | Should Be @(1,2,3) (note the unary comma at the beginning of the command, to produce an array containing another array), the PesterBe function is not going to be seeing the entire $someData array at a time. I tried to get the Should command to work without requiring this unary comma, but it's quite a pain to produce reasonable behavior in all cases (mainly edge cases involving empty arrays and $null), and I backed out that pull request for the moment.

The second problem is that these assertion functions haven't been written with input collections in mind. That's easy enough to update.

And finally, Should Contain already exists with a completely different purpose. Whether that was a good decision or not, the fact remains that we'd be making a breaking change to have Should Contain do a different job now. I've been trying to keep breaking changes to an absolute minimum in the v3 code.

[Jaykul]

I know I'm in the minority here, having so few tests that I don't mind breaking things ... and forgive me for a slightly tangential post ...

I really think that in a PowerShell testing framework, the "Should" stuff ought to prioritize matching PowerShell's operators over matching the semantics from other testing frameworks -- I think there are far more people going to come to Pester from PowerShell because they have PowerShell code to test than from other frameworks.

Should Contain ... ought to be PowerShell's -contains operator. Should Match ... ought to be PowerShell's -match operator. Should Be ... ought to be PowerShell's -is operator. Should Equal ... ought to be PowerShell's -eq operator. etc.

I would love to see file contents as an overload where if the pipeline object is a FileInfo, then Contain and Match and BeLike and Equals and ... all the rest of them ... do a file contents search.

And I think I'd even like an -All switch so I could write things like this (and I don't care if it needs "-" or not, really, although I'm not convinced that not having the "-" helps anything when you're reading a test that's in PowerShell anyway).

1,2,3 | Should -All -BeLessThan 4  # Passes    
1,2,3 | Should BeLessThan 2  # Passes
1,2,3 | Should All BeLessThan 2  # Fails
1,2,3 | Should -BeGreaterThan 2  # Passes
1,2,3 | Should -All -BeGreaterThan 2  # Fails

I was playing with a parallel "Must" command that behaves in a more PowerShell way for existing PowerShell operators, but with support to fall back to the Should functions when there's not an operator (for extensibility),

However, because of backwards compatibility, and the confusion of having two ways of doing things, it almost feels like a thing where I need a usability study to prove it's better, and even then, a poll to decide if it's worth it -- or maybe just some better options about how to do it without screwing things up.

1. Is it OK to just have two systems side-by-side? E.g. Must and Should.
2. Would it be OK to break backwards compatibility? This seems like a bad plan, for a testing framework.
3. Can we deprecate things? E.g. Can we hide Should in favor of Must but still provide an easy way (in configuration) to turn on Should (and even hide Must), for people with existing code.

4. Should I expose the "Must" functionality as an extension method instead to keep them separate?

   $result.MustEqual( 4 )

An interesting side effect of doing it as an extension method is that you don't have to worry about the difference between $array | must and ,$array | must ...

[dlwyatt]

The only reservation I have about an extension method approach is null reference exceptions... $someVariable.Should.Be($null) seems valid from a test point of view, but won't actually execute if $someVariable is $null as far as I know. Other than that, it's superior to the pipeline syntax in every way, particularly around these headaches with collections.

Another option would be to make a function that reads well, but doesn't involve the pipeline. Something along the lines of Assert $someVariable -eq $null, which is easy to extend any way we like (and the extensions can actually plug in to both Should and Assert at the same time), and has no problems handling $null or collections. I'd be in favor of deprecating the Should command for something like that, but I would be in no great hurry to actually remove the Should command; I have no idea how long it would take people to update all of their tests.

[dlwyatt]

We could also allow the user to pass in a script block: Assert { $someVariable -eq $null }. This gives complex expression support, etc, but would give up on the user friendly failure messages that come from the operators we implement directly.

[ghostsquad]

$someVariable.Should().Be($null) probably won't work in powershell, because I believe that Update-TypeData requires an actual object in order to recognize, at runtime, what methods should be "added". Though, in strictly C#, an extension method is just syntactic sugar for a static method.

In fact, this is how you write an extension method:

public static class ObjectExtensions {
    public static Should(this object someVariable) { ... }

I was actually thinking of porting FluentAssertions into a usable PowerShell module. https://github.com/dennisdoomen/fluentassertions

$null will probably need to be dealt with in a different way like this:

Assert $myObject IsNotNull

Where "IsNotNull" could be part of an ENUM for the purposes of a tab completion, and evaluated internally.

[nohwnd]

I agree that the names of some of the assertions should change. Contains really is confusing, but changing the meaning of Be is a huge change, so I'd prefer if we just upgraded it to do type checks if Type is provided on the right side.

I like the fluent assertions, but we already deprecated this way once in favor of more PowerShell like code.

I would also like ale the assertions names and options became parameters. That is Should -Be 10, but only because it enables intellisense support, so we should investigate if all the current an possible future combinations can be expressed via Parameter Sets. Otherwise this change has no point.

Personally I think I need to get more familiar with rspec and cucumber before having a strong opinion on this.

@Jaykul 1., 4. I think having two systems would be bit confusing, and is not a good idea in the long run. 2., 3. Breaking backwards compatibility is OK as long as the change is relatively small and is released with a major version. If the change is big, like removing support for the fluent api it should be deprecated in the previous major version.

But I will look at your Must command, I am sure you have some great ideas there.

[nohwnd]

Thinking about it we are not sure who uses the framework and how :(

[dlwyatt]

Well, Pester's getting a lot of attention lately, and I'd say that if we want to make some improvements to our built-in assertions, now's the time. The impact is only going to get worse as time goes on. There are two current problems we'd like to address:

1. Using the pipeline input to Should (and its current implementation of calling an assertion method once for each input object) is annoying. It doesn't work with collections at all the way you think it should.
2. Should Contain is confusing when PowerShell has a -contains operator that means something completely different. This is easily changed without necessarily dealing with 1.

I started working on a branch some time ago to improve the current Should command so that the assertion method is only called once per call to Should, having it process collections and such, but it was ugly and still had trouble distinguishing between $null and an empty collection, and I ultimately withdrew the pull request to think about it some more.

Since the PowerShell extension method syntax doesn't work with $null values, I'm leaning toward the syntax that starts the statement with a command name instead of piping input (such as Assert $something -eq $null, though the command name doesn't necessarily have to be Assert.) This solves all of the headaches with $null or collection values, and gives us a clean slate to change the operator names without having an immediate breaking change for scripts that still use the existing Should command.

[Jaykul]

I like the idea of using Assert, it's classic :-) and in my original PSAINT project all I had in this regard was an Assert-That wrapper around ForEach ...

However, I think we have to be careful in fixing one problem to not create another. When I start writing tests that I wish would work on arrays, it gets complicated to explain what I want. Especially the difference between testing a property of an object, or enumerating a collection to test the property. Especially in PS4 with property unrolling...

$AResult = @(1,2,3)
$SResult = "Hello World"
$MResult = @(1,2),@(1,3,5),@(2,4,6)

# Should these both work?
Assert-That $AResult -Contains 2
Assert-That $SResult -Contains "World"
# Is it ok if you have to write this instead, for strings?
Assert-That $SResult -Match "World"

# How do I say that ALL the results should be numbers less than 4?
# And how do I know which failed if they aren't?
Assert-That -All $AResult -is [int]
Assert-That -All $AResult -lt 4

# How about "at least one of these" must be greater than 2?
Assert-That -Any $AResult -gt 2

# Does that mean this is the same as the Contains 2 test?
Assert-That -Any $AResult -eq 2

# How do I say there should be at least 3 results?
Assert-That $MResult.Length -ge 3
# How do I assert a property on each result, can I just switch to pipeline mode?
$MResult | Assert-That -All Length -ge 2
# Because this seems hard to figure out
Assert-That { $MResult | Where Length -lt 2 } -IsEmpty

Assert-That @() -IsNullOrEmpty
Assert-That $null -IsNullOrEmpty
# Does this fail because you didn't say -Any or -All ?
Assert-That @($null) -IsNullOrEmpty

[nohwnd]

"...we have to be careful in fixing one problem to not create another." Exactly!

[ghostsquad]

Maybe something like this:

$stringResult = "hello world"
$arrayResult = @(1,2,3)
Assert-That $stringResult -is "hello world" 
#true
Assert-That $stringResult -Matches "hello" 
#true
Assert-That $stringResult -DoesNotMatch "world" 
#false

Assert-ThatCollection $arrayResult -Contains 1 
#true
Assert-That $arrayResult[0] -ShouldBe 1 
#true
Assert-ThatCollection $arrayResult -HasCount 1 
#false

# this is experimental syntax, I'm not sure how it would work, it would probably 
# need to be parsed and converted to a delegate
Assert-ThatCollection $arrayResult -Any {$a => $a -as [int]} 
#true
Assert-ThatCollection $arrayResult -Any {$_ -gt 4} 
#false

# using delegates is easy to implement and understand
Assert-ThatCollection $arrayResult -Any {param($item) $item -gt 4} 
#false

#Using the neato FluentAssertions syntax of also allowing a "because" statement
Assert-ThatCollection $arrayResult `
    -Any {param($item) $item -gt 4} `
    -Because "at least one item should be larger than 5"

[dlwyatt]

$AResult = @(1,2,3)
$SResult = "Hello World"
$MResult = @(1,2),@(1,3,5),@(2,4,6)

# Should these both work?
Assert-That $AResult -Contains 2
Assert-That $SResult -Contains "World"

# Is it ok if you have to write this instead, for strings?
Assert-That $SResult -Match "World"

This seems fine to me. I thought the idea was that the Pester assertion operators should mirror what the equivalent PowerShell operator does, and PowerShell's -contains operator only works for arrays, not String.Contains().

As for more complicated comparisons (properties on objects, etc), we can potentially just say that Pester doesn't provide that functionality (though you can construct it with loops and comparisons of the primitive members yourself), instead of trying to come up with a syntax that works for everything. Or, maybe we can come up with something similar to IComparer, giving the user the option to pass in a script block that's responsible for comparing two elements of a collection:

$array1 = @(
    [System.Drawing.Point] @{
        X = 1
        Y = 2
    }

    [System.Drawing.Point] @{
        X = 3
        Y = 4
    }
)

$array2 = $array1.Clone()

Assert $array1 -eq $array2 -Comparer { $args[0].X -eq $args[1].X -and $args[0].Y -eq $args[1].Y }

-eq might not be a good choice here if we're supposed to be emulating the behavior of PowerShell comparison operators, which are weird about collections, but you get the idea. It might also make more sense to have separate commands for comparing scalars and arrays, to avoid confusion.

[Jaykul]

We've already talked about the idea of a scriptblock -- I don't think we need to complicate it any further than that, if you need custom comparison logic, you can write anything in a scriptblock.

I definitely think that I would want this thing to "magically" deal with anything that Where-Object can deal with (and one extra thing, which is to allow you not to specify the "Property" parameter and compare to the actual object).

I also think we should enumerate the testing functions (the "Should*" stuff) and generate dynamic parameters and parameter sets (we can copy this from OneGet, I think).

So I have two more questions (I think):

First: do we want it to handle pipeline the way Where-Object does?

1. Handle pipeline. If you want to test the array, you pass it to the InputObject parameter (we can call it "Actual" and call the Value parameter "Expected"), but if you want to test the items in it, you pipe them in.
2. No pipeline, always test the object and have a wrapper for Assert-That called Assert-ThatArray with mandatory -All or -Any which enumerates it's input to Assert-That ...
3. Handle pipeline, but write the wrapper anyway.

Note that if we do not accept pipeline input, then the command can take the input object positionally, so you can write:

Assert-That $AResult -Contains 2
Assert-That $AResult.Length -Eq 3
Assert-ThatArray $FResult Extension -eq ".xml"

But if we accept pipeline input, then the input object has to be specified when you're not piping:

Assert-That -Actual $AResult -Contains 2
Assert-That -Actual $AResult Length -Eq 3
$FResult | Assert-That Extension -eq ".xml"
# We can always get fully explicit:
Assert-ThatArray -Actual $FResult -Property Extension -eq  -Expected ".xml"

[Jaykul]

Oh, I also think we should have an error message parameter as @ghostsquad suggested. If nothing else it should be an optional way to override the failure messages from the should function.

The last question that I can think of for now is about how we deal with the output. In the should function, test cases return boolean and the should wrapper turns it into an exception ... is that how it has to work?

I wrote a first draft of "Assert-That" aka "Must" with a few tests (it needs a lot more), but right now it just outputs booleans ... and it only supports the PowerShell operators. It was meant as a proof of concept, but took too much effort :-P

[ghostsquad]

@Jaykul - Not everything is an array. Lists, Arrays, Dictionaries, IEnumerable are all collections. So, Assert-ThatCollection should the lingo to avoid confusion.

Can you clarify your second statement/question? Why would a should function do anything other than throw an exception if the predicate is false?

[Jaykul]

@ghostsquad Two reasons:

First of all, these test cases inherently return $true or $false: $Actual.Length -eq 3 doesn't throw an exception, it returns a boolean.

Additionally, this is PowerShell. When things are wrong, we like to write errors without terminating. If we do that, then we can see all the assertions that fail, instead of just the first one.

In the test framework I wrote previously, I consider a test a pass only when there are no errors AND (the test outputs nothing OR the test outputs just $true OR the test returns an array with the first item $true, and no $false items).

[ghostsquad]

whats wrong with this?

try {
   [Void]$testCase.Run()
catch {
   $testCase.Failed = $true
}

the problem with the the return value from $Actual.Length -eq 3 that Powershell allows through here should be ignored by the framework (as seen above with [Void]). I would consider this is a typo on the user's part.

We shouldn't force users to use the asserts that come bundled with Pester. And if you look at any assert from any other framework, they throw exceptions. An exception is the only indicator of failure, and that is expected behavior.

This should pass because I haven't asserted anything. To see this fail would be confusing.

It "should pass" {
    $true -eq $false
}

This should fail because an exception was thrown and caught by the framework.

It "fails" {
    throw (new-object system.notimplementedexception)
}

Also, remember, Powershell is a wrapper for .NET to be used in a console environment, and I would be surprised if I heard that there are a lot of PowerShell developers that aren't also .NET developers. Principle of Least Surprise comes in to play somewhere here...

[dlwyatt]

We're getting a bit off topic here. The It command already has the semantics of treating a test case as failed if any terminating exceptions are thrown, and success otherwise. (It also suppresses the test block's pipeline output.)

The assertion function's job is to throw exceptions if the expected conditions are not true. In some fashion (whether as a single script block, or by choosing another parameter set) we pass it some sort of predicate. If that predicate returns false, the assertion function throws an exception, which will cause the test to fail.

Users already don't have to leverage our assertion commands if they don't want to; they can just use the throw statement inside an It block to cause a test to fail.

Let's just lay out the use cases that we want the assertion function to cover, and then decide which API meets them cleanly.

• We want to be able to compare scalars to scalars, collections to collections, and scalars to collections (for -contains / -in behavior, at a minimum.)
• When dealing with collections, we may want to express some sort of All or Any concept.
• For each of those comparisons, we may want to support both default PowerShell comparison operator semantics, as well as forced reference equality checks and custom comparisons (checking that certain properties of objects are equal, etc.)
• We may want to allow the user to specify their own failure message.

Sound about right so far? Did I miss anything?

[Jaykul]

To wake up this thread ... that sounds about right.

[pcgeek86]

+1 to OP