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

tools: eslint rule for assert.throws -> common.expectsError #17557

Closed

Conversation

apapirovski
Copy link
Member

Pretty straight-forward — eslint rule that checks for usage of assert.throws(fn, common.expectsError(err)); and suggests using common.expectsError(fn, err); instead.

First commit fixes all remaining instances that violate this rule. Second commit introduces the rule & a test for it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Dec 8, 2017

module.exports = function(context) {
return {
CallExpression(node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could no-restricted-syntax handle this?

Copy link
Member Author

@apapirovski apapirovski Dec 9, 2017

Choose a reason for hiding this comment

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

Hmm, possibly but it'll definitely be a pretty convoluted rule in that case. There's no "or" for AST selectors, as far as I know, so it would get super verbose and repetitive to account for all these possibilities. Even with "or" it would be super long.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the "or" condition since that checks for a function as the first argument to assert.throws(), which is always required. This selector seems to match all of the cases you have in this PR:

"CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<3][arguments.1.type='CallExpression'][arguments.1.callee.object.name='common'][arguments.1.callee.property.name='expectsError']"

It's not beautiful, but it might be able to be simplified further.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. That should work. Will update.

Copy link
Member

@Trott Trott Dec 9, 2017

Choose a reason for hiding this comment

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

A possible problem with no-restricted-syntax here is that this is a rule that should only apply in test. I'm not sure if there's a way to specify no-restricted-syntax in test in a way that won't override no-restricted-syntax from the config file in the parent directory. /cc @not-an-aardvark

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm guessing that's why we also have prefer-common-mustnotcall rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, there's not a good way to do that at the moment. As a workaround, you could copy-paste the configuration from the parent directory, add the new entry, and put the resulting config in test/.eslintrc.yaml. This would have the desired effect, but it could cause the two configs to get out of sync if someone updates .eslintrc.yaml later on and forgets to update the copied version in test/.eslintrc.yaml.

There is some discussion on improving this in eslint/eslint#9192.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's unfortunate. I'm not a fan of adding custom rules that we don't need, but keeping the two config files in sync seems like a worse idea.

@Trott
Copy link
Member

Trott commented Dec 9, 2017

This might be overly ambitious but here's an idea: Instead of forcing common.expectsError() in all of test, implement the missing functionality in assert.throws() itself and then get rid of common.expectsError() altogether.

Doesn't have to be in this PR, though, especially since it might stall due to some resistance. Can be a future enhancement.

@apapirovski
Copy link
Member Author

apapirovski commented Dec 9, 2017

This might be overly ambitious but here's an idea: Instead of forcing common.expectsError() in all of test, implement the missing functionality in assert.throws() itself and then get rid of common.expectsError() altogether.

I've thought about that before but is that something we even want? The Node.js core error objects are pretty specific and the functionality within common.expectsError is pretty targeted towards those. We could always make it more generic but then it would lose quite a bit of the functionality.

@apapirovski apapirovski force-pushed the patch-eslint-test-throws branch 4 times, most recently from 155251f to 849a7a1 Compare December 9, 2017 16:29
@apapirovski
Copy link
Member Author

apapirovski commented Dec 9, 2017

@cjihrig PTAL — I've updated it to use AST selectors instead. Thanks for the suggestion.

Add lint rule to validate that common.expectsError is being used
instead of assert.throws(fn, common.expectsError(err));
@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/12004/

(had an extra space in one of the error messages, force pushed a fix)

@BridgeAR
Copy link
Member

BridgeAR commented Dec 9, 2017

@Trott as @apapirovski already pointed out common.expectsError is very specific for Node.js. The functionality would definitely deviate from the current implementation and it would be specific for assert.throws and assert.doesNotThrow would not work that similar anymore.

@BridgeAR BridgeAR mentioned this pull request Dec 10, 2017
4 tasks
@apapirovski
Copy link
Member Author

Landed in 094bfaf and e51fb90

@apapirovski apapirovski deleted the patch-eslint-test-throws branch December 11, 2017 23:26
apapirovski added a commit that referenced this pull request Dec 11, 2017
PR-URL: #17557
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
apapirovski added a commit that referenced this pull request Dec 11, 2017
Add lint rule to validate that common.expectsError(fn, err) is being
used instead of assert.throws(fn, common.expectsError(err));

PR-URL: #17557
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17557
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Add lint rule to validate that common.expectsError(fn, err) is being
used instead of assert.throws(fn, common.expectsError(err));

PR-URL: #17557
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #17557
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Add lint rule to validate that common.expectsError(fn, err) is being
used instead of assert.throws(fn, common.expectsError(err));

PR-URL: #17557
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Could this be backported to v8.x-staging and v6.x-staging?

guide

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 21, 2017
From now on it is possible to use a validation object in throws
instead of the other possibilites.

PR-URL: nodejs#17584
Refs: nodejs#17557
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Mar 8, 2018
From now on it is possible to use a validation object in throws
instead of the other possibilites.

PR-URL: nodejs#17584
Refs: nodejs#17557
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 15, 2018
From now on it is possible to use a validation object in throws
instead of the other possibilites.

Backport-PR-URL: #19230
PR-URL: #17584
Refs: #17557
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
From now on it is possible to use a validation object in throws
instead of the other possibilites.

Backport-PR-URL: #19230
PR-URL: #17584
Refs: #17557
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018
From now on it is possible to use a validation object in throws
instead of the other possibilites.

PR-URL: nodejs#17584
Refs: nodejs#17557
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
From now on it is possible to use a validation object in throws
instead of the other possibilites.

Backport-PR-URL: #23223
PR-URL: #17584
Refs: #17557
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants