test: split up and refactor test-domain #13614

addaleax · 2017-06-11T16:28:51Z

Split up test-domain into multiple, more focused test files and use more modern JS inside of them.
/cc @nodejs/testing

refack · 2017-06-11T16:43:20Z

I'm a bit ambivalent about this 🤔
Pros:

more focused tests => better error => easier tracing
readability
better use of common and modern syntax

Cons

I like that in test-domain it's all handled by the same Domain object
[42, null, undefined, false, () => {}, 'string'].forEach(function(something) was nice
multiple tests => multiple node bootstrapping (~1s each)
duplication of setup code

Bottom line

Maybe add another test that asserts that a single domain can handle multiple error events and multiple values?

addaleax · 2017-06-11T16:54:56Z

Maybe add another test that asserts that a single domain can handle multiple error events and multiple values?

Fwiw I added a vaiant of the error-types test that throws all errors inside the same domain.

Trott · 2017-06-11T20:40:10Z

Pros:

One more for the pro list: Reduced likelihood of side effects between tests.

I'm definitely +1 on this.

Trott

LGTM

Trott · 2017-06-11T20:41:16Z

/cc @misterdjules

misterdjules

This generally looks good to me. I like the idea of splitting the tests in separate files, so thanks for doing that!

In general, I'd like it if we kept, or improved on, the existing comments. They help people unfamiliar with the code base to understand the tests' purpose, behavior and internals.

I left a few questions as well. For instance, it seems that a test might have been lost in the transition, but I may have missed something too.

misterdjules · 2017-06-15T15:44:48Z

test/parallel/test-domain-intercept.js

+{
+  const d = new domain.Domain();
+
+  const bound = d.intercept(common.mustCall((data) => {


Instead of removing them, keeping (or improving on) the comments present in the previous version of this test would help understand it.

The problem I see with the previous comments regarding this test (and some of the others) is that they seemed more like API documentation rather than actually helping to explain what the test does. In this case, I hope it becomes much more visible what each part of the test does by identifying the groups of code that test a single piece of the functionality.

Some of these comments describe the intention of these tests. For instance:

// catch thrown errors no matter how many times we enter the event loop [...]

Looking at the new version of that test in test-domain-implicit-binding.js, it's not clear why the error is thrown from a nested async operation nested several layers deep, and so it's not clear to the person who hasn't written this test whether it even makes sense to keep this test.

I still think it would be worth it to keep those comments.

misterdjules · 2017-06-15T15:47:45Z

test/parallel/test-domain-multiple-errors.js

@@ -0,0 +1,23 @@
+'use strict';


What is the difference between this test and test/parallel/test-domain-error-types.js?

The only difference is that one of the tests emits the errors on the same domain object, because that was requested here. I’ve added comments referencing the other test file in each of them.

Great, thanks!

misterdjules · 2017-06-15T15:50:37Z

test/parallel/test-domain-run.js

+
+const d = new domain.Domain();
+
+assert.strictEqual(d.run(() => 'return value'),


Here too, keeping or improving on the comments of the previous version of this test would help understand its purpose.

misterdjules · 2017-06-15T15:52:26Z

test/parallel/test-domain.js

-// this only uses implicit binding, except for the first function
-// passed to d.run().  The rest are implicitly bound by virtue of being
-// set up while in the scope of the d domain.
-d.run(function() {


Was this test removed by this change, if so, why?

Yes, that was an oversight. I added it as test-domain-implicit-binding.js.

Excellent, thank you!

addaleax · 2017-06-16T13:44:22Z

CI: https://ci.nodejs.org/job/node-test-commit/10605/

addaleax · 2017-07-23T14:11:20Z

Does anybody mind if I land this next-week-ish?

BridgeAR · 2017-08-30T22:06:37Z

This needs a rebase. Otherwise I think this could land as it.

The comments were addressed as far as I see it. There was no response for a long time. Please have another look.

BridgeAR · 2017-09-08T02:15:08Z

Ping @addaleax

BridgeAR · 2017-09-19T22:43:07Z

Ping @addaleax once more

addaleax · 2017-09-28T19:03:23Z

@BridgeAR … rebased. :)

CI: https://ci.nodejs.org/job/node-test-commit/12676/

jasnell · 2017-09-29T16:35:40Z

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

BridgeAR · 2017-10-02T02:48:50Z

@addaleax seems like there are related failures.

https://ci.nodejs.org/job/node-test-binary-windows/11454/COMPILED_BY=vcbt2015,RUNNER=win10,RUN_SUBSET=2/console

not ok 99 parallel/test-domain-fs-enoent-stream
  ---
  duration_ms: 0.155
  severity: fail
  stack: |-
    assert.js:44
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'ENOENT: no such file or directory, open \'c:\\workspace\\node-test-binary-windows\\COMPILED_BY\\vs2017\\RUNNER\\win2016\\RUN_SU === 'ENOENT: no such file or directory, open \'stream for nonexistent file\''
        at Domain.d.on.common.mustCall (c:\workspace\node-test-binary-windows\COMPILED_BY\vs2017\RUNNER\win2016\RUN_SUBSET\2\test\parallel\test-domain-fs-enoent-stream.js:13:10)
        at Domain.<anonymous> (c:\workspace\node-test-binary-windows\COMPILED_BY\vs2017\RUNNER\win2016\RUN_SUBSET\2\test\common\index.js:517:15)
        at emitOne (events.js:115:13)
        at Domain.emit (events.js:210:7)
        at ReadStream.emit (events.js:180:14)
        at fs.js:2025:12
        at FSReqWrap.oncomplete (fs.js:138:15)

addaleax · 2017-10-07T18:53:46Z

Rebased, new CI to see whether errors persist:

CI: https://ci.nodejs.org/job/node-test-commit/12858/

refack · 2017-10-07T20:52:54Z

All windows machines returned:

not ok 113 parallel/test-domain-fs-enoent-stream
  ---
  duration_ms: 0.143
  severity: fail
  stack: |-
    assert.js:45
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'ENOENT: no such file or directory, open \'c:\\workspace\\node-test-binary-windows\\COMPILED_BY\\vcbt2015\\RUNNER\\win10\\RUN_SU === 'ENOENT: no such file or directory, open \'stream for nonexistent file\''
        at Domain.d.on.common.mustCall (c:\workspace\node-test-binary-windows\COMPILED_BY\vcbt2015\RUNNER\win10\RUN_SUBSET\3\test\parallel\test-domain-fs-enoent-stream.js:13:10)
        at Domain.<anonymous> (c:\workspace\node-test-binary-windows\COMPILED_BY\vcbt2015\RUNNER\win10\RUN_SUBSET\3\test\common\index.js:517:15)
        at emitOne (events.js:115:13)
        at Domain.emit (events.js:210:7)
        at ReadStream.emit (events.js:180:14)
        at fs.js:2028:12
        at FSReqWrap.oncomplete (fs.js:139:15)
  ...

P.S. Seems like the wWindows fs error has a full path.

Split up test-domain into multiple, more focused test files and use more modern JS inside of them.

addaleax · 2017-10-12T06:41:24Z

Okay, I’ve relaxed the error message a bit. This is not an fs test anyway.

CI: https://ci.nodejs.org/job/node-test-commit/13060/

addaleax · 2017-10-16T09:30:25Z

Landed in 1a2f579

Split up test-domain into multiple, more focused test files and use more modern JS inside of them. PR-URL: #13614 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Split up test-domain into multiple, more focused test files and use more modern JS inside of them. PR-URL: nodejs/node#13614 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Split up test-domain into multiple, more focused test files and use more modern JS inside of them. PR-URL: #13614 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

addaleax added domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests. labels Jun 11, 2017

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

addaleax force-pushed the test-domain-refactor branch from 291dd0a to cf4f46f Compare June 11, 2017 16:54

refack approved these changes Jun 11, 2017

View reviewed changes

Trott reviewed Jun 11, 2017

View reviewed changes

cjihrig approved these changes Jun 12, 2017

View reviewed changes

jasnell approved these changes Jun 12, 2017

View reviewed changes

misterdjules previously requested changes Jun 15, 2017

View reviewed changes

addaleax force-pushed the test-domain-refactor branch 2 times, most recently from 0655c38 to 3ccee05 Compare June 16, 2017 13:42

BridgeAR approved these changes Aug 30, 2017

View reviewed changes

BridgeAR added the stalled Issues and PRs that are stalled. label Sep 8, 2017

addaleax force-pushed the test-domain-refactor branch from 3ccee05 to 7b1ae46 Compare September 28, 2017 19:01

addaleax force-pushed the test-domain-refactor branch from 7b1ae46 to 87b083f Compare October 7, 2017 18:53

addaleax force-pushed the test-domain-refactor branch from 87b083f to 0f5c8a1 Compare October 12, 2017 06:37

test: split up and refactor test-domain

ceca532

Split up test-domain into multiple, more focused test files and use more modern JS inside of them.

addaleax force-pushed the test-domain-refactor branch from 0f5c8a1 to ceca532 Compare October 12, 2017 06:40

addaleax removed the stalled Issues and PRs that are stalled. label Oct 16, 2017

addaleax closed this Oct 16, 2017

addaleax deleted the test-domain-refactor branch October 16, 2017 09:30

MylesBorins added the land-on-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: split up and refactor test-domain #13614

test: split up and refactor test-domain #13614

addaleax commented Jun 11, 2017

refack commented Jun 11, 2017 •

edited

Loading

addaleax commented Jun 11, 2017

Trott commented Jun 11, 2017

Trott left a comment

Trott commented Jun 11, 2017

misterdjules left a comment

misterdjules Jun 15, 2017

addaleax Jun 16, 2017

misterdjules Jun 16, 2017

misterdjules Jun 15, 2017

addaleax Jun 16, 2017

misterdjules Jun 16, 2017

misterdjules Jun 15, 2017

misterdjules Jun 15, 2017

addaleax Jun 16, 2017

misterdjules Jun 16, 2017

addaleax commented Jun 16, 2017

addaleax commented Jul 23, 2017

BridgeAR commented Aug 30, 2017

BridgeAR commented Sep 8, 2017

BridgeAR commented Sep 19, 2017

addaleax commented Sep 28, 2017

jasnell commented Sep 29, 2017

BridgeAR commented Oct 2, 2017 •

edited

Loading

addaleax commented Oct 7, 2017

refack commented Oct 7, 2017 •

edited

Loading

addaleax commented Oct 12, 2017

addaleax commented Oct 16, 2017


		const d = new domain.Domain();

		assert.strictEqual(d.run(() => 'return value'),

test: split up and refactor test-domain #13614

test: split up and refactor test-domain #13614

Conversation

addaleax commented Jun 11, 2017

refack commented Jun 11, 2017 • edited Loading

Bottom line

addaleax commented Jun 11, 2017

Trott commented Jun 11, 2017

Trott left a comment

Choose a reason for hiding this comment

Trott commented Jun 11, 2017

misterdjules left a 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

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

addaleax commented Jun 16, 2017

addaleax commented Jul 23, 2017

BridgeAR commented Aug 30, 2017

BridgeAR commented Sep 8, 2017

BridgeAR commented Sep 19, 2017

addaleax commented Sep 28, 2017

jasnell commented Sep 29, 2017

BridgeAR commented Oct 2, 2017 • edited Loading

addaleax commented Oct 7, 2017

refack commented Oct 7, 2017 • edited Loading

addaleax commented Oct 12, 2017

addaleax commented Oct 16, 2017

refack commented Jun 11, 2017 •

edited

Loading

BridgeAR commented Oct 2, 2017 •

edited

Loading

refack commented Oct 7, 2017 •

edited

Loading