Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

mcornac
Copy link
Contributor

@mcornac mcornac commented Oct 30, 2015

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
Copy link

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

@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Oct 30, 2015
@mscdex mscdex added repl Issues and PRs related to the REPL subsystem. windows Issues and PRs related to the Windows platform. labels Oct 30, 2015
@john-yan
Copy link

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

@jasnell
Copy link
Member

jasnell commented Oct 31, 2015

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

@jasnell
Copy link
Member

jasnell commented Oct 31, 2015

PR LGTM

@john-yan
Copy link

@jasnell Thanks for letting me know.

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, '\\\\');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use common.isWindows

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks

@thefourtheye
Copy link
Contributor

Windows accepts slash separated paths as well, right?

@mcornac
Copy link
Contributor Author

mcornac commented Nov 2, 2015

@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
Copy link
Contributor Author

mcornac commented Nov 10, 2015

Can someone give a suggestion or merge this in please?

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, '\\\\');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split this across two lines please.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2015

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
Copy link
Contributor Author

mcornac commented Nov 10, 2015

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

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thanks!

@bnoordhuis
Copy link
Member

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

@cjihrig
Copy link
Contributor

cjihrig commented Nov 11, 2015

@john-yan
Copy link

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

@mcornac
Copy link
Contributor Author

mcornac commented Nov 12, 2015

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
Copy link
Contributor

cjihrig commented Nov 12, 2015

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

@cjihrig
Copy link
Contributor

cjihrig commented Nov 13, 2015

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

@joaocgreis
Copy link
Member

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

@cjihrig
Copy link
Contributor

cjihrig commented Nov 14, 2015

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
Copy link
Member

jasnell commented Nov 15, 2015

@jasnell
Copy link
Member

jasnell commented Nov 15, 2015

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
Copy link
Contributor

cjihrig commented Nov 15, 2015

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

jasnell pushed a commit that referenced this pull request Nov 16, 2015
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
Copy link
Member

jasnell commented Nov 16, 2015

Landed in c0bac95

@jasnell jasnell closed this Nov 16, 2015
MylesBorins pushed a commit that referenced this pull request Nov 16, 2015
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
Copy link
Contributor

landed in v4.x-staging as c10f17f

Fishrock123 pushed a commit that referenced this pull request Nov 17, 2015
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>
rvagg pushed a commit that referenced this pull request Dec 4, 2015
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 jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
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 pushed a commit that referenced this pull request Dec 23, 2015
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants