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: change equal to strictEqual in path tests #8628

Closed
wants to merge 2 commits into from

Conversation

lrlna
Copy link
Contributor

@lrlna lrlna commented Sep 17, 2016

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.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2016
@MylesBorins
Copy link
Contributor

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM

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

// 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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax ofc!

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

misaligned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

'\\\\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');
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and done 🎉

@addaleax
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Sep 17, 2016

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
Copy link
Member

Trott commented Sep 17, 2016

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
Copy link
Member

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

@Trott
Copy link
Member

Trott commented Sep 17, 2016

(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
Copy link
Member

Trott commented Sep 18, 2016

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

@lrlna
Copy link
Contributor Author

lrlna commented Sep 18, 2016

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Trott added a commit to Trott/io.js that referenced this pull request Sep 20, 2016
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>
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome job!

jasnell pushed a commit that referenced this pull request Sep 20, 2016
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
Copy link
Member

jasnell commented Sep 20, 2016

Landed in daa0f90! thank you!

@jasnell jasnell closed this Sep 20, 2016
@lrlna
Copy link
Contributor Author

lrlna commented Sep 20, 2016

omg YAY! 🎉

Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
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>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants