path: fix basename() regressions #6590
Conversation
mscdex
force-pushed
the
path-basename-fix-6587
branch
from
1979099
to
e3b43c0
Compare
|
@mscdex ... just for purposes of keeping track, was this related to the performance changes that landed in v5 or separate issue? (@nodejs/lts has been keeping an eye on the path changes and related regressions in case we decide to pull those in to v4 at some point) |
|
Unrelated build bot failure in CI (https://ci.nodejs.org/job/node-test-commit-arm/3146/nodes=armv8-ubuntu1404/console). LGTM otherwise. No specific comments on the commits. |
|
@jasnell Yes, this is fixing a regression from the |
| assert.equal(path.basename('aaa/bbb', 'bbb'), 'bbb'); | ||
| assert.equal(path.basename('aaa/bbb//', 'bbb'), 'bbb'); | ||
| assert.equal(path.basename('aaa/bbb', 'bb'), 'b'); | ||
| assert.equal(path.basename('aaa/bbb', 'b'), 'bb'); |
There was a problem hiding this comment.
maybe add a few tests for absolute paths with this case?
There was a problem hiding this comment.
I've now added absolute versions of these tests, is that what you meant?
There was a problem hiding this comment.
I was thinking maybe something more like path.basename('/aaa/bbb') since we have seen regressions for absolute vs relative paths. They may already be there though
There was a problem hiding this comment.
Ok, I've added that and a couple more.
There was a problem hiding this comment.
@evanlucas does this LGTY now?
mscdex
force-pushed
the
path-basename-fix-6587
branch
3 times, most recently
from
b274124
to
f1595e1
Compare
|
LGTM if CI is happy |
| assert.throws(path.win32.basename.bind(null, true), TypeError); | ||
| assert.throws(path.win32.basename.bind(null, 1), TypeError); | ||
| assert.throws(path.win32.basename.bind(null), TypeError); | ||
| assert.throws(path.win32.basename.bind(null, {}), TypeError); |
There was a problem hiding this comment.
Just wondering, why were these tests removed?
There was a problem hiding this comment.
The TypeError checks are already done later on in the test file
for all path functions.
|
One last CI before landing, just in case: https://ci.nodejs.org/job/node-test-pull-request/2684/ |
mscdex
force-pushed
the
path-basename-fix-6587
branch
from
f1595e1
to
f4a87ab
Compare
This commit fixes a regression in basename() for the case when a supplied extension name completely matches the resulting basename. Fixes: nodejs#6587 PR-URL: nodejs#6590 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
The TypeError checks are already done later on in the test file for all path functions. PR-URL: nodejs#6590 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: nodejs#6590 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
mscdex
force-pushed
the
path-basename-fix-6587
branch
from
f4a87ab
to
27549f6
Compare
The TypeError checks are already done later on in the test file for all path functions. PR-URL: #6590 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: #6590 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
The TypeError checks are already done later on in the test file for all path functions. PR-URL: #6590 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
PR-URL: #6590 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
Checklist
Affected core subsystem(s)
Description of change
This commit fixes a regression in basename() for the case when a supplied extension name completely matches the resulting basename.
Fixes: #6587