Skip to content

test: Verify the shell option works properly on execFile - #18384

Closed
jvelezpo wants to merge 3 commits into
nodejs:masterfrom
jvelezpo:verifyShellOptionWorksProperly
Closed

test: Verify the shell option works properly on execFile#18384
jvelezpo wants to merge 3 commits into
nodejs:masterfrom
jvelezpo:verifyShellOptionWorksProperly

Conversation

@jvelezpo

@jvelezpojvelezpo commented Jan 25, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.

This are test for this #18199
documented here #18237

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

test

@jvelezpo
test: Verify the shell option works properly on execFile
ce61105
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Jan 25, 2018
@mmarchini

mmarchini commented Jan 26, 2018

Copy link
Copy Markdown
Contributor

Nice contribution, thanks for working on this!

Maybe we could use process.execPath as the executed command instead of ls, to avoid platform-dependency? I'm not sure if there's a platform which doesn't have ls though (maybe some older Windows version).

@jvelezpo

jvelezpo commented Jan 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Awesome thanks @mmarchini i will add the change tomorrow 👍

richardlau
richardlau reviewed Jan 26, 2018
View reviewed changes
Comment threadtest/parallel/test-child-process-execfile.js Outdated
const fixtures = require('../common/fixtures');

const fixture = fixtures.path('exit.js');
const execOpts = { encoding: 'utf8', shell: process.env.SHELL };

@richardlaurichardlauJan 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

process.env.SHELL is likely to not be set on Windows so this will end up being undefined.

@jvelezpojvelezpoJan 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You are correct mr @richardlau, i tested it on another pc running windows and process.env.SHELL was undefined.
Thanks for feedback I appreciate it.

@jvelezpo
test: Verify the shell option works properly on execFile
c85545b
from @richardlau `process.env.SHELL is likely to not be set on Windows
so this will end up being undefined.`
This commit prevents this from happening.
@bzoz

bzoz commented Jan 29, 2018

Copy link
Copy Markdown
Contributor

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

@jvelezpo

jvelezpo commented Jan 29, 2018

Copy link
Copy Markdown
ContributorAuthor

can anyone please explain me why this 2 checks in the tests are failing??

here is an SS of the tests ran on my local.

image

@mmarchini

mmarchini commented Jan 29, 2018

Copy link
Copy Markdown
Contributor

@jvelezpo don't worry, those errors are unrelated to this PR

@mmarchinimmarchini mentioned this pull request Jan 29, 2018
Closed
BridgeAR
BridgeAR approved these changes Jan 31, 2018
View reviewed changes

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM in general, just left a nit.

Comment threadtest/parallel/test-child-process-execfile.js
{
// Verify the shell option works properly
execFile(process.execPath, [fixture, 0], execOpts, common.mustCall((err) => {
assert.strictEqual(err, null);

@BridgeARBridgeARJan 31, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a perfect place for assert.ifError.

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2018
@addaleax

addaleax commented Feb 4, 2018

Copy link
Copy Markdown
Member

Landed in 6f8de31 with @BridgeAR’s comment addressed

Thanks for the PR! 🎉

@addaleaxaddaleax closed this Feb 4, 2018
@addaleaxaddaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018
addaleax pushed a commit that referenced this pull request Feb 4, 2018
@jvelezpo@addaleax
test: verify the shell option works properly on execFile
6f8de31
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
aecc551
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
b534b69
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
52aeb2a
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
aaa896c
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 20, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
279ea15
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
de312a3
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
6ef1fa3
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
b2a2a55
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2018
@jvelezpo@MylesBorins
test: verify the shell option works properly on execFile
e55e08c
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: #18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorinsMylesBorins mentioned this pull request Apr 13, 2018
Merged
@MylesBorinsMylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@jvelezpo@MayaLekova
test: verify the shell option works properly on execFile
4bc3a53
Useful for executing in a shell because it accepts arguments as
an array instead of a string as exec does.
Depending on the circumstances,
that can prove to be useful if the arguments are already prepared.
PR-URL: nodejs#18384
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jvelezpo
jvelezpo deleted the verifyShellOptionWorksProperly branch September 6, 2018 13:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlaurichardlaurichardlau left review comments
@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@BridgeARBridgeARBridgeAR approved these changes

Assignees

No one assigned

Labels

testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@jvelezpo@mmarchini@bzoz@addaleax@jasnell@lpinca@richardlau@BridgeAR@MylesBorins@nodejs-github-bot