Chore: avoid loose equality assertions #9415
Conversation
This updates the `no-restricted-syntax` configuration to disallow
`assert.equal` and `assert.notEqual` in favor of `assert.strictEqual`
and `assert.notStrictEqual`. It also fixes a few places where the tests
were relying on a loose equality check (e.g. comparing a single-element
array to a string).
|
LGTM |
tests/lib/config.js
Outdated
| assert.equal(actual.length, 1); | ||
| assert.equal(actual, expected); | ||
| assert.strictEqual(actual.length, 1); | ||
| assert.deepEqual(actual, [expected]); |
There was a problem hiding this comment.
Given that there's a length assertion above this line, we could assert for strict equality as follows:
assert.strictEqual(actual[0], expected);There was a problem hiding this comment.
Actually, it looks like assert.deepStrictEqual is supported on all necessary Node versions, so maybe we should switch to using that as well.
There was a problem hiding this comment.
I'd be in favor of that for sure. Maybe we could add assert.deepEqual to no-restricted-properties as well?
tests/lib/ignored-paths.js
Outdated
| @@ -127,14 +127,14 @@ describe("IgnoredPaths", () => { | |||
| const ignorePatterns = getIgnorePatterns(ignoredPaths); | |||
|
|
|||
| assert.isNotNull(ignoredPaths.baseDir); | |||
| assert.equal(getIgnoreFiles(ignoredPaths), expectedIgnoreFile); | |||
| assert.deepEqual(getIgnoreFiles(ignoredPaths), [expectedIgnoreFile]); | |||
There was a problem hiding this comment.
Similarly, by asserting on length here, we could also do a strict comparison of the first element.
tests/lib/options.js
Outdated
| @@ -92,7 +92,7 @@ describe("options", () => { | |||
| const currentOptions = options.parse("--rulesdir /morerules"); | |||
|
|
|||
| assert.isArray(currentOptions.rulesdir); | |||
| assert.equal(currentOptions.rulesdir, "/morerules"); | |||
| assert.deepEqual(currentOptions.rulesdir, ["/morerules"]); | |||
There was a problem hiding this comment.
Could achieve strict comparison by comparing length and first element.
tests/performance/jshint.js
Outdated
| @@ -9596,10 +9596,10 @@ assert.ok = ok; | |||
|
|
|||
| // 5. The equality assertion tests shallow, coercive equality with | |||
| // ==. | |||
| // assert.equal(actual, expected, message_opt); | |||
| // assert.strictEqual(actual, expected, message_opt); | |||
tests/performance/jshint.js
Outdated
|
|
||
| assert.equal = function equal(actual, expected, message) { | ||
| if (actual != expected) fail(actual, expected, message, '==', assert.equal); | ||
| assert.strictEqual = function equal(actual, expected, message) { |
There was a problem hiding this comment.
Would it make more sense to reconfigure no-restricted-properties for this file to allow the local assert.equal? I'm assuming this is third party code, maybe I'm misunderstanding.
There was a problem hiding this comment.
Oops, this was an accidental change resulting from a global find/replace.
|
LGTM |
|
Thanks for contributing to ESLint! |
eslint-deprecated
bot
added
the
archived due to age
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:
What changes did you make? (Give an overview)
This updates the
no-restricted-syntaxconfiguration to disallowassert.equalandassert.notEqualin favor ofassert.strictEqualandassert.notStrictEqual. It also fixes a few places where the tests were relying on a loose equality check (e.g. comparing a single-element array to a string).A few loose equality assertions in
RuleTesterwere kept for backwards compatibility.Is there anything you'd like reviewers to focus on?
Nothing in particular