Push users to .thenAnswer(<Future|Stream>)?

[matanlurey]

This is what I see a lot:

var mockThing = new MockThing();
when(mock.isDone).thenReturn(new Future.value(true));

// ...
runTest(thing: mockThing);

But it can break in basically un-debuggable ways, especially around zones. Normally a Future or Stream won't be created until an invocation is made, but with the .thenReturn pattern, it has been created in the test setup.

The "correct" behavior here would be to use .thenAnswer:

var mockThing = new MockThing();
when(mock.isDone).thenAnswer((_) => new Future.value(true));

I don't know if this a documentation, API, or just UX issue, or there is even a real issue - maybe mockito alone is mysterious so nobody assumes code to work great 100% of the time. We could make the API more verbose, i.e.:

thenAnswerSync<T>(T value);
thenAnswerAsync<T>(T Function() run);

But I don't know if that would help unless we threw on an invalid type at runtime.

[TedSander]

I definitely think there is a couple of problems here:
a) Documentation thenReturn is used 18 times in the README thenAnswer is used once. There is no documentation on how to use either.
b) Naming the two aren't really clear in the name alone what the difference is, and I think return is the more natural name.
c) I do think the dart language makes thenReturn easier to use then thenAnswer, but people don't understand the the object is getting created right away.

A lot of times using thenReturn is safe since when an object is created isn't that important. Async types are a very painful exception.

We should definitely fix a, and b.

What if we did an assert in thenReturn and threw if the type was a Future or Stream? If we needed to we could have a boolean optional variable of 'acceptAsyncTypes' of course I'm not sure if there is ever a case we should allow them.

[alanrussian]

+1. A lot of people on my team have ran into this issue.

[matanlurey]

I'm definitely motivated to fix this @alanrussian @TedSander.

That being said, I'm low on cycles. I'd be happy to review code/strategies from @alanrussian and his team on moving forward. I think this works for me:

What if we did an assert in thenReturn and threw if the type was a Future or Stream?

[alanrussian]

I will proceed with the assertion idea. I'll try to work on this soon and send a pull request.

[dskloetg]

The fix in commit #88 does not guarantee that the future is created inside the callback, nor does it even suggest that it should.
If people simply follow the instructions in the assertion message, it is likely not to make any difference.

"Invalid argument(s): thenReturn should not be used to return a Future. Instead, use thenAnswer((_) => future)."

Maybe the assertion message should explain why?

[mattalbr]

+1 lost 2-3 hours debugging yesterday because I followed the assertion message verbatim, definitely want to echo how nearly undebuggable this makes the test (if I ran the test once, it worked fine. Twice, any code after awaiting that future would never run).

I'm not a Dart expert, so I don't really know why, but for me it seemed doing thenAnswer((_) async => future) worked 100% of the time, possibly because it guarantees the future is created inside the callback? If that's a viable solution and not just dumb luck, it might be nice to change the assertion message to suggest making the lambda async.