test: fix path to module for repl test on Windows #3608

mcornac · 2015-10-30T17:12:19Z

Use path join to construct the path instead of concatenating strings.
Replace backslash with double backslash so that they are escaped
correctly in the string passed to REPL.

john-yan · 2015-10-30T17:20:51Z

This is to fix the following message on windows:

not ok 8 test.js
#
#assert.js:89
#  throw new assert.AssertionError({
#  ^
#AssertionError: false == true
#    at process.<anonymous> (D:\build\jenkins-new\workspace\ibmnode-v4-test-win\SUITE\node\arch\x64\label\bvt\os\win\node\test\addons\repl-domain-abort\test.js:11:3)
#    at emitOne (events.js:82:20)
#    at process.emit (events.js:169:7)
  ---
  duration_ms: 0.499
  ...

john-yan · 2015-10-31T14:44:03Z

Can someone add a 'lts-v4.x-watch' tag please?

jasnell · 2015-10-31T14:57:53Z

@john-yan ... added... likely won't go in until after v4.2.2 tho

jasnell · 2015-10-31T14:58:06Z

PR LGTM

john-yan · 2015-10-31T16:48:00Z

@jasnell Thanks for letting me know.

thefourtheye · 2015-11-02T03:12:10Z

test/addons/repl-domain-abort/test.js

-var buildPath = __dirname + '/build/' + buildType + '/binding';
+var buildPath = path.join(__dirname, 'build', buildType, 'binding');
+// On Windows, escape backslashes in the path before passing it to REPL.
+if (os.platform() == 'win32') buildPath = buildPath.replace(/\\/g, '\\\\');


Please use common.isWindows

Updated, thanks

thefourtheye · 2015-11-02T06:36:12Z

Windows accepts slash separated paths as well, right?

mcornac · 2015-11-02T16:22:59Z

@thefourtheye
I tried replacing with forward slash now that I have access to the Windows machine again using:
if (common.isWindows) buildPath = buildPath.replace(//g, '/');
The test passes like this as well.

The reason the original test did not pass is that it has a mixture of (not escaped) backslashes from __dirname with hard coded forward slashes. I.e. "D:\node-1\test\addons\repl-domain-abort/build/Release/binding".

Should I make the change from '\' to '/' ?

mcornac · 2015-11-10T21:39:06Z

Can someone give a suggestion or merge this in please?

cjihrig · 2015-11-10T21:42:28Z

test/addons/repl-domain-abort/test.js

-var buildPath = __dirname + '/build/' + buildType + '/binding';
+var buildPath = path.join(__dirname, 'build', buildType, 'binding');
+// On Windows, escape backslashes in the path before passing it to REPL.
+if (common.isWindows) buildPath = buildPath.replace(/\\/g, '\\\\');


Can you split this across two lines please.

LGTM although I would have replaced them with forward slashes. It should work the same and is arguably easier to read / less ambiguous.

cjihrig · 2015-11-10T21:44:30Z

Can't really merge this just yet, as the CI is under repairs, but LGTM.

Use path join to construct the path instead of concatenating strings. Replace backslash with double backslash so that they are escaped correctly in the string passed to REPL.

mcornac · 2015-11-10T21:57:54Z

I split the if statement to two lines and replaced with forward slash.

bnoordhuis · 2015-11-10T22:02:43Z

test/addons/repl-domain-abort/test.js

 var buildType = process.config.target_defaults.default_configuration;
-var buildPath = __dirname + '/build/' + buildType + '/binding';
+var buildPath = path.join(__dirname, 'build', buildType, 'binding');
+// On Windows, escape backslashes in the path before passing it to REPL.


Comment is wrong now. Don't worry, I'll update it when I land it. :-)

Oops, thanks!

bnoordhuis · 2015-11-10T22:03:39Z

LGTM with a comment. CI: https://ci.nodejs.org/job/node-test-pull-request/694/

cjihrig · 2015-11-11T17:36:51Z

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

john-yan · 2015-11-12T00:40:35Z

@thinkingdust Could you please take a look at the failures on the CI and see if any failure are related.

mcornac · 2015-11-12T04:00:41Z

I took a look. I'm not sure if I'm parsing the output right but I can't see anything related. Many of them look like this one on Windows with a Java exception.
https://ci.nodejs.org/job/node-test-binary-windows/RUN_SUBSET=1,VS_VERSION=vs2015,label=win10/249/console
Caused by: java.io.IOException: Connection timed out

cjihrig · 2015-11-12T04:11:17Z

Yea, those are issues with the CI itself. Will try running it again tomorrow, if no one else does by then.

cjihrig · 2015-11-13T16:14:27Z

Let's see how the CI is feeling today: https://ci.nodejs.org/job/node-test-pull-request/723/

joaocgreis · 2015-11-14T12:06:40Z

@cjihrig CI was bad, but should be better now: https://ci.nodejs.org/job/node-test-pull-request/729/

cjihrig · 2015-11-14T13:06:11Z

Thanks @joaocgreis.

This is failing for me locally on OS X:

$ ./node test/addons/repl-domain-abort/test.js 

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at process.<anonymous> (/Users/cjihrig/iojs/node/test/addons/repl-domain-abort/test.js:15:3)
    at emitOne (events.js:82:20)
    at process.emit (events.js:169:7)

jasnell · 2015-11-15T00:59:15Z

New CI Run: https://ci.nodejs.org/job/node-test-pull-request/737/

jasnell · 2015-11-15T19:13:19Z

CI is mostly green except for the known CI failures. @cjihrig it appears to be running fine for me on OS X and is green on OS X in CI (https://ci.nodejs.org/job/node-test-commit-osx/1186/). Can you run the test again and verify if you're still seeing errors?

cjihrig · 2015-11-15T22:09:53Z

If the CI is happy, don't let me hold this back. LGTM

Use path join to construct the path instead of concatenating strings. Replace backslash with double backslash so that they are escaped correctly in the string passed to REPL. PR-URL: #3608 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell · 2015-11-16T00:34:01Z

Landed in c0bac95

Use path join to construct the path instead of concatenating strings. Replace backslash with double backslash so that they are escaped correctly in the string passed to REPL. PR-URL: #3608 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2015-11-16T23:23:52Z

landed in v4.x-staging as c10f17f

Use path join to construct the path instead of concatenating strings. Replace backslash with double backslash so that they are escaped correctly in the string passed to REPL. PR-URL: #3608 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Fishrock123 added the test Issues and PRs related to the tests. label Oct 30, 2015

mscdex added repl Issues and PRs related to the REPL subsystem. windows Issues and PRs related to the Windows platform. labels Oct 30, 2015

jasnell added the lts-watch-v4.x label Oct 31, 2015

thefourtheye reviewed Nov 2, 2015
View reviewed changes

cjihrig reviewed Nov 10, 2015
View reviewed changes

test: fix path to module for repl test on Windows

a2b7bf9

Use path join to construct the path instead of concatenating strings. Replace backslash with double backslash so that they are escaped correctly in the string passed to REPL.

bnoordhuis reviewed Nov 10, 2015
View reviewed changes

jasnell closed this Nov 16, 2015

MylesBorins added lts-landed-on-v4.x and removed lts-watch-v4.x labels Nov 16, 2015

rvagg mentioned this pull request Nov 17, 2015

v4.2.2+1 commit candidates nodejs/Release#57

Closed

MylesBorins added land-on-v4.x and removed lts-landed-on-v4.x labels Dec 17, 2015

jasnell mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

B020239 mentioned this pull request Mar 9, 2024

[Snyk] Upgrade esbuild from 0.19.5 to 0.20.0 B020239/node#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix path to module for repl test on Windows #3608

test: fix path to module for repl test on Windows #3608

mcornac commented Oct 30, 2015

john-yan commented Oct 30, 2015

john-yan commented Oct 31, 2015

jasnell commented Oct 31, 2015

jasnell commented Oct 31, 2015

john-yan commented Oct 31, 2015

thefourtheye Nov 2, 2015

mcornac Nov 2, 2015

thefourtheye commented Nov 2, 2015

mcornac commented Nov 2, 2015

mcornac commented Nov 10, 2015

cjihrig Nov 10, 2015

bnoordhuis Nov 10, 2015

cjihrig commented Nov 10, 2015

mcornac commented Nov 10, 2015

bnoordhuis Nov 10, 2015

mcornac Nov 10, 2015

bnoordhuis commented Nov 10, 2015

cjihrig commented Nov 11, 2015

john-yan commented Nov 12, 2015

mcornac commented Nov 12, 2015

cjihrig commented Nov 12, 2015

cjihrig commented Nov 13, 2015

joaocgreis commented Nov 14, 2015

cjihrig commented Nov 14, 2015

jasnell commented Nov 15, 2015

jasnell commented Nov 15, 2015

cjihrig commented Nov 15, 2015

jasnell commented Nov 16, 2015

MylesBorins commented Nov 16, 2015

test: fix path to module for repl test on Windows #3608

test: fix path to module for repl test on Windows #3608

Conversation

mcornac commented Oct 30, 2015

john-yan commented Oct 30, 2015

john-yan commented Oct 31, 2015

jasnell commented Oct 31, 2015

jasnell commented Oct 31, 2015

john-yan commented Oct 31, 2015

thefourtheye Nov 2, 2015

Choose a reason for hiding this comment

mcornac Nov 2, 2015

Choose a reason for hiding this comment

thefourtheye commented Nov 2, 2015

mcornac commented Nov 2, 2015

mcornac commented Nov 10, 2015

cjihrig Nov 10, 2015

Choose a reason for hiding this comment

bnoordhuis Nov 10, 2015

Choose a reason for hiding this comment

cjihrig commented Nov 10, 2015

mcornac commented Nov 10, 2015

bnoordhuis Nov 10, 2015

Choose a reason for hiding this comment

mcornac Nov 10, 2015

Choose a reason for hiding this comment

bnoordhuis commented Nov 10, 2015

cjihrig commented Nov 11, 2015

john-yan commented Nov 12, 2015

mcornac commented Nov 12, 2015

cjihrig commented Nov 12, 2015

cjihrig commented Nov 13, 2015

joaocgreis commented Nov 14, 2015

cjihrig commented Nov 14, 2015

jasnell commented Nov 15, 2015

jasnell commented Nov 15, 2015

cjihrig commented Nov 15, 2015

jasnell commented Nov 16, 2015

MylesBorins commented Nov 16, 2015