test: split up and refactor test-domain by addaleax · Pull Request #13614 · nodejs/node · GitHub

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

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?

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.

Pros:

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

I'm definitely +1 on this.

LGTM

/cc @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.

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.

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!

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

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!

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

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

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.

Ping @addaleax

Ping @addaleax once more

@BridgeAR … rebased. :)

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

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

@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)

Rebased, new CI to see whether errors persist:

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

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.

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/

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 <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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

test: split up and refactor test-domain by addaleax · Pull Request #13614 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: split up and refactor test-domain - #13614

test: split up and refactor test-domain#13614addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:test-domain-refactoraddaleax/node:test-domain-refactorCopy head branch name to clipboard

Conversation

addaleax commented Jun 11, 2017

Uh oh!

refack commented Jun 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bottom line

Uh oh!

addaleax commented Jun 11, 2017

Uh oh!

Trott commented Jun 11, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Jun 11, 2017

Uh oh!

misterdjules left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Jun 16, 2017

Uh oh!

addaleax commented Jul 23, 2017

Uh oh!

BridgeAR commented Aug 30, 2017

Uh oh!

BridgeAR commented Sep 8, 2017

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

addaleax commented Sep 28, 2017

Uh oh!

jasnell commented Sep 29, 2017

Uh oh!

BridgeAR commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 7, 2017

Uh oh!

refack commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 12, 2017

Uh oh!

addaleax commented Oct 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

test: split up and refactor test-domain#13614
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:test-domain-refactor

refack commented Jun 11, 2017 •
edited
Loading

BridgeAR commented Oct 2, 2017 •
edited
Loading

refack commented Oct 7, 2017 •
edited
Loading