test: refactoring and cleanup on child-process tests #32078

jasnell · 2020-03-03T23:58:50Z

Commit 1 in this scratches an itch I've had for a while: it adds a common.debug() to test/common/index.js that uses util.debuglog() as a way of avoiding use of console.log() statements in tests that are not part of the test itself.

~~To enable the debug output while running tests, set the environment variable NODE_DEBUG=test.~~

~~Commit 2~~ Makes a number of improvements to child process tests including use of the debuglog(), more must calls, and use of ../common/countdown where appropriate.

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

gengjiawen · 2020-03-04T03:00:04Z

test parallel/test-child-process-fork-net-server failed.

jasnell · 2020-03-04T19:26:33Z

Interesting... it's passing locally for me. Will investigate.

jasnell · 2020-03-04T19:49:34Z

Looks like one of the events in question is not emitted consistently across platforms. Trying again.

jasnell · 2020-03-04T20:57:38Z

~~Ok looks good now.~~ The events in that test definitely are not very consistent. Added some TODO comments to encourage they be looked at again later.

nodejs-github-bot · 2020-03-04T22:54:55Z

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

jasnell · 2020-03-05T01:14:41Z

Finally, a good CI run. Turns out the actual emission of events in parallel/test-child-process-fork-net-server is quite flaky from one OS to the next to degree that we cannot make any guarantees about which are going to fire. That's very unfortunate really. I've added todo comments in case someone wishes to get back to it.

test/common/README.md

sam-github · 2020-03-20T01:43:08Z

I assumed this would have landed already, and was just searching for the debug utility, with no luck.

We really need to be able to leave debug messages in tests, ones that normally have no output, but where the output can be turned on.

Every time I have to debug a test I start by injecting console.log statements into them so I know where they are failing... then delete them before PRing. Its not a good way to do things.

If the PR is stalled on the stability of tests, maybe the base feature can be PRed on its own?

cjihrig · 2020-03-20T02:22:45Z

@sam-github I think it's stalled on this alternate proposal: #32078 (comment)

jasnell · 2020-03-23T20:09:02Z

Went head and removed the common.debug() from this PR to allow the improvements in the tests to move forward.

nodejs-github-bot · 2020-03-23T20:15:07Z

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

PR-URL: #32078 Reviewed-By: Gireesh Punathil <[email protected]>

jasnell · 2020-03-24T18:23:10Z

Landed in d0c0e20

PR-URL: #32078 Reviewed-By: Gireesh Punathil <[email protected]>

PR-URL: nodejs#32078 Reviewed-By: Gireesh Punathil <[email protected]>

PR-URL: #32078 Reviewed-By: Gireesh Punathil <[email protected]>

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

These rules only apply for the test folder and will already be checked for. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

These rules only apply for the test folder and will already be checked for. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This makes sure all usages of `util.debuglog()` must contain the string 'test' as argument. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

These rules only apply for the test folder and will already be checked for. PR-URL: #32161 Refs: #32078 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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

jasnell force-pushed the test-common-debug branch from 07b1131 to 52df9f5 Compare March 4, 2020 00:06

This comment has been minimized.

Sign in to view

jasnell force-pushed the test-common-debug branch from 8b557b8 to 27c8456 Compare March 4, 2020 22:53

jasnell requested a review from Trott March 5, 2020 20:19

BridgeAR reviewed Mar 9, 2020

View reviewed changes

test/common/README.md Outdated Show resolved Hide resolved

BridgeAR mentioned this pull request Mar 9, 2020

tools: add debuglog eslint rule #32161

Closed

4 tasks

This comment has been minimized.

Sign in to view

test: refactoring / cleanup on child-process tests

bd51d52

jasnell force-pushed the test-common-debug branch from 27c8456 to bd51d52 Compare March 23, 2020 20:07

jasnell changed the title ~~test: add common.debug utility and improve child_process tests~~ test: refactoring and cleanup on child-process tests Mar 23, 2020

gireeshpunathil approved these changes Mar 24, 2020

View reviewed changes

jasnell added a commit that referenced this pull request Mar 24, 2020

test: refactoring / cleanup on child-process tests

d0c0e20

PR-URL: #32078 Reviewed-By: Gireesh Punathil <[email protected]>

jasnell closed this Mar 24, 2020

MylesBorins pushed a commit that referenced this pull request Mar 25, 2020

test: refactoring / cleanup on child-process tests

07e4131

PR-URL: #32078 Reviewed-By: Gireesh Punathil <[email protected]>

MylesBorins mentioned this pull request Mar 25, 2020

v13.12.0 proposal #32376

Merged

targos added backport-blocked-v12.x and removed backport-blocked-v12.x labels Apr 22, 2020

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020

test: refactoring / cleanup on child-process tests

be92b25

PR-URL: nodejs#32078 Reviewed-By: Gireesh Punathil <[email protected]>

targos pushed a commit that referenced this pull request Apr 28, 2020

test: refactoring / cleanup on child-process tests

e3baee6

PR-URL: #32078 Reviewed-By: Gireesh Punathil <[email protected]>

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: refactoring and cleanup on child-process tests #32078

test: refactoring and cleanup on child-process tests #32078

jasnell commented Mar 3, 2020 •

edited

Loading

gengjiawen commented Mar 4, 2020

jasnell commented Mar 4, 2020

This comment has been minimized.

jasnell commented Mar 4, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jasnell commented Mar 4, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Mar 4, 2020

jasnell commented Mar 5, 2020

sam-github commented Mar 20, 2020

cjihrig commented Mar 20, 2020

This comment has been minimized.

jasnell commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

jasnell commented Mar 24, 2020

test: refactoring and cleanup on child-process tests #32078

test: refactoring and cleanup on child-process tests #32078

Conversation

jasnell commented Mar 3, 2020 • edited Loading

Checklist

gengjiawen commented Mar 4, 2020

jasnell commented Mar 4, 2020

This comment has been minimized.

jasnell commented Mar 4, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jasnell commented Mar 4, 2020 • edited Loading

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Mar 4, 2020

jasnell commented Mar 5, 2020

sam-github commented Mar 20, 2020

cjihrig commented Mar 20, 2020

This comment has been minimized.

jasnell commented Mar 23, 2020

nodejs-github-bot commented Mar 23, 2020

jasnell commented Mar 24, 2020

jasnell commented Mar 3, 2020 •

edited

Loading

jasnell commented Mar 4, 2020 •

edited

Loading