Fix 11826 #11920

clarenced · 2017-03-18T22:47:17Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows [commit guidelines][]

Affected core subsystem(s)

test, domain

clarenced · 2017-03-18T22:49:47Z

@Trott and @gibfahn Here is the PR for the issue #11826.

mscdex · 2017-03-18T23:13:01Z

Perhaps it would be better to combine all of the tests (including the childShouldNotThrowAndAbort() implementation) into one file?

clarenced · 2017-03-18T23:45:54Z

@mscdex I got inspire by what was done in the PR #10329

mscdex · 2017-03-19T00:34:29Z

Yeah I don't agree with that PR's test splitting either. It's quite unnecessary IMHO. All that's needed is to run each test sequentially instead of in parallel. This can be done quite easily by actually keeping the forEach() but using execSync() instead of exec() to spawn the child process for each test.

Trott · 2017-03-19T03:07:28Z

Can you please add documentation for the new common function to test/README.md?

Trott · 2017-03-19T03:18:47Z

Shouldn't this delete the existing test file as it is being replaced by 13 individual test files?

Trott · 2017-03-19T03:19:31Z

@mscdex wrote:

Yeah I don't agree with that PR's test splitting either.

Maybe it would be good for @nodejs/testing to reconvene and try to come up with some test organization guidelines that might be useful in cases like this.

I personally prefer lots of short test files that are very specific over long and monolithic test files that are more likely to have tests suffer from side effects from other tests in the file and so on. But this file isn't that long to begin with so ¯\(ツ)/¯.

Trott · 2017-03-19T20:43:36Z

test/README.md

@@ -410,6 +410,10 @@ The realpath of the 'tmp' directory.

 Name of the temp directory used by tests.

+### childShouldNotThrowAndAbort


Nit: should be childShouldNotThrowAndAbort()

jasnell · 2017-03-20T17:19:35Z

test/common.js

+  const child = child_process.exec(testCmd);
+  child.on('exit', (code, signal) => {
+    const errorMsg = `Test should have exited with exit code 0 
+    but insteand exited with code ${code} and signal ${signal}`;


The multiline string literal is likely going to be problematic because the additional whitespace is preserved. Can you make this the following instead:

const errorMessage = 'Test should have exited with exit code 0 but instead ' + `exited with ${code} and signal ${signal}`;

Generally speaking we do not use multiline string literals in core code

@jasnell Is that extra indentation unintentional?

no, not intentional on the extra indentation.

cjihrig · 2017-03-20T17:29:45Z

test/README.md

@@ -410,6 +410,10 @@ The realpath of the 'tmp' directory.

 Name of the temp directory used by tests.

+### childShouldNotThrowAndAbort
+ This test makes sure that when using --abort-on-uncaught-exception and when throwing an error


--abort-on-uncaught-exception should be in backticks I think.

cjihrig · 2017-03-20T17:30:10Z

test/common.js

@@ -250,6 +250,28 @@ exports.childShouldThrowAndAbort = function() {
  });
 };

+
+// This test makes sure that when using --abort-on-uncaught-exception and


s/test/function/

clarenced · 2017-03-22T23:20:43Z

I have the changes as suggested

jasnell · 2017-03-22T23:34:51Z

test/README.md

@@ -410,6 +410,10 @@ The realpath of the 'tmp' directory.

 Name of the temp directory used by tests.

+### childShouldNotThrowAndAbort()
+ This test makes sure that when using `--abort-on-uncaught-exception` and when throwing an error


There's an unnecessary indent on these two lines

jasnell · 2017-03-22T23:37:55Z

test/parallel/test-domain-abort-on-uncaught-0.js

+
+  d.once('error', (err) => {
+    errorHandlerCalled = true;
+  });


The errorHandlerCalled can be replaced by using common.mustCall() ...e.g.

d.once('error', common.mustCall(() => {}));

clarenced · 2017-04-01T22:13:26Z

New changes submitted

addaleax

@jasnell’s comment on test/README.md should still be addressed, but otherwise LGTM. Thank you!

aqrln · 2017-04-15T01:12:56Z

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

jasnell · 2017-04-17T15:46:45Z

@cjihrig ... PTAL when you get a moment

cjihrig · 2017-04-17T17:46:17Z

test/README.md

@@ -412,6 +412,9 @@ The realpath of the 'tmp' directory.

 Name of the temp directory used by tests.

+### childShouldNotThrowAndAbort()
+ This test makes sure that when using `--abort-on-uncaught-exception` and when throwing an error from within a domain that has an error handler setup, the process _does not_ abort.


Please add a newline here, and wrap the line at 80 characters.

@cjihrig Sorry what do you mean by naming the temp directory used by tests? I am a little confused here

I meant a blank line between ### childShouldNotThrowAndAbort() and the following paragraph. Also, lines should be 80 characters long. I'm not sure what you mean by temp directory.

Sorry I misread your comment, I thought Name of the temp directory used by tests. was part of your comment.

I submitted the changes as suggested

…dAbort()

…ragraph

Trott · 2017-06-14T20:40:27Z

@clarenced Any chance you can rebase this to eliminate the conflicts? (The biggest issue you are likely to run into is that the stuff you put in test/README.md and test/common.js now goes in test/common/README.md and test/common/index.js.)

If that happens, I'll give it another review and try to loop in @cjihrig to get this unstuck.

@clarenced

@clarenced just let me know when/if you rebase this. Happy to take another look. Dismissing my review for now

Trott · 2017-07-30T04:10:39Z

This seems to be stalled. I'm going to close it. By all means, if you're still working on this, please comment to that effect. Thanks!

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

clarenced mentioned this pull request Mar 18, 2017

Fix flaky sequential/test-domain-abort-on-uncaught and move back to parallel #11826

Closed

mscdex added the domain Issues and PRs related to the domain subsystem. label Mar 18, 2017

Trott reviewed Mar 19, 2017

View reviewed changes

jasnell requested changes Mar 20, 2017

View reviewed changes

cjihrig previously requested changes Mar 20, 2017

View reviewed changes

jasnell requested changes Mar 22, 2017

View reviewed changes

addaleax approved these changes Apr 2, 2017

View reviewed changes

jasnell approved these changes Apr 3, 2017

View reviewed changes

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

cjihrig reviewed Apr 17, 2017

View reviewed changes

clarenced added 8 commits April 17, 2017 23:43

test: break up the tests in separate file

86ff5b0

test: lint error correction

ef6819c

test:lint error correction

d1d1a7d

test: documentation for function childShouldNotThrowAndAbort added

aeaf585

test: old file test-domain-abort-on-uncaught.js deleted

8b663fa

test: documentation fix and msg error update in childShouldNotThrowAn…

6156f98

…dAbort()

test: jslint correction

c78ca3b

test: remove unnecessary indentation

38ac775

test: blank line between childShouldNotThrowAndAbort and following pa…

4bb01a7

…ragraph

Trott closed this Jul 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 11826 #11920

Fix 11826 #11920

clarenced commented Mar 18, 2017 •

edited

Loading

clarenced commented Mar 18, 2017

mscdex commented Mar 18, 2017

clarenced commented Mar 18, 2017

mscdex commented Mar 19, 2017 •

edited

Loading

Trott commented Mar 19, 2017

Trott commented Mar 19, 2017

Trott commented Mar 19, 2017

Trott Mar 19, 2017

jasnell Mar 20, 2017

mscdex Mar 20, 2017

jasnell Apr 3, 2017

cjihrig Mar 20, 2017

cjihrig Mar 20, 2017

clarenced commented Mar 22, 2017

jasnell Mar 22, 2017

jasnell Mar 22, 2017

clarenced commented Apr 1, 2017

addaleax left a comment

aqrln commented Apr 15, 2017

jasnell commented Apr 17, 2017

cjihrig Apr 17, 2017

clarenced Apr 17, 2017 •

edited

Loading

cjihrig Apr 17, 2017

clarenced Apr 17, 2017

clarenced Apr 24, 2017

Trott commented Jun 14, 2017

Trott commented Jul 30, 2017

		@@ -410,6 +410,10 @@ The realpath of the 'tmp' directory.

		Name of the temp directory used by tests.

		### childShouldNotThrowAndAbort

Fix 11826 #11920

Fix 11826 #11920

Conversation

clarenced commented Mar 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

clarenced commented Mar 18, 2017

mscdex commented Mar 18, 2017

clarenced commented Mar 18, 2017

mscdex commented Mar 19, 2017 • edited Loading

Trott commented Mar 19, 2017

Trott commented Mar 19, 2017

Trott commented Mar 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarenced commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarenced commented Apr 1, 2017

addaleax left a comment

Choose a reason for hiding this comment

aqrln commented Apr 15, 2017

jasnell commented Apr 17, 2017

Choose a reason for hiding this comment

clarenced Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jun 14, 2017

Trott commented Jul 30, 2017

clarenced commented Mar 18, 2017 •

edited

Loading

mscdex commented Mar 19, 2017 •

edited

Loading

clarenced Apr 17, 2017 •

edited

Loading