path: fixing a test that breaks on some machines.#6067
Uh oh!
There was an error while loading. Please reload this page.
Conversation
A win32-only test was verifying that path.win32._makeLong('C:') would return the current working directory. This would only work if current working directory was also on the C: device. Fix is to grab the device letter for current working directory, and pass that to _makeLong().
.gitignore Outdated
| ipch/ | ||
| *.sdf | ||
| *.opensdf | ||
| node.VC.opendb |
There was a problem hiding this comment.
I this this belongs in your own global gitignore. :)
There was a problem hiding this comment.
Sure, I can remove this, but I believe this is an artifact of vs 2015, so anyone using this IDE will need to add this to their .gitignore. Is there precedent here?
There was a problem hiding this comment.
And FWIW, this seems to be a common thing to add to .gitignore files. e.g.,http://stackoverflow.com/questions/34975423/visual-studio-2015-git-error-opensomefile-vc-opendb-permission-denied-fa
There was a problem hiding this comment.
No opinion from me on whether or not it should be added, but if it should, then let's do it in a separate PR. It's unrelated to the main change here.
Trott commented Apr 5, 2016
@nodejs/testing @nodejs/platform-windows |
Trott added windows Trott commented Apr 5, 2016
Trott commented Apr 5, 2016
Minus the |
mike-kaufman commented Apr 5, 2016
reverted change to .gitignore. |
mike-kaufman commented Apr 5, 2016
Opened a #6070 for the .gitignore change. |
mike-kaufman commented Apr 6, 2016
So, what's the process here? I should squash commits & update the commit message to reflect reviewers & reference the PR, and then someone can merge in? Or is there something else I need to do? |
Trott commented Apr 6, 2016
@mike-kaufman Typically, stuff gets left open for 48 hours (longer if there's a weekend involved) before landing. So I think that's where it's at right now. Squashing helps out a little bit, but if you don't do it, the person landing it will do it, so no big deal. |
mike-kaufman commented Apr 6, 2016
Thanks @Trott. So who adds in the reviewer details & PR link to the commit message. Is that the person submitting the PR, or the person merging it in? |
Trott commented Apr 6, 2016
@mike-kaufman Usually the person merging it in. |
test/parallel/test-path.js Outdated
| '\\\\?\\'+process.cwd().toLowerCase()); | ||
| constcurrentDeviceLetter=path.parse(process.cwd()).root.substring(0,2); | ||
| assert.equal(path.win32._makeLong(currentDeviceLetter).toLowerCase(), | ||
| '\\\\?\\'+process.cwd().toLowerCase()); |
There was a problem hiding this comment.
This last line was likely an oversight: I see only 2 added spaces, it was correct before.
joaocgreis commented Apr 6, 2016
LGTM minus comment. Thanks for contributing @mike-kaufman ! |
mike-kaufman commented Apr 7, 2016
Fixed indentation. |
benjamingr commented Apr 7, 2016
jasnell commented Apr 7, 2016
LGTM |
benjamingr commented Apr 8, 2016
Test failures look unrelated but I'm no 100% sure - @nodejs/build |
jbergstroem commented Apr 8, 2016
Looks unrelated indeed. LGTM. |
A win32-only test was verifying that path.win32._makeLong('C:') would return the current working directory. This would only work if current working directory was also on the C: device. Fix is to grab the device letter for current working directory, and pass that to _makeLong(). PR-URL: nodejs#6067 Reviewed-By: Trott - Rich Trott <[email protected]> Reviewed-By: Joao Reis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]>Trott commented Apr 8, 2016
Landed in 1384de2 Thanks for the contribution, @mike-kaufman! |
A win32-only test was verifying that path.win32._makeLong('C:') would return the current working directory. This would only work if current working directory was also on the C: device. Fix is to grab the device letter for current working directory, and pass that to _makeLong(). PR-URL: #6067 Reviewed-By: Trott - Rich Trott <[email protected]> Reviewed-By: Joao Reis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]>A win32-only test was verifying that path.win32._makeLong('C:') would return the current working directory. This would only work if current working directory was also on the C: device. Fix is to grab the device letter for current working directory, and pass that to _makeLong(). PR-URL: #6067 Reviewed-By: Trott - Rich Trott <[email protected]> Reviewed-By: Joao Reis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
A win32-only test was verifying that path.win32._makeLong('C:') would return the current working directory. This would only work if current working directory was also on the C: device. Fix is to grab the device letter for current working directory, and pass that to _makeLong(). PR-URL: #6067 Reviewed-By: Trott - Rich Trott <[email protected]> Reviewed-By: Joao Reis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
A win32-only test was verifying that path.win32._makeLong('C:') would return the current working directory. This would only work if current working directory was also on the C: device. Fix is to grab the device letter for current working directory, and pass that to _makeLong(). PR-URL: #6067 Reviewed-By: Trott - Rich Trott <[email protected]> Reviewed-By: Joao Reis <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]>9 participants
Pull Request check-list
Please make sure to review and check all of these items:
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Caveats:
Affected core subsystem(s)
path
Description of change
A win32-only test was verifying that path.win32._makeLong('C:')
would return the current working directory. This would only work if
current working directory was also on the C: device. Fix is to grab
the device letter for current working directory, and pass that to
_makeLong().