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: allow passing args to executable #5376

Closed
wants to merge 2 commits into from

Conversation

stefanmb
Copy link
Contributor

To set up CI jobs for #5181 there has to be a way to pass an argument to the Node executable at test time. The makefile supports the TEST_CI_ARGS environment variable, but the test harness (test.py) provides no facility to further pass the argument to the executable under test.

There is a "--special-command" argument, but it is only able to suffix or prefix the existing command line in the format [node, test] so it is insufficient to produce commands in the form [node, node_args, test]. In this PR I've introduced an extra argument to the test harness called --node-args which can be used to pass commands verbatim.

Some support for this feature already existed in test/init.py in the form of the "additional" argument, however none of the individual test modules' testcfg.py make use of it, so in the interest of minimal footprint I've opted to directly set this value form test.py, instead of passing it through as a constructor argument.

There is an additional complication in "test-cluster-debug-port.js" which for reasons that are unclear refuses to run if any arguments are passed to the process. I've simply relaxed this requirement.

Add --node-args option that will pass arguments.
@stefanmb
Copy link
Contributor Author

@indutny @mhdawson I'd appreciate any comments regarding the impact on #5181, thanks.

@mscdex mscdex added the test Issues and PRs related to the tests. label Feb 23, 2016
@bnoordhuis
Copy link
Member

LGTM. Maybe break up the long line. CI: https://ci.nodejs.org/job/node-test-pull-request/1731/

@Fishrock123
Copy link
Contributor

Can't you already pass arguments to node by having a comment at the top of the file ala https://github.com/nodejs/node/blob/master/test/parallel/test-repl-persistent-history.js#L3 ? Am I missing something?

@stefanmb
Copy link
Contributor Author

@Fishrock123 That facility is provided in test/testpy/init.py by statically scanning the source of each file, which is similar to what I'm proposing here, but not identical. The requirement here is to be able to dynamically choose whether to pass a flag to every test, or not, at runtime. Specifically, for FIPS builds (which now default to FIPS off) the test suites have to be run twice, once with FIPS off, and once with FIPS on. This can be achieved by passing --enable-fips to the Node executable under test, using the "additional_flags" variable already present in test/testpy/init.py

Further details are included in #5181.

@Fishrock123
Copy link
Contributor

Ah, makes sense. 👌

@stefanmb
Copy link
Contributor Author

@bnoordhuis I split the function signature as indicated, thanks.

@Trott
Copy link
Member

Trott commented Feb 23, 2016

As much as I dislike adding more stuff to test.py, it's hard to argue with the need here. LGTM

@mhdawson
Copy link
Member

LGTM. going to land

@mhdawson mhdawson self-assigned this Feb 25, 2016
@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

Only one failure in CI which is unrelated to this change - reopened #5184 and added link to failure

mhdawson pushed a commit that referenced this pull request Feb 25, 2016
Add --node-args option that will pass arguments.

PR-URL: #5376
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

Landed as 23a584d

@stefanmb
Copy link
Contributor Author

This PR introduced a regression that is fixed in #5446.

rvagg pushed a commit that referenced this pull request Feb 27, 2016
Add --node-args option that will pass arguments.

PR-URL: #5376
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 27, 2016
Add --node-args option that will pass arguments.

PR-URL: #5376
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Mar 1, 2016
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants