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

jvelezpo · 2018-01-25T20:00:40Z

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

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.

mmarchini · 2018-01-26T00:50:40Z

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 · 2018-01-26T01:29:15Z

Awesome thanks @mmarchini i will add the change tomorrow 👍

richardlau · 2018-01-26T16:05:45Z

test/parallel/test-child-process-execfile.js

@@ -7,6 +7,7 @@ const { getSystemErrorName } = require('util');
 const fixtures = require('../common/fixtures');

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


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

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.

@richardlau

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 · 2018-01-29T08:29:02Z

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

jvelezpo · 2018-01-29T14:02:39Z

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.

mmarchini · 2018-01-29T14:35:19Z

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

BridgeAR

LGTM in general, just left a nit.

BridgeAR · 2018-01-31T00:37:33Z

test/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);


This is a perfect place for assert.ifError.

addaleax · 2018-02-04T15:44:25Z

Landed in 6f8de31 with @BridgeAR’s comment addressed

Thanks for the PR! 🎉

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>

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>

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-bot added the test Issues and PRs related to the tests. label Jan 25, 2018

test: Verify the shell option works properly on execFile

c1aa515

jasnell approved these changes Jan 26, 2018

View reviewed changes

richardlau reviewed Jan 26, 2018

View reviewed changes

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.

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:03

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

lpinca approved these changes Jan 28, 2018

View reviewed changes

mmarchini mentioned this pull request Jan 29, 2018

Flaky parallel/test-http-url.parse-path #18430

Closed

BridgeAR approved these changes Jan 31, 2018

View reviewed changes

BridgeAR 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 closed this Feb 4, 2018

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the land-on-v6.x label Mar 20, 2018

MylesBorins added the land-on-v8.x label Mar 20, 2018

MylesBorins mentioned this pull request Apr 13, 2018

v6.14.2 proposal #19996

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

jvelezpo deleted the verifyShellOptionWorksProperly branch September 6, 2018 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jvelezpo commented Jan 25, 2018 •

edited

mmarchini commented Jan 26, 2018

jvelezpo commented Jan 26, 2018

richardlau Jan 26, 2018

jvelezpo Jan 26, 2018

bzoz commented Jan 29, 2018

jvelezpo commented Jan 29, 2018

mmarchini commented Jan 29, 2018

BridgeAR left a comment

BridgeAR Jan 31, 2018 •

edited

addaleax commented Feb 4, 2018

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

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

Conversation

jvelezpo commented Jan 25, 2018 • edited

Checklist

Affected core subsystem(s)

mmarchini commented Jan 26, 2018

jvelezpo commented Jan 26, 2018

richardlau Jan 26, 2018

Choose a reason for hiding this comment

jvelezpo Jan 26, 2018

Choose a reason for hiding this comment

bzoz commented Jan 29, 2018

jvelezpo commented Jan 29, 2018

mmarchini commented Jan 29, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Jan 31, 2018 • edited

Choose a reason for hiding this comment

addaleax commented Feb 4, 2018

jvelezpo commented Jan 25, 2018 •

edited

BridgeAR Jan 31, 2018 •

edited