Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rules): no-expect-resolves #364

Merged
merged 7 commits into from
Aug 7, 2019
Merged

Conversation

eranshabi
Copy link
Contributor

This rule prevents the use of await expect(promise).resolves in favor of expect(await promise),
for consistency and readability.

},
});

ruleTester.run('no-expect-resolves', rule, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this rule should probably have more tests, but I ran it against Jest and found no errors (at least no false positives and no crashes)

@SimenB SimenB merged commit f41d5c4 into jest-community:master Aug 7, 2019
@SimenB
Copy link
Member

SimenB commented Aug 7, 2019

Thanks for the contribution @eranshabi!

@SimenB
Copy link
Member

SimenB commented Aug 7, 2019

🎉 This PR is included in version 22.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SimenB SimenB added the released label Aug 7, 2019
@mrtnzlml
Copy link
Contributor

mrtnzlml commented Aug 7, 2019

@SimenB Hi, I have just a silly question - what is the recommended style? Is it await expect(promise).resolves or expect(await promise)? Just wondering why does this rule exist with such defaults. Maybe it's because resolves is going to be deprecated? Thanks! :)

@G-Rath
Copy link
Collaborator

G-Rath commented Aug 7, 2019

@mrtnzlml I'm no Simen, but can weight in :)

While I'm not part of the jest team, so can't say this "Officially", I don't think they'll be deprecating resolves, unless Promises go away.

The idea behind this rule is to encourage the usage of await, which tends to make testing promises a lot nicer, as it lets you test asynchronous values as if they're synchronous without having to manage callbacks or returns.

It's easy to write passing tests that don't actually test promised values, but from all angles look like they are.

The other thing is that await was only added in Node 7.5, and it's still something a lot of people are learning to use :)

The writeup for this rule could use some refinement, as as you've found it's example jumps straight to using await in the before & after, giving the impression that resolves is bad, when really it's about encouraging await.

We'd welcome some more examples & tests, so feel free to make a PR if you'd like :)

@SimenB
Copy link
Member

SimenB commented Aug 8, 2019

We won't be removing resolves as its sibling rejects is not ergonomic to use with await. You could do something like expect(await promise.then(res => Promise.reject(res), res => res)).toEqual(someRejectedValue), but it's not really readable compared to await expect(promise).rejects.toEqual(someRejectedValue). Could of course extract that into a helper, but you might not want abstractions like that in your tests. (the alternative is a try-catch, which has its own caveats, like asserting that it actually rejects).

As to what you should use - up to you! I generally prefer expect(await promise) but if I'm testing a bunch of resolving and rejecting promises at once it might be easier to read if the assertions are kept in the same style. I merged the rule since I personally prefer less library boilerplate, but this rule won't be made part of the recommended rules as that's purely a personal preference.


Note that after resolves/rejects was added to Jest (jestjs/jest#3068), we've added async matchers (jestjs/jest#5919), so you could add a custom toRejectAndEqual matcher or whatever you want. However, resolves and rejects won't go away as they allow you to make any matcher, custom or bundled, work with promises instead of having to reimplement existing sync matchers.

Hope that clears things up 🙂

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

Successfully merging this pull request may close these issues.

4 participants