test: fix hijackStdout behavior in console #14647

XadillaX · 2017-08-06T06:18:44Z

console.log and some other function will swallow the exception in stdout.write. So an asynchronous exception is needed, or common.hijackStdout won't detect some exception.

Refs: https://github.com/nodejs/node/blob/v8.2.1/lib/console.js#L87

Checklist

make -j4 test
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

`console.log` and some other function will swallow the exception in `stdout.write`. So an asynchronous exception is needed, or `common.hijackStdout` won't detect some exception. Refs: https://github.com/nodejs/node/blob/v8.2.1/lib/console.js#L87

XadillaX · 2017-08-07T04:48:13Z

/cc @nodejs/testing

refack

1 CR
2 nits

refack · 2017-08-09T02:00:20Z

test/common/index.js

+    try {
+      listener(data);
+    } catch(e) {
+      process.nextTick(function() {


Nit: I know you like functions but this could be a single line.
IMHO the indentation is weird

refack · 2017-08-09T02:02:14Z

test/parallel/test-common.js

+});
+
+let uncaughtTimes = 0;
+process.on('uncaughtException', common.mustCallAtLeast(function(e) {


CR: How is the mustCallAtLeast fit with the ${([ 'err', 'out' ])[uncaughtTimes++]} trick?

The second parameter of common.mustCallAtLeast is 2.

Sorry I wasn't clear. I meant it should be mustCall, as any more calls would generate console undefined error

refack · 2017-08-09T02:04:59Z

test/parallel/test-common.js

+
+// hijackStderr and hijackStdout again
+// for console
+[ 'err', 'out' ].forEach((txt) => {


Optional: do [['err', 'error'], ['out', 'log']].forEach(([ext, method]) => {
instead of console[txt === 'err' ? 'error' : 'log']('test');

XadillaX · 2017-08-11T02:31:48Z

@refack updated.

refack

LGTM. Just need to replace mustCallAtLeast with mustCall

refack · 2017-08-11T02:45:38Z

test/parallel/test-common.js

+});
+
+let uncaughtTimes = 0;
+process.on('uncaughtException', common.mustCallAtLeast(function(e) {


Sorry I wasn't clear. I meant it should be mustCall, as any more calls would generate console undefined error

XadillaX · 2017-08-11T06:17:45Z

@refack How about now? I've added the code:

assert.strictEqual(uncaughtTimes < 2, true);

refack · 2017-08-11T11:17:10Z

@refack How about now? I've added the code:

assert.strictEqual(uncaughtTimes < 2, true);

That works too.

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

jasnell · 2017-08-29T05:23:44Z

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

jasnell · 2017-08-29T17:05:02Z

Trying again on windows https://ci.nodejs.org/job/node-test-commit-windows-fanned/11379/

`console.log` and some other function will swallow the exception in `stdout.write`. So an asynchronous exception is needed, or `common.hijackStdout` won't detect some exception. Refs: https://github.com/nodejs/node/blob/v8.2.1/lib/console.js#L87 PR-URL: #14647 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

jasnell · 2017-08-29T18:00:47Z

Landed in 1ffd01c

`console.log` and some other function will swallow the exception in `stdout.write`. So an asynchronous exception is needed, or `common.hijackStdout` won't detect some exception. Refs: https://github.com/nodejs/node/blob/v8.2.1/lib/console.js#L87 PR-URL: nodejs/node#14647 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

`console.log` and some other function will swallow the exception in `stdout.write`. So an asynchronous exception is needed, or `common.hijackStdout` won't detect some exception. Refs: https://github.com/nodejs/node/blob/v8.2.1/lib/console.js#L87 PR-URL: nodejs#14647 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

`console.log` and some other function will swallow the exception in `stdout.write`. So an asynchronous exception is needed, or `common.hijackStdout` won't detect some exception. Refs: https://github.com/nodejs/node/blob/v8.2.1/lib/console.js#L87 PR-URL: #14647 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins · 2017-09-20T19:08:47Z

it looks like this is a patch to test features that don't exist on v6.x

Should we backport them?

MylesBorins · 2017-10-16T22:10:07Z

ping

MylesBorins · 2017-11-14T20:19:38Z

ping @XadillaX

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 6, 2017

XadillaX force-pushed the test-improve-hijack-std branch from d3d6dba to c45e347 Compare August 6, 2017 06:21

XadillaX requested a review from joyeecheung August 6, 2017 06:22

jasnell approved these changes Aug 8, 2017

View reviewed changes

refack suggested changes Aug 9, 2017

View reviewed changes

update after reviewing

9dbe31b

refack approved these changes Aug 11, 2017

View reviewed changes

update after reviewing

0ef550c

hiroppy approved these changes Aug 27, 2017

View reviewed changes

BridgeAR approved these changes Aug 27, 2017

View reviewed changes

joyeecheung approved these changes Aug 29, 2017

View reviewed changes

jasnell closed this Aug 29, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the lts-watch-v6.x label Sep 20, 2017

MylesBorins added dont-land-on-v6.x and removed lts-watch-v6.x labels Dec 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix hijackStdout behavior in console #14647

test: fix hijackStdout behavior in console #14647

XadillaX commented Aug 6, 2017 •

edited

Loading

XadillaX commented Aug 7, 2017

refack left a comment

refack Aug 9, 2017 •

edited

Loading

refack Aug 9, 2017 •

edited

Loading

XadillaX Aug 10, 2017 •

edited

Loading

refack Aug 11, 2017

refack Aug 9, 2017

XadillaX commented Aug 11, 2017

refack left a comment

refack Aug 11, 2017

XadillaX commented Aug 11, 2017

refack commented Aug 11, 2017

jasnell commented Aug 29, 2017

jasnell commented Aug 29, 2017

jasnell commented Aug 29, 2017

MylesBorins commented Sep 20, 2017

MylesBorins commented Oct 16, 2017

MylesBorins commented Nov 14, 2017

test: fix hijackStdout behavior in console #14647

test: fix hijackStdout behavior in console #14647

Conversation

XadillaX commented Aug 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

XadillaX commented Aug 7, 2017

refack left a comment

Choose a reason for hiding this comment

refack Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

refack Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

XadillaX Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

refack Aug 11, 2017

Choose a reason for hiding this comment

refack Aug 9, 2017

Choose a reason for hiding this comment

XadillaX commented Aug 11, 2017

refack left a comment

Choose a reason for hiding this comment

refack Aug 11, 2017

Choose a reason for hiding this comment

XadillaX commented Aug 11, 2017

refack commented Aug 11, 2017

jasnell commented Aug 29, 2017

jasnell commented Aug 29, 2017

jasnell commented Aug 29, 2017

MylesBorins commented Sep 20, 2017

MylesBorins commented Oct 16, 2017

MylesBorins commented Nov 14, 2017

XadillaX commented Aug 6, 2017 •

edited

Loading

refack Aug 9, 2017 •

edited

Loading

refack Aug 9, 2017 •

edited

Loading

XadillaX Aug 10, 2017 •

edited

Loading