Only the result of the final expectation is used

[frankwallis]

I am seeing an issue when using multiple expectations in a single test. If the final expectation succeeds then the test passes, regardless of the outcome of the others. E.g.

    testOnly "sums numbers" (fun () => {
      expect (1 + 2) |> toEqual (3);
      expect (1 + 2) |> toEqual (4);
    });

correctly fails the test, but

    testOnly "sums numbers" (fun () => {
      expect (1 + 2) |> toEqual (4);
      expect (1 + 2) |> toEqual (3);
    });

incorrectly passes the test.

If I have time I will investigate further...

[glennsl]

Yes, this is intentional. expect ... |> toEqual ... won't actually do an assertion, but will return a value that is then picked up by test, which then runs the actual assertion. That way you can't accidentally forget to run an assertion and still have the test pass. And if you have more than one assertion you should get a warning about an unused value. It also forces tests to be simpler.

If you really need to do multiple assertions in a single test, you can assert on a tuple, e.g. expect (a, b) |> toEqual (4, 3).

I've been thinking over whether or not it should be possible to compose assertions somehow, but haven't come across a situation that really need it yet.

[frankwallis]

ah ok, that's what all those warnings were about... 😄

So these are the tests I am writing https://github.com/frankwallis/redouble/blob/reason/src/model/game/__tests__/boardTests.re#L45

I could split them into a describe with multiple tests but it would nice (I think) to be able to run them all under a single test. I'm not sure what it would look like though, maybe

expectMany [
   expect (1 + 2) |> toEqual (3),
   expect (1 + 2) |> toEqual (4)
]

expectMany (
   expect (1 + 2) |> toEqual (3),
   expect (1 + 2) |> toEqual (4)
)

expectMany
   |> expect (1 + 2) |> toEqual (3)
   |> expect (1 + 2) |> toEqual (4);

[glennsl]

Right, yeah this is a good example, where you have a fixed set of example inputs you want to test. IMO your tests should still be split into one for valid tests and one for invalid tests though, and I think I'd prefer a less general approach.

One for mapping a set of inputs to a single output, e.g.:

test "..." (fun () => {
  let board = Board.create Seat.East;
  let inputs = [...];
  expectAll inputs (fun input => Board.validateBid input board) |> toEqual None
});

And one for mapping a set of input and output pairs:

test "..." (fun () => {
  let board = Board.create Seat.East;
  let testCases = [(input, output), ...];
  expectForEach testCases (fun input output => Board.validateBid input board |> toEqual output)
});

How does that look to you?

[frankwallis]

That's a good point about splitting the test into valid and invalid inputs and testing them separately, I will do that. The examples above look good but I can't help feeling that it's taking away some flexibility, and it will be necessary to write lots of different expect combinators when it would be better if possible to use the existing ones of List.

The first one is quite nice as it lists outs the inputs expected to return None, and so it is easier to add test cases as they arise. The second one would work for my situation but you are always restricted to using a toEqual assertion, perhaps you might want to use different assertions for different inputs.

Another example test I have is here https://github.com/frankwallis/redouble/blob/reason/src/ui/table/__tests__/biddingHistoryTests.re#L29 and again arguably the test is a little woolly. But a general scenario might be:

test "displays buttons correctly in read-only mode" (fun () => {
    let wrapper = Enzyme.shallow <DetailsScreen readOnly=true />;

    let saveButtonLength = wrapper
      |> Enzyme.find "button.save-button"
      |> Enzyme.length;
    expect (cellLength) |> toBe 0;

    let closeCaption = wrapper
      |> Enzyme.find "button.cancel-button"
      |> Enzyme.text;
    expect (closeCaption) |> toBe "Close";
  });

A final point is that in JavaScript tests can return a promise to indicate to Jest when they have completed (see https://facebook.github.io/jest/docs/en/tutorial-async.html) is that functionality going to be lost now or do you think it is no longer needed?

UPDATE: ok I see now there are separate methods testPromise & testAsync for handling async tests which makes sense

[glennsl]

The lack of flexibility is intentional. Restricting what you can do makes it easier to understand what a test does, precisely because it's limited in what it can do. And it makes it more difficult to write tests that are too vague, which I think is the case with your last example above. You should only test a single property per test (that doesn't necessarily mean a single assert though, which is why expectAll and expectForEach are ok).

But there's certainly a trade-off, and the reason why I haven't added APIs like these yet is because I'm not totally sure exactly what the trade-off should be. It might also be that there's other list operations that would be useful, but I can't really think of any right now.

[frankwallis]

Ok I'm persuaded and I agree allowing multiple expectations is an invitation to write vague tests. I still think that the combinator should be as primitive as possible - if it was something like this:

expectAll [
   Board.validateBid NoBid board |> toEqual None,
   Board.validateBid Double board |> toEqual None
]

then that would also allow:

let inputs = [NoBid, Double];
expectAll (List.map  (fun input => Board.validateBid input board |> toEqual None))

and

let cases = [(NoBid, None) (Double, None)];
expectAll (List.map  (fun (input, output) => Board.validateBid input board |> toEqual output))

Which might be nice I think?

[frankwallis]

And

let expectForEach testCases assertion =>
   expectMany (List.map assertion testcases)

[glennsl]

It would also allow and even encourage lots of unwanted behavior, without providing any real benefit as far as I'm aware. Furthermore, this primitive can always be added later (or rather just exposed, probably), and the more specific functions can be implemented in terms of it and gradually phased out. It'd be a lot harder to go the other way.

[frankwallis]

The benefit is that people can write their own combinators using it, and the library doesn't have to predict every single use case up-front.

However it does add flexibility so if you think it's more important to lock everything down then maybe just go with the 2 you suggested.

[glennsl]

We don't have to predict everything up front. If we find there's a real benefit to adding it, we can easily just do so then. But we can't easily remove it once it's been added. Look at Scala if you want a good example of what happens when things are added "just in case".

I'll add the two APIs I proposed above then. Thanks for bringing this up, and for having a good discussion :)