path,win: fix bug in resolve and normalize #55623
Conversation
nodejs-github-bot
added
c++
| assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo\\'); | ||
| assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo'); |
There was a problem hiding this comment.
Is stripping the / okay? We wouldn't want to repeat that happened with npm 😅
There was a problem hiding this comment.
I think it should be OK since this PR doesn't change anything other than this particular path format. But yes, it would be useful to see the citgm test results.
There was a problem hiding this comment.
Yes, a CITGM would be nice. I've applied the labels. If possible, could you also run npm's test suite? The CITGM doesn't cover all of it, and IMO we should be extra cautious JIC.
Although, it seems like it should be fine. Just want to confirm
There was a problem hiding this comment.
I ran the test in the npm repo locally and didn't encounter any problems.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55623 +/- ##
=======================================
Coverage 88.43% 88.43%
=======================================
Files 654 654
Lines 187698 187729 +31
Branches 36126 36135 +9
=======================================
+ Hits 165986 166024 +38
Misses 14953 14953
+ Partials 6759 6752 -7
|
StefanStojanovic
added
the
request-ci
github-actions
bot
removed
the
request-ci
|
CITGM ( SUCCESS: No new failures in 3501 compared to 3499 🎉 |
aduh95
added
baking-for-lts
This PR fixes resolving device paths like
\\.\PHYSICALDRIVE.The
resolveandnormalizefunctions were adding a trailing backslash, considering that this device path was a UNC path. However, device paths are different than UNC paths.In addition to fixing this issue, the previous attempt also fixed other inconsistencies. So, this PR only fixes the issue itself.
Previous PR: #54224
Fixes: #54025