-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Chore: remove assert.doesNotThrow in tests #10199
Conversation
I'll leave a note here that while I will defer to team consensus, I like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request! I left a few suggestions for changes that I'd like to see if we do end up accepting this.
That said, I agree with @platinumazure -- I think assert.doesNotThrow
is useful for explicitness. I doubt the performance impact is very significant.
tests/lib/config/config-validator.js
Outdated
@@ -109,7 +109,7 @@ describe("Validator", () => { | |||
it("should do nothing with an empty config", () => { | |||
const fn = validator.validate.bind(null, {}, "tests", ruleMapper, linter.environments); | |||
|
|||
assert.doesNotThrow(fn); | |||
fn(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to convert these to direct calls, e.g.
validator.validate({}, "tests", ruleMapper, linter.environments);
tests/lib/cli.js
Outdated
cli.execute(code); | ||
}); | ||
|
||
cli.execute(code); | ||
cli.execute(code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to me like the original assert.doesNotThrow
call was redundant, so I think it can just be removed rather than duplicating this call.
tests/bin/eslint.js
Outdated
); | ||
|
||
// "Cache file should contain valid JSON" | ||
JSON.parse(fs.readFileSync(CACHE_PATH, "utf8")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Can you remove the quotes around these comments?
Comments addressed. About being explicit: often the tests are named as: |
The API is not really useful and just slows down the test run by catching errors and then rethrowing in case of an error. In case there is no error, it is a noop.
c6d3976
to
9307af2
Compare
Rebased due to conflicts. |
I missed this comment:
So I'll go with consensus here. I withdraw my objections. |
@not-an-aardvark There have been some changes since your last review. Have your concerns been addressed? |
Merged. Thanks for contributing and apologies for holding this up so long. |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:
The
assert.doesNotThrow
API is not useful and just slows down the test run bycatching errors and then rethrowing in case of an error. In case
there is no error, it is a noop.
What changes did you make? (Give an overview)
Removed all
assert.doesNotThrow
calls and added a eslint rule to prevent any new additions.Is there anything you'd like reviewers to focus on?
The wording for the error message.