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

test: fixes for more strict linting #7920

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 30, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected 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 readline test file) and also provide an opportunity to change some instances of assert.equal() and assert() to assert.strictEqual().

@Trott Trott added wip Issues and PRs that are still a work in progress. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jul 30, 2016
}));
.on('online', function() { this.disconnect(); })
.on('exit', common.mustCall(function(status) {
assert.equal(status, SENTINEL);
Copy link
Member

Choose a reason for hiding this comment

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

strictEqual ?

@targos
Copy link
Member

targos commented Jul 30, 2016

+1 for better readability. LGTM.
The commit message could be a bit more precise about what is changed.

.from(b.toString('hex'), 'hex')
.includes(Buffer.from('64', 'hex'), 0, 'hex'),
true
);

Choose a reason for hiding this comment

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

Is this really an improvement? It looks like the opposite to my eyes...

Copy link
Member Author

@Trott Trott Jul 30, 2016

Choose a reason for hiding this comment

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

@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
);

@Trott
Copy link
Member Author

Trott commented Jul 30, 2016

Addressed comments by @targos (including improving the commit message) , squashed, force pushed.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 1, 2016

I generally think this is an improvement - especially the changes to strictEqual(). I personally prefer the following style from your examples in #7920 (comment).

assert.strictEqual(
  Buffer.from(b.toString('hex'), 'hex')
    .includes(Buffer.from('64', 'hex'), 0, 'hex'),
  true
);

@jasnell
Copy link
Member

jasnell commented Aug 1, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Aug 1, 2016

Changed to format preferred by @cjihrig.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 1, 2016

LGTM (although that might be premature for a WIP).

@Trott
Copy link
Member Author

Trott commented Aug 1, 2016

Removing the in progress label as there's no reason not to land this if it is generally regarded as an improvement. (I think the equal->strictEqual stuff may be sufficient justification to land sooner rather than later.)

@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Aug 1, 2016
@Trott Trott changed the title [WIP] tools: fixes for more strict linting [WIP] test: fixes for more strict linting Aug 1, 2016
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
Copy link
Member Author

Trott commented Aug 1, 2016

@Trott Trott changed the title [WIP] test: fixes for more strict linting test: fixes for more strict linting Aug 1, 2016
Trott added a commit to Trott/io.js that referenced this pull request Aug 2, 2016
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 <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member Author

Trott commented Aug 2, 2016

Landed in b4258bb

@Trott Trott closed this Aug 2, 2016
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Trott added a commit to Trott/io.js that referenced this pull request Aug 9, 2016
Trott added a commit to Trott/io.js that referenced this pull request Aug 10, 2016
Refs: nodejs#7920
PR-URL: nodejs#7999
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
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 <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Conflicts:
	test/parallel/test-readline-interface.js
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
Refs: #7920
PR-URL: #7999
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
@Trott Trott deleted the eslint-update branch October 10, 2021 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants