Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How are tests involving enumerating types meant to be written? #29

Open
alx9r opened this issue Nov 20, 2017 · 10 comments
Open

How are tests involving enumerating types meant to be written? #29

alx9r opened this issue Nov 20, 2017 · 10 comments

Comments

@alx9r
Copy link

alx9r commented Nov 20, 2017

By "enumerating types" I mean any type for which [LanguagePrimitive]::GetEnumerator() exists or IsUnrolledByPipeline is true.

Consider the following tests:

Describe 'array' {
    It 'A1. succeeds' {
        1,2 |
            Assert-Type ([array])
    }
    It 'A2. succeeds' {
        ,@(1,2) |
            Assert-Type ([array])
    }
    It 'A3. fails' {
        1,2 |
            Assert-Type ([int])
    }
}

Describe 'not array, but [LanguagePrimitive]::GetEnumerator() exists' {
    It 'B1. fails' {
        [System.Collections.Queue](1,2) |
            Assert-Type ([System.Collections.Queue])
    }
    It 'B2. succeeds' {
        ,[System.Collections.Queue](1,2) |
            Assert-Type ([System.Collections.Queue])
    }
    It 'B3. fails' {
        [System.Collections.Queue](1,2) |
            Assert-Type ([int])
    }
}

Many of these results are surprising to me:

  • A1: Shouldn't this fail? If the behavior of Assert-Type were customary PowerShell, it would be checking the type of each element of the array. I get that users who haven't yet learned what | really does might be surprised if this test failed. But such users are going to experience a lot of pain until they learn that anyway. It seems to me that promoting tools that hide PowerShell's default pipeline behavior just prolongs that painful learning period.
  • A3,B3: Shouldn't these pass? Each of the elements are [int].
  • Why does neither B1 nor B3 pass? It seems like at least one or the other should pass.

It strikes me that there are two distinct intentions a user might have for such enumerating types:

  1. The user intends to test the enumerating object.
  2. The user intends to test the elements the enumerator emits.

And there are two corresponding things the assertions could do:

a. Test the enumerating object.
b. Test the elements the enumerator emits.

Customary PowerShell involving | would lead to (a) regardless of the user's intentions. It seems like that would be good behavior for all users for the following reasons:

  • Novice Users: Pipeline behavior is consistent whether writing tests or production code so there is less to learn.
  • Experienced Users: Pipeline behavior of Assert- is what you'd expect from any other customary advanced function.

It seems like (b) should only be done when | is involved when the user explicitly expresses their intentions as such. I see the following reasons for this explicitness requirement:

  • The language doesn't usually behave in a way that leads to (b), so such unusual behavior ought be accompanied by some explicit language.
  • Explicitly stating intention (2) is likely to make Assert more maintainable because intention (1) is clearly distinguished from intention (2).

I can imagine a few ways to implement Assert's commands in a manner that clearly couples (1) to (a) and (2) to (b). But I'm wondering if there is a different line of thinking I'm missing for the way Assert currently handles enumerating types. How are tests involving enumerating types meant to be written?

@nohwnd
Copy link
Owner

nohwnd commented Nov 21, 2017

The reasoning here is that people rarely care about the type of the collection they pass in, rather they care about the data, and I want to keep the assertions consistent and useful, even though there are some tradeoffs to be made. With Should we learned that for this assertion:

1, 2 | Assert-Equal 1

People favor seeing Expected 1 but got collection 1, 2., instead of Expected 1 but got 2.. To do that I need to collect the complete input and assert on it.

With types it is a bit more difficult, because there you are asserting on the internal details of how I collect the input. To get correct result you would need to circumvent the pipeline and do:

Assert-Type -Actual 1,2 -Expected ([Array])

Or is there a way to get the original collection after the pipeline without anything special being done by the user?

Another reason is that assertions over collections can have multiple requirements, sometimes you look for Any item to pass, sometimes you look for all items to pass, sometimes you want the items to be ordered but you don't really care about the data etc. That's why I am trying to make one set of assertions that will deal with "values" and another one that will deal with collections.

@alx9r
Copy link
Author

alx9r commented Nov 21, 2017

tl;dr I think that most new PowerShell users are surprised by how the pipeline works. The reason for that seems to be deficient courseware. Should already caters to those users. I thought Assert was for advanced users.


With Should we learned that for this assertion:
1, 2 | Assert-Equal 1
People favor seeing Expected 1 but got collection 1, 2.

It seems to me that what we learned from Should is that users unfamiliar with PowerShell's pipeline are surprised by PowerShell's pipeline behavior. I certainly was surprised when I was starting out. That surprise occurred in many varied cases aside from Should. In retrospect for me, it seems like this was caused by PowerShell training materials that were prepare by instructors who had a great deal of C# experience and very little PowerShell experience. C# does not have pipeline. PowerShell is not C#.

When I was starting out I expected

    1,2 | Assert-Equal 1

to fail. "Of course 1,2 isn't the same as 1," is what I thought. But that was because I had neither learned what | does nor how PowerShell's pipeline works. So I was basing my reading of most PowerShell statements on an incorrect understanding. PowerShell was extremely painful during that time. At one point I re-watched all 11 courses offered through my video training subscription thinking I had missed something. I hadn't. There was (and is to this day) no part of that learning path (or any other PowerShell learning path I have found) that provides even a remotely adequate primer of PowerShell's pipeline. Yet most PowerShell statements involve the pipeline in one form or another. After much language-introspective testing and many stackoverflow questions I eventually figured out how the pipeline works for myself. Now when I read

    1,2 | Assert-Equal 1

I expect it to pass for the first record, and fail for the second record. It's apparent that 1,2 is an array which is enumerable so the individual elements 1 and 2 are piped as separate records into Assert-Equal. Of course Assert-Equal could accumulate 1 and 2 but that would be weird given its name. If that's what it's doing, I would expect it to be called Assert-ElementsEqual or something more elaborate. The simplicity of the name Assert-Equal evokes simplicity of behavior.

I get that there is popular demand for a testing framework that works in the the way expected by people who haven't learned how PowerShell's pipeline works. I would have preferred that when I was starting out. Thankfully for me though, Should at the time behaved in the customary PowerShell way. So I was forced even by Pester to learn how the pipeline works. Since Pester 4.x Should introduced some unusual pipeline behavior. I'm fine with that. Pester is ubiquitous so Should should probably behave the way most people expect it to.

I suppose I interpreted the words "advanced" and "simplify" in "A set of advanced assertions for Pester to simplify how you write tests" to mean that Assert would embrace the PowerShell pipeline. It seems to me that building unusual pipeline behavior into assertion primitives like Assert-Equal is contrary to both "advanced" and "simplify."


I feel like we might be miscommunicating about the matter of collections and enumerations and assertions. Here's how I'm thinking about this:

  1. The pipeline doesn't care about whether an object is a collection. Rather, the pipeline cares about whether an object is a certain sort of enumerable to decide whether to unroll it.
  2. Some things that aren't really collections get unrolled (e.g. [XmlElement] enumerates nodes, [ConfigInstructions] enumerates steps that depend on the outcome of previous steps).
  3. Some things that are collections do not get unrolled (e.g. [string],[hashtable],[SortedList]).
  4. There needs to be assertions that can act on the contents of collections.
  5. Acting on the contents of collections is not the same as acting on the values that appear in the pipeline from the collection. This is because of (1),(2), and (3). It's often the same thing, but there are plenty of cases where it's not.

@nohwnd
Copy link
Owner

nohwnd commented Nov 21, 2017

I never meant this module to be for advanced users only, the opposite actually. The purpose is to simplify the assertions to behave consistently and predictably. Which is something the current Should assertions in Pester don't do.

I think you are reading too much in the collections and enumerators, most people are dealing with arrays and lists 90% of the time, and they expect it to show the whole collection in the assertion output even thought they pass it through pipeline. So the a collection in this sense is something that will be enumerated when passed through pipleline, or won't be wrapped when you do @(). If that works for every type in the universe is not that important, as long as it behaves uniformly and predictably for the most used types.

That people don't understand pipeline might be true, but there is not much I can do about it. And I still like writing 1,2,3 | Assert-Equal "a" better than Assert-Equal 1,2,3 "a".

I was looking at the code the whole day, and I can't really decide how to progress. I would like to merge some of the features with Pester, (especially the formatting), but I am not sure what it will break, and also not sure how to keep it modularized and still use the functions in Pester. :/

@alx9r
Copy link
Author

alx9r commented Nov 30, 2017

Or is there a way to get the original collection after the pipeline without anything special being done by the user?

Per this comment the answer seems to be "no". 😢

@alx9r
Copy link
Author

alx9r commented Feb 8, 2018

For posterity, pester/Pester#386 is one real-world example of the confusion caused by the pipeline's conditional enumeration while unit testing.

@nohwnd
Copy link
Owner

nohwnd commented Feb 10, 2018

@alx9r yeah you are right, now the question is what would be the best way forward. Would getting rid of pipeline completely help to eradicate those problems and make the syntax coherent and less surprising? At what cost?

@alx9r
Copy link
Author

alx9r commented Feb 10, 2018

...now the question is what would be the best way forward.

Since this debate came up I have been experimenting with an assertion library that assuages this issue (and a number of other pathologies). I expect I'll publish the library at some point. That might be a way forward.

Would getting rid of pipeline completely help to eradicate those problems...

AFAICT avoiding all kinds of unchecked conditional enumeration is critical to meaningful unit tests in PowerShell. It turns out there are a number of conditions where that occurs. | is the obvious case. Operators like -eq (see also pester/Pester#984) and the . member access operator (see PowerShell/PowerShell#5832) are others.

... and make the syntax coherent and less surprising?

Here's how I see the lay of the land:

  1. PowerShell's rich behavior means that a subtle difference often leads to a dramatically different (and sometimes surprising) result. Conditional enumeration is perhaps the most apparent such behavior, but there are many others.
  2. When writing unit tests this rich behavior needs to be taken into account and assumptions explicitly checked, otherwise false passes (eg. $obj.Misspelt | Should -beNullOrEmpty) and misleading test failure messages (eg. $arrayYouThoughtWasScalar.a | Should -be 1) abound.
  3. Because of (2) an assertion library that streamlines the task of explicitly checking common assumptions is necessary. (eg. automatic checks for the existence of the Misspelt and a members)
  4. Composition must be used to cover with manageable complexity the vast variations encountered in real-world tests. In other words, a practical assertion library described in (3) must support composition and the test writer must understand how to compose tests. (AFAICT this is how gtest and other assertion libraries achieve good coverage, good failure messages, and manageable complexity.)
  5. Reading test code written using an assertion library described in (4) is a significant improvement over equivalent tests written using Should. I expect this is true even for readers unfamiliar with the library. This is mainly because the verbosity of the equivalent tests written using Should explodes around the numerous explicit checks mentioned in (2).
  6. For authors who have learned to use the assertion library in (4), writing test code is significantly improved over writing the equivalent tests using Should. This is mainly because there is much less repetition than the equivalent tests written using Should.
  7. For authors who have not learned to use the assertion library in (4), writing test code is probably more productive using Should.

In other words, authors able to learn can benefit from an improved assertion library that leads to coherent test code with fewer surprises.

Authors unable to learn are probably best served by Pester's Should despite its attendant surprises. I see no practical way to improve Should to avert surprises. I also see no way to implement less surprising general-purpose assertions that are as easy to use for the uninitiated as Should.

FWIW, here is an example of test code written against my experimental library:

Describe Example {
    $arrayYouThoughWasScalar = @(
        [pscustomobject]@{a=1}
        [pscustomobject]@{a=1}
    )
    It 'Verbose style' {
        New-TestSubject $arrayYouThoughWasScalar |
            Select-InnerObject (Property a) {
                $_ | Assert-That -ItIs -EqualTo 1
            }
    }
    It 'Terse style' {
        subject $arrayYouThoughWasScalar |
            pick (prop a) {
                $_ | assert -eq 1
            }
    }
}

The output is as follows:

image

@nohwnd
Copy link
Owner

nohwnd commented Feb 11, 2018

This is a lot of very useful information. To me it seems that the most strict solution to avoid the problems would be to reject using the pipeline and differentiate between scalar and "enumerable" assertions. This way input parameters can be rejected (as I do in some cases already) so that:

  • for scalar assertion providing enumerable to Expected will throw argument exception suggesting you should use some of the enumerable assertions

  • for scalar assertion providing enumerable to Actual will fail the assertion, because enumerable cannot be equal to scalar

  • enumerable assertions will implement operations like All, Any etc. and provide a predicate. the predicate can either return $false or throw so we can reuse our assertions without providing another set of Test-* functions

One big annoyance is that negative numbers become strings when passed as parameters:

function a ($a) {
    $a.gettype().Name
}

a 1 # int
a -1 # string
a (-1)

One way to solve this is trying to cast the value to decimal, but then we should have an option to disable that casting. Or possibly a parameter set can be implemented that defines a well typed parameter (-ExpectedNumber ), or a -Type parameter that specifies what is the type of the input data. All of those are non-intuitive (and it is difficult to detect what the user meant and suggest a better solution - like using a specialized assertion).


The thing is that I don't want to get rid of the pipeline input because it is just so convenient. And without it I doubt anyone will ever use this libary, no matter how safe and awesome it is :)

So aditionally we could:

  • detect that the expected type in Assert-Type is an enumerable (by construcing an instance of it, and testing it by is-colleciton - there will be some conditionals for types without default ctor - like array, but it should help) and suggest that the pipeline input should not be used for that assertion

In your code example, you are doing pretty much what I envisioned this library to do, except the names are different, and I don't get what the subject does differently than if we just gave the value directly to Select-InnerObject via parameter.

@iRon7
Copy link

iRon7 commented Feb 12, 2018

Coming from: pester/Pester#386 (comment)
I agree that it is difficult to detect what the user meant if you only look to the input, but it gets more clear if you also look to what the user expects it Should -Be.
e.g.: if I test:
$Something | Should -BeOfType [System.Data.DataTable]
I apparently expect something to be a DataTable Type and if it is not, it would be valuable if I get a hint that I e.g. should add an unary comma operator in front of the input object.

In other words, I should (only) get a hint (to add unary comma operator) if the following conditions are true:

  • (Pester error, the result is false)
  • the -BeOfType is any of the types that gets automatically unrolled, e.g. [ArrayList],
    [DataTable], [XmlElement], [ConfigInstructions], etc. (which might be programmically recognizable somehow).
  • there is more then 1 object in the pipeline (?, this also means that I will -incorrectly- not get a hint on e.g. an empty DataTable or a DataTable with just one row. At the other hand, it could limit false hints)

The hint will then only be unnecessarily displayed if there are (multiple) objects in the pipeline tested against any of the specific "automatically unrolling" types:
In examples like below, I will probably get an unnecessarily hint:
$XmlElement | Should -BeOfType [System.Data.DataTable]
$Object1, $Object2 | Should -BeOfType [System.Data.DataTable] #(Where $Object1 -isNot [System.Data.DataTable])

@alx9r
Copy link
Author

alx9r commented Feb 12, 2018

The thing is that I don't want to get rid of the pipeline input because it is just so convenient.

Putting | between the test subject and the assertion means (categorically, it seems) that the assertion cannot know exactly what the original object was. With that as a starting point there will always be surprising results. I think you can probably mitigate some of the most common surprises using some of the tactics suggested above, but each such mitigation I experimented with created another different, perhaps less likely, usually more surprising, sometimes more pernicious, behavior. Usually this boils down to replacing one problem with another.

And without it I doubt anyone will ever use this libary, no matter how safe and awesome it is :)

It seems you and I have different priorities. My goal is to build an assertion library that is robust to PowerShell's rich behavior. Some of us desperately need such a library, and one currently does not exist. Of course I think it would be good if others can benefit from such a library, but I've decided not to compromise that goal in pursuit of popularity. I found that to be an easy decision because Should already strikes the compromise between the ease of use and robustness necessary to achieve popularity.

I don't get what the subject does differently than if we just gave the value directly to Select-InnerObject via parameter.

In practice it's unusual to use subject because the test subject is usually the results of invoking the command under test. A test fixture is necessary to gather results of invoking the command under test and the fixture's output is piped straight into assert or pick without ever involving subject. Without such a test fixture you end up repeating boilerplate in your test code to test for common things like the following:

  • Null versus nothing being output - in the context of a pipeline this is an important difference
  • Writes to the error stream - without this tests pass despite non-terminating errors occuring
  • Rethrowing exceptions thrown downstream - necessary for testing that meaningful exceptions are produced when the command is in a pipeline

This is just a small sampling; there are many other benefits to using a test fixture for command invocation.

Even when no command invocations are involved, the use of subject results from parameter binding tradeoffs. Once you account for all the ways that assert and Select-InnerObject ought to be composable, their parameters become a very crowded place. Moving the test subject to the pipeline reduces that crowding in a manner that has a hope of being intuitive. There are a lot of tradeoffs at play there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants