test: change equal to strictEqual in path tests #8628

lrlna · 2016-09-17T12:59:37Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

tests

Description of change

Use strict equality in test-path set of unit tests.

MylesBorins · 2016-09-17T13:11:26Z

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

cjihrig

LGTM

addaleax · 2016-09-17T14:19:55Z

test/parallel/test-path.js


 // POSIX filenames may include control characters
 // c.f. http://www.dwheeler.com/essays/fixing-unix-linux-filenames.html
 const controlCharFilename = 'Icon' + String.fromCharCode(13);
-assert.equal(path.posix.basename('/a/b/' + controlCharFilename),
+assert.strictEqual(path.posix.basename('/a/b/' + controlCharFilename),
             controlCharFilename);


could you try and align the arguments here and in cases like this? :)

@addaleax ofc!

Fishrock123

Seems fine other than some alignment issues.

@lrlna can you try running make -j4 lint? I think it should catch these... maybe not though.

I don't think this should cause too many conflicts as I'm not aware of any major changes to path recently.

Fishrock123 · 2016-09-17T17:26:01Z

test/parallel/test-path.js

+                   '\\\\unc\\share\\');
+assert.strictEqual(path.win32.dirname('\\\\unc\\share\\foo\\'),
+                   '\\\\unc\\share\\');
+assert.strictEqual(path.win32.dirname('\\\\unc\\share\\foo\\bar'),
             '\\\\unc\\share\\foo');


Fishrock123 · 2016-09-17T17:26:11Z

test/parallel/test-path.js

             '\\\\unc\\share\\foo');
-assert.equal(path.win32.dirname('\\\\unc\\share\\foo\\bar\\baz'),
+assert.strictEqual(path.win32.dirname('\\\\unc\\share\\foo\\bar\\baz'),
             '\\\\unc\\share\\foo\\bar');


and done 🎉

addaleax · 2016-09-17T18:52:04Z

@Fishrock123 The linter does not catch this, and I think we’ve talked about maybe enabling a rule like that, so… @Trott?

Trott · 2016-09-17T20:02:23Z

I implemented the align-function-arguments custom rule but it's intentionally very lenient and ignores cases like this. I (or someone else) could look at dialing it up a bit. I think my initial concern was tons of churn and the accompanying probable bike-shedding over oddball exceptions.

Trott · 2016-09-17T20:32:07Z

Quick implementation suggests enabling alignment for this situation will find around 66 alignment problems in the existing code base. That sounds like the upper edge of acceptable churn for something like this to me so I'll put together a PR.

@Irina, if you could align the instances this PR, that would be great. Sorry we didn't have a lint rule in place to catch it before it got submitted as a PR.

addaleax · 2016-09-17T20:38:45Z

@Trott I think it’s worth it because usually someone will point out those misalignments… maybe wait a couple of days, though. 😄

Trott · 2016-09-17T20:51:27Z

(There will still be lots of alignment issues that the rule doesn't catch but I'm OK with slowly ratcheting it up over time rather than all at once.)

Trott · 2016-09-18T04:01:20Z

@addaleax @Fishrock123 Here's a PR with the alignment linting: #8642

lrlna · 2016-09-18T11:08:13Z

howdy, apologies for disappearing! Miss-alignments have been fixed and pushed 💯

addaleax

CI: https://ci.nodejs.org/job/node-test-commit/5142/

LGTM!

A few nits in recent PR comments suggest that we can have slightly more strict linting for argument alignment in multiline function calls. This enables the existing linting requirements to apply when one or more of the arguments themselves are function calls. Previously, that situation had been excluded from linting. Refs: nodejs#8628 (comment) PR-URL: nodejs#8642 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

jasnell

LGTM! Awesome job!

PR-URL: #8628 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell · 2016-09-20T18:19:45Z

Landed in daa0f90! thank you!

lrlna · 2016-09-20T18:26:23Z

omg YAY! 🎉

A few nits in recent PR comments suggest that we can have slightly more strict linting for argument alignment in multiline function calls. This enables the existing linting requirements to apply when one or more of the arguments themselves are function calls. Previously, that situation had been excluded from linting. Refs: #8628 (comment) PR-URL: #8642 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

PR-URL: #8628 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

test: changing equal to strictEqual in path

0134505

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016

cjihrig approved these changes Sep 17, 2016

View reviewed changes

addaleax added the path Issues and PRs related to the path subsystem. label Sep 17, 2016

addaleax reviewed Sep 17, 2016

View reviewed changes

Fishrock123 suggested changes Sep 17, 2016

View reviewed changes

align function arguments in test-path

5a1769b

addaleax approved these changes Sep 19, 2016

View reviewed changes

jasnell approved these changes Sep 20, 2016

View reviewed changes

jasnell pushed a commit that referenced this pull request Sep 20, 2016

test: changing equal to strictEqual in path

daa0f90

PR-URL: #8628 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell closed this Sep 20, 2016

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016

test: changing equal to strictEqual in path

5a59ca6

PR-URL: #8628 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: change equal to strictEqual in path tests #8628

test: change equal to strictEqual in path tests #8628

lrlna commented Sep 17, 2016

MylesBorins commented Sep 17, 2016

cjihrig left a comment

addaleax Sep 17, 2016

lrlna Sep 18, 2016

Fishrock123 left a comment

Fishrock123 Sep 17, 2016

lrlna Sep 18, 2016

Fishrock123 Sep 17, 2016

lrlna Sep 18, 2016

addaleax commented Sep 17, 2016

Trott commented Sep 17, 2016

Trott commented Sep 17, 2016

addaleax commented Sep 17, 2016

Trott commented Sep 17, 2016

Trott commented Sep 18, 2016

lrlna commented Sep 18, 2016

addaleax left a comment

jasnell left a comment

jasnell commented Sep 20, 2016

lrlna commented Sep 20, 2016

test: change equal to strictEqual in path tests #8628

test: change equal to strictEqual in path tests #8628

Conversation

lrlna commented Sep 17, 2016

Checklist

Affected core subsystem(s)

Description of change

MylesBorins commented Sep 17, 2016

cjihrig left a comment

Choose a reason for hiding this comment

addaleax Sep 17, 2016

Choose a reason for hiding this comment

lrlna Sep 18, 2016

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 Sep 17, 2016

Choose a reason for hiding this comment

lrlna Sep 18, 2016

Choose a reason for hiding this comment

Fishrock123 Sep 17, 2016

Choose a reason for hiding this comment

lrlna Sep 18, 2016

Choose a reason for hiding this comment

addaleax commented Sep 17, 2016

Trott commented Sep 17, 2016

Trott commented Sep 17, 2016

addaleax commented Sep 17, 2016

Trott commented Sep 17, 2016

Trott commented Sep 18, 2016

lrlna commented Sep 18, 2016

addaleax left a comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

jasnell commented Sep 20, 2016

lrlna commented Sep 20, 2016