Skip to content
This repository has been archived by the owner on Oct 9, 2019. It is now read-only.

Add Expect.any to complement Expect.all #228

Open
avh4 opened this issue Jan 10, 2018 · 13 comments
Open

Add Expect.any to complement Expect.all #228

avh4 opened this issue Jan 10, 2018 · 13 comments

Comments

@avh4
Copy link
Member

avh4 commented Jan 10, 2018

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
Copy link
Collaborator

How do we visualize n failure messages, though?

@avh4
Copy link
Member Author

avh4 commented Jan 10, 2018

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
Copy link
Collaborator

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
Copy link
Member

mgold commented Jan 11, 2018

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
Copy link
Collaborator

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

@avh4
Copy link
Member Author

avh4 commented Jan 11, 2018

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
Copy link
Member

mgold commented Jan 12, 2018

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
Copy link
Member Author

avh4 commented Jan 13, 2018

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.

@mgold
Copy link
Member

mgold commented Jan 13, 2018

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

@avh4
Copy link
Member Author

avh4 commented Jan 14, 2018

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
Copy link
Member

mgold commented Jan 15, 2018

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
Copy link
Member

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
Copy link
Collaborator

drathier commented Feb 7, 2018

I'm all for.

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

No branches or pull requests

4 participants