test: clarify role of domains in microtask queue tests

[Trott]

Add this comment:

> Requiring domain calls _setupDomainUse which in turns changes the
> tickCallback function that is used to call callbacks scheduled with
> process.nextTick.
>
> So requiring domain tests the _tickDomainCallback function, whereas
> the tests equivalent tests without domain test the _tickCallback
> function.

Also removes unnecessary assignment of domain module to a variable.

R: @misterdjules
R: @jasnell

[jasnell]

@misterdjules

[misterdjules]

Requiring domains calls _setupDomainUse which in turns changes the tickCallback function that is used to call callbacks scheduled with process.nextTick.

So these tests test the _tickDomainCallback function, whereas the tests without the domain suffix in the file name test the _tickCallback function.

Thus I would think we don't want to remove them.

[jasnell]

+1 to @misterdjules comments. However, it might be a good idea to add some comments into the test files themselves that explain so that the require('domain') isn't removed in the future.

[Trott]

Would it be OK to change the relevant lines from this:

var domain = require('domain');

To just this?:

require('domain');

(Along with adding a comment per @jasnell.)

If so, I'll go ahead and do that (unless someone else is very motivated to do it).

[misterdjules]

@jasnell @Trott Both your suggestions sound good to me.

[Trott]

Rebased, updated, force pushed, changed description and title here. PTAL.

[cjihrig]

LGTM

[misterdjules]

Not very important, but just an observation: I would remove the calls _setupDomainUse part and just keep the changes the function that is used to call callbacks.... The reason is that that much details is not needed, and it would make the comment depend too much on implementation.

[misterdjules]

I would personally rephrase it to something like:

// Requiring the domain module here changes the function that is used by node to call
// process.nextTick's callbacks to a variant that specifically handles domains. We want
// to test this specific variant in this test, and so even if the domain module is not used,
// this require call is needed and must not be removed.
require('domain')

It depends a bit less on implementation details while still being explicit and clear. But I'm also fine with the existing comments, as long as the typo that I mentioned in an inline comment is fixed.

[Trott]

@misterdjules Updated with your definitely-better suggested text. Thanks.

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

[misterdjules]

The two commits will need to be squashed into one. The commit message can probably be shorter and just mention that a comment was added to make the purpose of these tests clearer. I don't think we need to quote the whole comment.

Other than that, LGTM. Thank you very much for doing that!

[Trott]

Landed in b16a50d