Use done.fail(e) instead of done(e) for forced failures

[jamesreggio]

I hate to be the harbinger of bad news, but it looks like some of the tests have been failing silently.

I'm not 100% certain about this, but it appears that Jest and Jasmine have different semantics for the done function, used with async tests. Specifically, while Jasmine recognizes done(e) to be a failure, Jest just views any invocation of done to be a success, regardless of the presence of an argument.

From what I can tell, to force a failure in Jest you have to use, sigh, an undocumented method: done.fail(e). I only discovered this after finding jestjs/jest#1801 after a fair bit of Googling. Any Jest experts are welcome to suggest an alternative.

I've manually transformed all of the sites I could find within the apollo-client package, but the same needs to be done for all of the other packages in the repo.

[mention-bot]

@jamesreggio, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jbaxleyiii, @Slava and @calebmer to be potential reviewers.

[apollo-cla]

	Fails
🚫	No CHANGELOG added.

	Messages
📖	Please add your name and email to the AUTHORS file (optional)
📖	If this was a change that affects the external API, please update the docs and post a link to the PR in the discussion

Generated by 🚫 dangerJS

[jbaxleyiii]

oh no! That's no good at all! Thank you for bringing this up! I'll take a look!

[jbaxleyiii]

@jamesreggio this also explains why the jest 21 build was failing. jest 21 supports throwing via done(error).

[jbaxleyiii]

@jamesreggio I'm going to close this in favor of the jest 21 PR but THANK YOU for bringing this up!

[codecov]

Codecov Report

Merging #2199 into master will decrease coverage by 0.41%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2199      +/-   ##
==========================================
- Coverage    84.4%   83.99%   -0.42%     
==========================================
  Files          35       35              
  Lines        1949     1949              
  Branches      458      458              
==========================================
- Hits         1645     1637       -8     
- Misses        298      306       +8     
  Partials        6        6

Impacted Files	Coverage Δ
...ckages/apollo-client/src/util/subscribeAndCount.ts	100% <ø> (ø)	⬆️
packages/apollo-client/src/util/wrap.ts	59.09% <ø> (ø)	⬆️
packages/apollo-client/src/data/store.ts	96.61% <0%> (-1.7%)	⬇️
packages/apollo-client/src/core/QueryManager.ts	91.66% <0%> (-1.67%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8a6a5a...0c11235. Read the comment docs.

[jbaxleyiii]

Actually jest 21 appears to not respect the enumerability of object keys so it broke nearly all of our store related tests 👎 So I think we have to stay at jest 20 for now which stinks because 21 has a nice perf jump

[jamesreggio]

Wow... I can't believe that they're going to change the semantics of done in Jest 21.

If the existing done function ignores arguments, it's easy to pass a bare done as the recipient of any callback without concern about the arguments passed to the callback. As soon as it begins interpreting arguments as an error condition, all kinds of existing tests are going to break.

For example, the event argument received by the onPress callback will change this test from passing to failing.

it('fires the onPress callback', done => {
  const wrapper = shallow(<TouchableOpacity onPress={done} />);
  wrapper.find('TouchableOpacity').simulate('press');
});

I'd keep a careful eye on whether they waffle and retract that semantic change, because I think many Jest users are in for a world of hurt.

[jbaxleyiii]

I'm pretty inclined to agree! I'm frustrated as well by the change in the toEqual practice (at least from what I can tell its a change) which is going to make updating to 21 pretty much a non starter. I think its best if we skip 21 and I'll work on fixing these tests or the underlying features as needed

[jamesreggio]

I shared this FUD on the most relevant Jest PR I could find, over here: jestjs/jest#4016 (comment)

And just for notice, I didn't use a codemod to make the updates in this branch, so it's quite possible that I missed something.

In fact, my main concern is the relative un-greppability of the bare usage of done as the recipient of error callbacks, i.e.:

somePromise
  .then(() => done())
  .catch(done);

...wherein the payload to the catch callback is anticipated to fail the test.

All that to say, just keep an eye out for things I may have missed.

[jbaxleyiii]

cherry picked and moved to #2213