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
[new rule] prefer-to-throw / consistent-expect-error #295
Comments
Matching any try-catch where the catch block contains an |
I don't know enough about eslint to respond to this concern, but I would accept this as a limitation and document it. I would just use a This would also mean it should not be enabled in the recommended config (or it would enable to
I think this is a great additional rule and should be enabled as part of the recommended config. This should include a |
I don't know enough about eslint to respond to this concern, but I would
accept this as a limitation and document it.
Yeah, documenting the limitation might suffice in this case.
I think this is a great additional rule
Additional? I thought this was what your code examples are about?
…On Thu, 11 Jul 2019, 19:30 Chris Blossom, ***@***.***> wrote:
I don't see a good way to avoid false positives where the assertions in
the catch blocks do more than just check the error message
I don't know enough about eslint to respond to this concern, but I would
accept this as a limitation and document it. I would just use a //
eslint-disable-next-line consistent-expect-error if there is an actual
need to use a try / catch.
This would also mean it should not be enabled in the recommended config
(or it would enable to try / catch syntax, which I don't think is
preferred. For example, typescript/no-explicit-any
<https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-explicit-any.md>
is a great rule to use, but sometimes you need any.
Matching any try-catch where the catch block contains an expect to
recommend using expect.assertions/expect.hasAssertions
I think this is a great additional rule and should be enabled as part of
the recommended config. This should include a .catch for a promise.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AD2TI55F7HD5F2VQGUI7V5LP65U33A5CNFSM4H7IC7X2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZXNJGA#issuecomment-510579864>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AD2TI53C2PEJPPNXKPSYJDLP65U33ANCNFSM4H7IC7XQ>
.
|
I guess looking back it doesn't seem to fit in the description of |
We have a similar rule to this at Shopify: https://github.com/Shopify/eslint-plugin-shopify/blob/master/docs/rules/jest/no-try-expect.md for preventing the try/catch part. I was already planning on submitting a PR for it soon. @SimenB @jeysal, would this be a rule you are interested in having as part of this repo? @chrisblossom I think that would help prevent some of what you are looking to do. |
@cartogram yeah, definitely! |
eslint-plugin-shopify/jest/no-try-expect work great if jestjs/jest#8718 gets merged, but if it doesn't I'd prefer a more intelligent rule (and named accordingly) as discussed here that prefers the Valid: test('toThrow', () => {
expect(() => {
throw new Error('boom');
}).toThrow('boom');
});
test('try/catch 2+ expect', () => {
expect(() => {
throw new Error('boom');
}).toThrow('boom');
expect.hasAssertions();
try {
throw new Error('boom');
} catch (error) {
// has multiple expects
expect(error.message).toEqual('boom');
expect(error.code).toEqual('NOT_FOUND');
}
}); Invalid: test('try/catch one expect', () => {
// invalid, prefer .toThrow
expect.hasAssertions();
try {
throw new Error('boom');
} catch (error) {
expect(error.message).toEqual('boom');
}
});
test('try/catch missing hasAssertions', () => {
// missing hasAssertions but multiple expects
// expect.hasAssertions();
try {
throw new Error('boom');
} catch (error) {
expect(error.message).toEqual('boom');
expect(error.code).toEqual('NOT_FOUND');
}
}); EDIT: I'm not sure how this rule will work with aggregate errors without jestjs/jest#8718. Examples: If jestjs/jest#8718 does land, I think we could also add a fixer to this rule(however it gets merged). |
I too don't really understand the existing
I want to be able to assert on things that happened after my async invocation. I think that the warning should be removed if |
@ron23 you should be able to use
|
almost - need to be We don't have a really clean way if you wanna make multiple assertions on the error, though. |
Thanks @SimenB As for not having solution for multiple assertion means I have to disable the rule :( |
re my response: I had just woken up and have not retained the context for this issue since it's 6 months old, so I freeballed it to push you in the right direction 😂 I expected it to be "mostly right", but should have mentioned "something like [code example]" to make that clearer. @SimenB's code example is actually also "almost" - it actually needs to be:
So to break down whats going on here, and why:
Overall I think at this point (thats again without having the context from 6 months ago), I would personally recommend using a utility function of some kind if you want to test things on the actual error:
The benefit to me is that the That way you don't duplicate that across your tests, where it's possible make a typo that could still pass in your testbase. Granted we have tools to help reduce the likelihood of this, but it's still possible. (This is one of the reasons why In addition, we're balancing usefulness & time-saving-potential against complexity and maintainability:
Detecting if However, improving the text is definitely something we could do easily; I've not looked into the message myself, but more than happy to discuss that :) |
@G-Rath Thanks for the suggestion! One minor comment, shouldn’t the catch in getError rethrow if the try’d part throws “no error was thrown!”? Otherwise the caller would have to check this known error each invocation. |
@jp7837 (it's been another 6 months, so this is all iirc)
Yes, that's the point. The idea is that you're wanting to perform checks against the error, which will always fail against our If you throw, then you'll still get told the test failed, but not in a nice way because there's a difference between an error being thrown and an assertion failing :) So, with the above helper these would all fail as expected:
The above will would all assert gracefully if no error was actually thrown :) |
@chrisblossom I'm just re-reading this issue, and not sure what the action(s) for it are - we've now got I think to keep things clean it might be good if we closed this issue, and you can open new issues if you have any bugs, questions, features, etc - is that ok with you? |
🎉 This issue has been resolved in version 24.4.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This issue has been resolved in version 25.0.0-next.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I would like to see a new rule named
prefer-to-throw
, orconsistent-expect-error
that would (preferably) default to the.toThrow
syntax.If
consistent-expect-error
is set to use thetry/catch
syntax, an error should be thrown ifexpect.hasAssertions
(or a variation of) has not been set.The text was updated successfully, but these errors were encountered: