path: fixing a test that breaks on some machines. by mike-kaufman · Pull Request #6067 · nodejs/node

mike-kaufman · 2016-04-05T22:13:50Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Caveats:

didn't run the unix tests, as this change doesn't impact them.
seeing two non-related test failures on windows. I haven't investigated these
- addons/load-long-path/test - seeing "Error: The data area passed to a system call is too small."
- test\parallel\test-tls-server-verify.js - seeing a timeout.

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().

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().

Fishrock123 · 2016-04-05T22:18:07Z

.gitignore

 ipch/
 *.sdf
 *.opensdf
+node.VC.opendb


I this this belongs in your own global gitignore. :)

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?

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

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.

would be *.opendb

Trott · 2016-04-05T22:47:47Z

@nodejs/testing @nodejs/platform-windows

Trott · 2016-04-05T22:48:35Z

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

Trott · 2016-04-05T23:10:47Z

Minus the .gitignore change, this LGTM

mike-kaufman · 2016-04-05T23:23:07Z

reverted change to .gitignore.

mike-kaufman · 2016-04-05T23:31:10Z

Opened a #6070 for the .gitignore change.

mike-kaufman · 2016-04-06T18:10:57Z

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 · 2016-04-06T19:46:48Z

@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 · 2016-04-06T19:54:08Z

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 · 2016-04-06T20:57:19Z

@mike-kaufman Usually the person merging it in.

joaocgreis · 2016-04-06T23:10:34Z

test/parallel/test-path.js

-               '\\\\?\\' + process.cwd().toLowerCase());
+  const currentDeviceLetter = path.parse(process.cwd()).root.substring(0, 2);
+  assert.equal(path.win32._makeLong(currentDeviceLetter).toLowerCase(),
+                 '\\\\?\\' + process.cwd().toLowerCase());


This last line was likely an oversight: I see only 2 added spaces, it was correct before.

joaocgreis · 2016-04-06T23:12:52Z

LGTM minus comment. Thanks for contributing @mike-kaufman !

mike-kaufman · 2016-04-07T00:22:53Z

Fixed indentation.

benjamingr · 2016-04-07T11:22:30Z

LGTM.
CI: https://ci.nodejs.org/job/node-test-pull-request/2195/

jasnell · 2016-04-07T17:08:22Z

LGTM

benjamingr · 2016-04-08T20:30:01Z

Test failures look unrelated but I'm no 100% sure - @nodejs/build

jbergstroem · 2016-04-08T23:39:56Z

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 <rtrott@gmail.com> Reviewed-By: Joao Reis <reis@janeasystems.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Trott · 2016-04-08T23:46:08Z

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 <rtrott@gmail.com> Reviewed-By: Joao Reis <reis@janeasystems.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu>

Fishrock123 added the path Issues and PRs related to the path subsystem. label Apr 5, 2016

Fishrock123 reviewed Apr 5, 2016
View reviewed changes

Trott added windows Issues and PRs related to the Windows platform. test Issues and PRs related to the tests. labels Apr 5, 2016

reverting changes to include node.VC.opendb in .gitignore file

146b83e

mike-kaufman mentioned this pull request Apr 6, 2016

async_wrap,lib: Firing async-wrap callbacks for next-tick callbacks #6082

Closed

4 tasks

joaocgreis reviewed Apr 6, 2016
View reviewed changes

fixing indentation per PR comments

2508595

jasnell added the lts-watch-v4.x label Apr 7, 2016

addaleax mentioned this pull request Apr 8, 2016

test: test-path fails on Windows? #6119

Closed

Trott closed this Apr 8, 2016

mike-kaufman deleted the mkaufman-test-fix-for-path branch April 13, 2016 21:53

MylesBorins mentioned this pull request Apr 20, 2016

V5.10.2 proposal #6311

Closed

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels May 3, 2016

This was referenced Oct 3, 2017

implement transparent bounds. Ticket #6067 imdone/node#296

Open

implement transparent bounds. Ticket #6067 imdone/node#636

Open

Uh oh!

Conversation

mike-kaufman commented Apr 5, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

Fishrock123 Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

mike-kaufman Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

mike-kaufman Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

srl295 Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Apr 5, 2016

Uh oh!

Trott commented Apr 5, 2016

Uh oh!

Trott commented Apr 5, 2016

Uh oh!

mike-kaufman commented Apr 5, 2016

Uh oh!

mike-kaufman commented Apr 5, 2016

Uh oh!

mike-kaufman commented Apr 6, 2016

Uh oh!

Trott commented Apr 6, 2016

Uh oh!

mike-kaufman commented Apr 6, 2016

Uh oh!

Trott commented Apr 6, 2016

Uh oh!

joaocgreis Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

joaocgreis commented Apr 6, 2016

Uh oh!

mike-kaufman commented Apr 7, 2016

Uh oh!

benjamingr commented Apr 7, 2016

Uh oh!

jasnell commented Apr 7, 2016

Uh oh!

benjamingr commented Apr 8, 2016

Uh oh!

jbergstroem commented Apr 8, 2016

Uh oh!

Trott commented Apr 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants