test: do not require flags when executing a file #26858

BridgeAR · 2019-03-22T15:46:49Z

Instead of throwing an error in case a flag is missing, just start
a child_process that includes all flags. This improves the situation
for all developers in case they want to just plainly run a test.

Co-authored-by: Rich Trott [email protected]

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Instead of throwing an error in case a flag is missing, just start a `child_process` that includes all flags. This improves the situation for all developers in case they want to just plainly run a test. Co-authored-by: Rich Trott <[email protected]>

nodejs-github-bot · 2019-03-22T15:46:51Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/3008/pipeline

test/common/index.js

sam-github · 2019-03-22T17:54:04Z

I often run tests in a little harness that reruns them if the test code changes, or the node executable rebuilds, and run them directly with ./out/Release/node (not tools/test.py, its waaaayyyy slow), so this would be quite helpful to me, thanks.

refack · 2019-03-22T18:02:57Z

I'm ambivalent about this change. On the one hand it's convenient. On the other it's "magic" and definatly not K.I.S.S...

(not tools/test.py, its waaaayyyy slow)

Maybe put those lines in tools/test.js and then you'll be able to run:

node -r tools/test foo/test-barbaz.js

Make it a bit more picky and you can add it to your NODE_OPTIONS...

BridgeAR · 2019-03-24T23:08:00Z

@sam-github thanks for the review, I updated it similar to your suggestion. PTAL.

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

@refack

it's "magic" and definatly not K.I.S.S...

Everything around our flags is currently magical. This just makes it a lot more convenient to run the tests and it is especially significantly friendlier to newcomers.

I also do not see that it makes the code more complex than it currently is, so is there any actual downside?

@nodejs/testing PTAL

tniessen · 2019-03-25T14:43:48Z

I often run gdb --args ./node_g test/parallel/test-something. Generally, this change sounds like a good idea, but it might make debugging confusing... I think.

nodejs-github-bot · 2019-03-25T15:28:45Z

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

sam-github · 2019-03-25T16:47:02Z

Our javascript unit tests files are intended to be runnable standalone, because depency of tests on external runners introduces complexity and difficulty in debugging.

Usually they are runnable standalone, except sometimes they are only runnable with cooperation from a giant, undocumented, and difficult to understand glob of python (I say that as someone who has a half dozen times tried to figure out what some of its effectively undocumented command line arguments mean by reading the test.py source, unsucessfully). test.py seems to be the real black magic here.

I like this pure-node approach so much I'd be in favour of the dependency of our javascript tests on the python runner being stripped out, and this way of running the tests becoming the default.

As a developer friendly optimization, its great that the re-spawn of a child can be evaded when running with debuggers (gdb or inspector) by directly specifying the CLI options. This PR preserves that quality. 👍

tniessen · 2019-03-25T17:31:45Z

As a developer friendly optimization, its great that the re-spawn of a child can be evaded when running with debuggers (gdb or inspector) by directly specifying the CLI options.

My two cents: When debugging native code, I often use GDB with one of our tests and if I forget to specify flags, the test suite tells me. With this patch, when I forget to specify flags, I end up debugging the wrong process, and I don't think I'd notice that immediately (apart from "my breakpoints don't work"). Just a small concern, apart from that, it sounds like a great idea!

BridgeAR · 2019-03-26T15:48:59Z

@tniessen hm... I guess this is not a frequent use case for most users but it's definitely a very valid point. I guess we could have a blacklist for some environment variables or arguments that are used with GDB and the inspector. If anyone has a better idea, please let me know!

BridgeAR · 2019-03-26T16:00:44Z

I removed author ready until we have a solution for GDB.

refack · 2019-03-26T16:10:14Z

(I say that as someone who has a half dozen times tried to figure out what some of its effectively undocumented command line arguments mean by reading the test.py source, unsucessfully). test.py seems to be the real black magic here.

Challenge accepted 🔧
(I'd consider it more of a "black hole" then "black magic")

when I forget to specify flags, I end up debugging the wrong process

That's what I consider "magic". When the code tries to make non-trivial decisions for you...

IMO the flag parsing could go into the stdlib, similar to --exp█████████nals we could add a --run-test cli arg that triggers //FLAGS parsing in-proc.

BridgeAR · 2019-03-27T09:50:32Z

@refack I don't think that parsing our test flags justifies adding another flag and it also contradicts the actual intention here: to reduce the overhead. All new developers would have to learn about the CLI flag and would always have to use it, that does not sound very user friendly.

@tniessen I thought about it again and what about a very simple solution: adding a small user notification that the test is run with flags. That would a) remove the "magic" part as it's now clear what happens and b) everyone who forgot to add some flags while debugging native code would also realize that it's a different process.

tniessen · 2019-03-27T10:50:21Z

@BridgeAR I was about to propose a process warning but thought that might interfere with some tests. I like your idea!

The hasCrypto check should be checked before anything else to prevent overhead in case it's not falsy. Otherwise the file would be read without any further benefit.

BridgeAR · 2019-03-27T12:15:55Z

I added a comment. PTAL.

nodejs-github-bot · 2019-03-27T16:01:26Z

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

nodejs-github-bot · 2019-03-28T11:40:40Z

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

test/common/index.js

nodejs-github-bot · 2019-03-28T23:44:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22001/ ✔️

Instead of throwing an error in case a flag is missing, just start a `child_process` that includes all flags. This improves the situation for all developers in case they want to just plainly run a test. Co-authored-by: Rich Trott <[email protected]> PR-URL: nodejs#26858 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

The hasCrypto check should be checked before anything else to prevent overhead in case it's not falsy. Otherwise the file would be read without any further benefit. PR-URL: nodejs#26858 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

BridgeAR · 2019-03-29T10:00:42Z

Landed in f2dc99c and 6e5ffc4 🎉

Thanks for the reviews!

Instead of throwing an error in case a flag is missing, just start a `child_process` that includes all flags. This improves the situation for all developers in case they want to just plainly run a test. Co-authored-by: Rich Trott <[email protected]> PR-URL: #26858 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

The hasCrypto check should be checked before anything else to prevent overhead in case it's not falsy. Otherwise the file would be read without any further benefit. PR-URL: #26858 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Instead of throwing an error in case a flag is missing, just start a `child_process` that includes all flags. This improves the situation for all developers in case they want to just plainly run a test. Co-authored-by: Rich Trott <[email protected]> PR-URL: #26858 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

The hasCrypto check should be checked before anything else to prevent overhead in case it's not falsy. Otherwise the file would be read without any further benefit. PR-URL: #26858 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Signed-off-by: Beth Griggs <[email protected]>

BridgeAR requested a review from Trott March 22, 2019 15:46

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 22, 2019

sam-github reviewed Mar 22, 2019

View reviewed changes

test/common/index.js Outdated Show resolved Hide resolved

fixup! test: do not require flags when executing a file

e5e3d0d

BridgeAR requested a review from jasnell March 25, 2019 14:35

jasnell approved these changes Mar 25, 2019

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 Mar 25, 2019

fixup! test: do not require flags when executing a file

88ccbcd

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 26, 2019

BridgeAR added 2 commits March 27, 2019 13:13

test: move hasCrypto check

9a8748c

The hasCrypto check should be checked before anything else to prevent overhead in case it's not falsy. Otherwise the file would be read without any further benefit.

fixup! test: do not require flags when executing a file

cda4a8c

BridgeAR requested review from sam-github and tniessen March 28, 2019 11:40

BridgeAR requested a review from refack March 28, 2019 11:40

tniessen approved these changes Mar 28, 2019

View reviewed changes

test/common/index.js Outdated Show resolved Hide resolved

fixup! test: do not require flags when executing a file

56576c4

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 28, 2019

targos approved these changes Mar 29, 2019

View reviewed changes

BridgeAR closed this Mar 29, 2019

BethGriggs mentioned this pull request Apr 9, 2019

v11.14.0 proposal #27163

Merged

BridgeAR deleted the do-not-require-flags branch January 20, 2020 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: do not require flags when executing a file #26858

test: do not require flags when executing a file #26858

BridgeAR commented Mar 22, 2019

nodejs-github-bot commented Mar 22, 2019

sam-github commented Mar 22, 2019

refack commented Mar 22, 2019

BridgeAR commented Mar 24, 2019

tniessen commented Mar 25, 2019

nodejs-github-bot commented Mar 25, 2019

sam-github commented Mar 25, 2019

tniessen commented Mar 25, 2019

BridgeAR commented Mar 26, 2019

BridgeAR commented Mar 26, 2019

refack commented Mar 26, 2019 •

edited

Loading

BridgeAR commented Mar 27, 2019

tniessen commented Mar 27, 2019

BridgeAR commented Mar 27, 2019

nodejs-github-bot commented Mar 27, 2019

nodejs-github-bot commented Mar 28, 2019

nodejs-github-bot commented Mar 28, 2019 •

edited by BridgeAR

Loading

BridgeAR commented Mar 29, 2019

test: do not require flags when executing a file #26858

test: do not require flags when executing a file #26858

Conversation

BridgeAR commented Mar 22, 2019

Checklist

nodejs-github-bot commented Mar 22, 2019

sam-github commented Mar 22, 2019

refack commented Mar 22, 2019

BridgeAR commented Mar 24, 2019

tniessen commented Mar 25, 2019

nodejs-github-bot commented Mar 25, 2019

sam-github commented Mar 25, 2019

tniessen commented Mar 25, 2019

BridgeAR commented Mar 26, 2019

BridgeAR commented Mar 26, 2019

refack commented Mar 26, 2019 • edited Loading

BridgeAR commented Mar 27, 2019

tniessen commented Mar 27, 2019

BridgeAR commented Mar 27, 2019

nodejs-github-bot commented Mar 27, 2019

nodejs-github-bot commented Mar 28, 2019

nodejs-github-bot commented Mar 28, 2019 • edited by BridgeAR Loading

BridgeAR commented Mar 29, 2019

refack commented Mar 26, 2019 •

edited

Loading

nodejs-github-bot commented Mar 28, 2019 •

edited by BridgeAR

Loading