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: check that process.execPath is a realpath #9229

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

This test is only here to ensure consistent cross-platform behaviour.

This test is only here to ensure consistent cross-platform behaviour.
@addaleax addaleax added test Issues and PRs related to the tests. process Issues and PRs related to the process subsystem. labels Oct 21, 2016
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 21, 2016
assert.strictEqual(process.execPath, fs.realpathSync(process.execPath));

if (process.argv[2] === 'child') {
console.log(process.execPath);
Copy link
Member

Choose a reason for hiding this comment

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

can you please add a comment mentioning at the the console.log() output here is part of the test

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell done!

@addaleax
Copy link
Member Author

I’ve set the test to skip on Windows since I don’t know if it even makes sense there… if somebody in @nodejs/platform-windows has an opinion, feel free to voice that :)

if (common.isWindows) {
common.skip('symlinks are weird on windows');
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is the above check done? The test does not use symlinks. (If there is a good reason to skip the test, then the check for common.isWindows below can be removed.)

Copy link
Member

Choose a reason for hiding this comment

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

(Looks like this was meant for test-process-execpath.js and not test-child-process-exec-error.js?)

@Trott
Copy link
Member

Trott commented Oct 21, 2016

I’ve set the test to skip on Windows since I don’t know if it even makes sense there… if somebody in @nodejs/platform-windows has an opinion, feel free to voice that :)

Is it possible you accidentally added the skip to the wrong file? Looks that way to me...

@addaleax
Copy link
Member Author

Is it possible you accidentally added the skip to the wrong file? Looks that way to me...

That is totally possible. Updated! 😄

@joaocgreis
Copy link
Member

This works on Windows if you use 'symlinked-node.exe'. But the test fails and confirms that execPath is not a realpath.

@addaleax
Copy link
Member Author

This works on Windows if you use 'symlinked-node.exe'.

… silly me. Thanks for pointing that out!

But the test fails and confirms that execPath is not a realpath.

Sounds to me like that should be considered a bug, or at least an OS-specific difference that should be documented?

@joaocgreis
Copy link
Member

Looks like a bug to me, I'll take a look.

@addaleax If you want to land, include the Windows skip and I'll remove it with the fix.

Reviewing, the code looks good, but what's the motivation for this? Consistency sounds good, do we imply this somewhere in the docs or would it be just a nice to have?

@addaleax
Copy link
Member Author

If you want to land, include the Windows skip and I'll remove it with the fix.

Sounds good to me

Reviewing, the code looks good, but what's the motivation for this? Consistency sounds good, do we imply this somewhere in the docs or would it be just a nice to have?

It came from discussion around an npm change of mine, npm/npm#14374. We weren’t quite sure whether applying fs.realpathSync to process.execPath makes sense or not, so I got curious and wanted to see whether the behaviour is consistent (and if it is, try to get a test for that in here).

So, yes, it’s just nice to have, not something we guarantee in our docs right now.

@addaleax
Copy link
Member Author

Would anybody mind giving this a look? (@joaocgreis … was that an affirmative review?)

@bnoordhuis
Copy link
Member

New CI, the old one 404s: https://ci.nodejs.org/job/node-test-pull-request/4845/

@addaleax
Copy link
Member Author

addaleax commented Nov 16, 2016

Landed in 89469c8, thanks for reviewing!

@addaleax addaleax closed this Nov 16, 2016
@addaleax addaleax deleted the test-proc-execpath-realpath branch November 16, 2016 00:44
addaleax added a commit that referenced this pull request Nov 16, 2016
This test is only here to ensure consistent cross-platform behaviour.

PR-URL: #9229
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
addaleax added a commit that referenced this pull request Nov 22, 2016
This test is only here to ensure consistent cross-platform behaviour.

PR-URL: #9229
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@joaocgreis
Copy link
Member

(@addaleax I missed that last mention, sorry 😔)

Found this libuv PR: libuv/libuv#293 . Doesn't look like we have reasons here to push it farther than it went, so no real execPath on Windows for now.

@addaleax
Copy link
Member Author

addaleax commented Dec 6, 2016

@joaocgreis Thanks for followingup! I kind of wonder if we should just apply Node’s fs.realpath here… but then again maybe it’s best to just leave this alone. ;)

addaleax added a commit to addaleax/node that referenced this pull request Dec 8, 2016
This test is only here to ensure consistent cross-platform behaviour.

PR-URL: nodejs#9229
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
This test is only here to ensure consistent cross-platform behaviour.

PR-URL: #9229
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This test is only here to ensure consistent cross-platform behaviour.

PR-URL: #9229
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
This test is only here to ensure consistent cross-platform behaviour.

PR-URL: #9229
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants