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

Bug Report: expectRejectCase & expectResolveCase is not working #254

Closed
yatki opened this issue May 8, 2019 · 4 comments · Fixed by #255
Closed

Bug Report: expectRejectCase & expectResolveCase is not working #254

yatki opened this issue May 8, 2019 · 4 comments · Fixed by #255
Labels

Comments

@yatki
Copy link
Contributor

yatki commented May 8, 2019

Hello, I was interested to solve the issue #54 and I was looking at the code base.

const expectResolveCase = node =>
expectCase(node) &&
node.parent.parent.type === 'MemberExpression' &&
methodName(node) === 'resolve';
const expectRejectCase = node =>
expectCase(node) &&
node.parent.parent.type === 'MemberExpression' &&
methodName(node) === 'reject';

I realised that expectRejectCase & expectResolveCase functions are actually not working. methodName(node) === 'reject'; here the method name must be rejects I think, and the same goes with resolves.

I found the usages for those functions and removing them actually doesn't break anything, means they were not working.

So, would it be ok if i fix these 2 functions and use them to solve issue #54 ?

@SimenB
Copy link
Member

SimenB commented May 8, 2019

If they do nothing, a PR removing them is very much welcome 🙂

@yatki
Copy link
Contributor Author

yatki commented May 8, 2019

Sure, PR is on the way. Just a few things to clarify:

  1. Actually I prefer to fix and use those utility functions. I think they make identifying nodes easier for everyone and I like reusable logic :).
  2. For the issue Assert that async expects are awaited or returned #54, does it make sense to have a new rule, something like called valid-async-expect. Because I feel like expect statement with .not, .resolves, .rejects modifiers are valid. The way we handle them (not returning or not awaiting) is invalid. I am fine with refactoring valid-expect as well. Just let me know what you think.
  3. In either way, I'm thinking to add a boolean configuration option for the rule, something like alwaysAwait to make handling async expect statements consistent. Because if there is a single async expect, returning it is also fine. Please let me know if you have other ideas on this.

As soon as, we clarify these topics, I'll start working on this rule and probably submit the PR tonight or tomorrow.

@SimenB
Copy link
Member

SimenB commented May 8, 2019

  1. Sure, whatever's cleanest
  2. I think it should be in the same rule, as it asserts the same thing (from the user's perspective - the expect call is valid, async or not doesn't matter)
  3. Enforcing await can be a flag, sure 🙂

@SimenB
Copy link
Member

SimenB commented Jul 20, 2019

🎉 This issue has been resolved in version 22.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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 a pull request may close this issue.

2 participants