test: fixes for more strict linting #7920
Conversation
Trott
added
wip
| })); | ||
| .on('online', function() { this.disconnect(); }) | ||
| .on('exit', common.mustCall(function(status) { | ||
| assert.equal(status, SENTINEL); |
|
+1 for better readability. LGTM. |
| .from(b.toString('hex'), 'hex') | ||
| .includes(Buffer.from('64', 'hex'), 0, 'hex'), | ||
| true | ||
| ); |
There was a problem hiding this comment.
Is this really an improvement? It looks like the opposite to my eyes...
There was a problem hiding this comment.
@Slayer95 This was a bit outside the scope of what is described in the commit message, so (as @targos suggested previously), I should update the commit message. This is an improvement (in my opinion, anyway) because the previous test passed as long as buf.includes() returned any truthy value. But, in fact, buf.includes() is broken if it returns some other value than true. So this makes the test more strict.
However, in the process of making the test better, we've made the assertion less readable because now there is a second argument and the previous layout messes with the second argument.
If you do this, then it's easy to miss the second argument entirely:
assert.strictEqual(
Buffer.from(b.toString('hex'), 'hex')
.includes(Buffer.from('64', 'hex'), 0, 'hex'), true
);This is better, but it's not immediately clear that true is the second argument and not the third:
assert.strictEqual(
Buffer.from(b.toString('hex'), 'hex')
.includes(Buffer.from('64', 'hex'), 0, 'hex'),
true
);This next one is more lines to take in at once than might be ideal, but once you see the pattern, it's easy to scan the subsequent occurrences of the pattern. The same is not true for the above. They are more difficult to scan, especially if the number of chained properties in the first argument varies or if the number of arguments varies. So, in my opinion, this is the more readable formatting:
assert.strictEqual(
Buffer
.from(b.toString('hex'), 'hex')
.includes(Buffer.from('64', 'hex'), 0, 'hex'),
true
);I think the above (which is the one I chose) is better than this next one, but I could live with this too:
assert.strictEqual(
Buffer.from(b.toString('hex'), 'hex')
.includes(Buffer.from('64', 'hex'), 0, 'hex'),
true
);
|
Addressed comments by @targos (including improving the commit message) , squashed, force pushed. |
|
I generally think this is an improvement - especially the changes to assert.strictEqual(
Buffer.from(b.toString('hex'), 'hex')
.includes(Buffer.from('64', 'hex'), 0, 'hex'),
true
); |
|
LGTM |
|
Changed to format preferred by @cjihrig. |
|
LGTM (although that might be premature for a WIP). |
|
Removing the |
Trott
changed the title
A new version of ESLint flags chained properties on multiple lines that were not flagged by the previous version of ESLint. In preparation for turning that feature on, adjust alignment to that expected by the linter. This change happened to be predominantly around assertions using `assert()` and `assert.equal()`. These were changed to `assert.strictEqual()` where possible.
Trott
changed the title
A new version of ESLint flags chained properties on multiple lines that were not flagged by the previous version of ESLint. In preparation for turning that feature on, adjust alignment to that expected by the linter. This change happened to be predominantly around assertions using `assert()` and `assert.equal()`. These were changed to `assert.strictEqual()` where possible. PR-URL: nodejs#7920 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
|
Landed in b4258bb |
Refs: nodejs#7920 PR-URL: nodejs#7999 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
A new version of ESLint flags chained properties on multiple lines that were not flagged by the previous version of ESLint. In preparation for turning that feature on, adjust alignment to that expected by the linter. This change happened to be predominantly around assertions using `assert()` and `assert.equal()`. These were changed to `assert.strictEqual()` where possible. PR-URL: #7920 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Conflicts: test/parallel/test-readline-interface.js
Refs: #7920 PR-URL: #7999 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
tools test
Description of change
A new version of ESLint flags chained properties on multiple lines that
were not flagged by the previous version of ESLint. In preparation for
turning that feature on, adjust alignment to that expected by linter.
WIP. Will probably update ESLint itself after
3.2.1 is released, likely on Monday.a new version is released on August 12 (either 3.2.2 or 3.3.0). (The current 3.2.1 flags this issue a bit too liberally and would result in enormous churn.)These changes seem to me to improve readability in most cases (especially the
readlinetest file) and also provide an opportunity to change some instances ofassert.equal()andassert()toassert.strictEqual().