elm-test icon indicating copy to clipboard operation
elm-test copied to clipboard

Add `Expect.any` to complement `Expect.all`

Open avh4 opened this issue 7 years ago • 13 comments

I had a need for the following function, which passes iff at least one expectation in the list passes. I think this should be added as Expect.any to parallel List.any/List.all.

expectAny : List (subject -> Expectation) -> subject -> Expectation
expectAny expectations subject =
    case expectations of
        [] ->
            Expect.fail "expectAny: all expectations failed (TODO: show failure reasons)"

        next :: rest ->
            case Test.Runner.getFailureReason (next subject) of
                Nothing ->
                    Expect.pass

                Just _ ->
                    expectAny rest subject

Before adding, this would need to refactor to have a private helper function that would build up the list of failure reasons to display if all the expectations failed.

avh4 avatar Jan 10 '18 20:01 avh4

How do we visualize n failure messages, though?

drathier avatar Jan 10 '18 21:01 drathier

I imagined the message would be something like what elm-html-test does:

Expect.any

  1) <first expectation failure reason>

  2) <second expectation failure reason>

  3) ...

Expected: any of the above to pass
X: all of the above failed

avh4 avatar Jan 10 '18 21:01 avh4

Each of those would have the expected and actual values as well, right? So that's a lot of text. I think I like the idea, but only if the execution is good. Let's see what other people think :)

drathier avatar Jan 10 '18 22:01 drathier

I think the problem with Expect.any is that you are making a weaker claim about your code than any of the subclauses alone. That is, you are saying "I don't know which of these cases will turn out to be true".

Your test should probably be split up more. Either write multiple tests with more specific fuzzers, or do case analysis on your result to see which expectation you think should apply.

mgold avatar Jan 11 '18 03:01 mgold

What are your use-cases @avh4? What are people using it for in elm-html-test?

drathier avatar Jan 11 '18 10:01 drathier

I agree that this is not commonly needed, but here's the test I have; what would you suggest?

                    html
                        |> dropzoneNode 0 "Evidence"
                        |> expectAny
                            [ expectCorrect "Correct Answer" "ANSWER-B"
                            , expectCorrect "Correct Answer" "ANSWER-C"
                            ]

I don't want to force the implementation to show one or the other, because both are correct in the context of the specification that I want to enforce. I don't want this test to break unnecessarily if someone changes the implementation.

avh4 avatar Jan 11 '18 17:01 avh4

I think it's pretty strange that you can have two correct answers but I will trust that it makes sense in a specific, isolated context.

Does that justify inclusion in the library, where we say "this is a common thing to do and frequently a good idea"? I don't think so. I think the fact that you are able to write expectAny with the exposed API means that the API is already sufficiently flexible to handle uncommon cases.

mgold avatar Jan 12 '18 05:01 mgold

Further arguments for including Expect.any:

  • all/any parallel &&/||; even though any would be infrequently used, it's still needed to provide a complete API
  • The example given for all is all [ greaterThan -2, lessThan 5], which is testing whether the subject number is in the range (-2, 5). any would be required to test whether a subject number is not in the range (-2, 5), so it's unclear why all is more necessary than any. Is it also not best practice to avoid using all whenever possible?
  • the implementation given above was easy to write, but a correct implementation that gives an appropriate error message is not easy to write, so there is benefit to having this in elm-test, even if it will be infrequently used
  • the implementation given above relies on Test.Runner.getFailureReason; the Test.Runner module is meant for use by test runners, so currently the Expect module does not provide an API that allows implementation of any without making arguably inappropriate use of the Test.Runner API.

avh4 avatar Jan 13 '18 01:01 avh4

Alright, I'm ... still reluctant, but not completely opposed. We should include useful and dangerous use cases of these functions in the docs.

mgold avatar Jan 13 '18 04:01 mgold

Okay, I can prepare a PR at some point, then.

As a final consideration of whether it's something to not add, maybe it would be useful to try to come up with an example of how any might be used to write a really bad test?

avh4 avatar Jan 14 '18 02:01 avh4

Ooh, that's an interesting exercise. Although for completeness you'd need to verify that it isn't just as easy to come up with a bad test using Expect.all.

My initial intuition is something that tries to have each expectation in a list correspond to one case of a union type, which should be discouraged in favor of handling each case explicitly. However, it turns out this is harder to write than I thought, thanks to the less-than-obvious type signature. If the obvious implementation was available, either in the library, based on expectAny/Expect.any as here, or built from the pass and fail primitives --

brokenExpectAny : List Expectation -> Expectation
brokenExpectAny expectations = expectAny (List.map always expectations) ()

-- then you could do something idiotic like this:

test "example" <| \_ ->
  brokenExpectAny
        [ 7 |> Expect.lessThan 5
        , 900 |> Expect.greaterThan 200
        , 4 |> Expect.atLeast 12
        ]

Now you're testing a bunch of unrelated claims. Gross.

Going back to the real implementation, I think I was most worried about something like

universityPerson |> Expect.any
  [ expectValidStudent
  , expectValidFaculty
  , expectValidStaff
  ]

because shouldn't you know more about universityPerson anyway? But the only way this code can compile is if

  1. you have a union type of the different representations of people, and expectValidStudent considers a faculty (even a valid one) to not be a valid student. I can actually see this sort of test being useful if you have something like "faculty and staff are allowed here, but students aren't".
  2. you have a single record representing multiple logically distinct classes of person, likely with a lot of nullable and interrelated fields, which is a bad idea anyway.

Does the university example strike anyone as a big red flag?

mgold avatar Jan 15 '18 01:01 mgold

I agree that any : List Expectation -> Expectation could facilitate writing worse tests, and I also agree that @avh4's proposed any : List (subject -> Expectation) -> subject -> Expectation does not have this problem. The university example doesn't seem like a concern to me.

I'm sold. I say we add it!

Does anyone have any objections to adding it?

rtfeldman avatar Feb 06 '18 23:02 rtfeldman

I'm all for.

drathier avatar Feb 07 '18 16:02 drathier