test: fix flaky test-preload

[Trott]

Checklist

• tests and code linting passes
• the commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

Use close event rather than exit event to make sure all output has
been received before checking assertions.

Fixes: #6722

/cc @Fishrock123 @mhdawson
h/t @santigimeno

[Trott]

Stress test showing the test flaky on current master on CentOS 7 (64-bit):
https://ci.nodejs.org/job/node-stress-single-test/720/nodes=centos7-64/console

Stress test showing the test reliable on this PR's branch on CentOS 7 (64-bit):
https://ci.nodejs.org/job/node-stress-single-test/719/nodes=centos7-64/console

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

[santigimeno]

I can reproduce the problem locally on OS X. With this fix, the problem disappears.

LGTM

[evanlucas]

LGTM. I just noticed this locally and was trying to figure out what was up. :]

[cjihrig]

LGTM

[Fishrock123]

LGTM, should we add comments for this, or is this expected?

[Trott]

@Fishrock123 asked:

...should we add comments for this, or is this expected?

Not sure...

According to the docs for exit:

Note that when the 'exit' event is triggered, child process stdio streams might still be open.

So, expected behavior, hooray! Except...

On the other hand, the docs for close say:

The 'close' event is emitted when the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams.

Multiple processes sharing stdio...we don't have that here, right? Hmmm...

It might also be a manifestation of a known issue around exit and buffering stdio but again, I'm not sure.

[Trott]

Anyone else think it's a good idea to land this soon rather than giving it the 72 hours that it would require if it were non-trivial? I'm thinking about:

• the trivial nature of this change
• the ease with which it could be reverted or amended if necessary (most likely with a comment or two)
• the importance (to me, at least) of having reliable CI results that people don't ignore; these frequent failures are a menace
• the exactly-as-expected stress test and CI results for the PR
• four LGTMs

[Fishrock123]

It fixes the CI and the related issue have been open for at least a day? Trivial enough to land imo. (Also it's not the weekend yet, 48 hours.)

[addaleax]

LGTM and go ahead, please :)

[santigimeno]

It may also be worth noting that the behavior might change with the upcoming libuv release, which, IIUIC, would guarantee that the exit event would be emitted always after the close event. See @bnoordhuis comment here: #6575 (comment)

[Trott]

Landed in b5a75ba.