elm-community / elm-test

moved to elm-explorations/test
https://github.com/elm-explorations/test
BSD 3-Clause "New" or "Revised" License
339 stars 35 forks source link

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

Open avh4 opened 6 years ago

avh4 commented 6 years ago

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.

drathier commented 6 years ago

How do we visualize n failure messages, though?

avh4 commented 6 years ago

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
drathier commented 6 years ago

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 :)

mgold commented 6 years ago

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.

drathier commented 6 years ago

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

avh4 commented 6 years ago

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.

mgold commented 6 years ago

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.

avh4 commented 6 years ago

Further arguments for including Expect.any:

mgold commented 6 years ago

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

avh4 commented 6 years ago

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?

mgold commented 6 years ago

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?

rtfeldman commented 6 years ago

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?

drathier commented 6 years ago

I'm all for.