test_runner: global after not run if handles are open

[mcollina]

Version

v20.5.0

Platform

mac

Subsystem

test_runner

What steps will reproduce the bug?

Considder the following test:

const { before, after, test } = require('node:test')
const { createServer } = require('http')

let server

before(async () => {
  console.log('before');
  server = createServer((req, res) => {
    res.end('hello')
  })

  await new Promise((resolve, reject) => {
    server.listen(0, (err) => {
      if (err) reject(err)
      else resolve()
    })
  })
})

after(() => {
  console.log('after');
  server.close()
})

test('something', () => {
  console.log('test');
})

We are trying to dispose of a server (or a connection to a DB) inside a global after but the global after is never run.

How often does it reproduce? Is there a required condition?

all the times

What is the expected behavior? Why is that the expected behavior?

for the test to pass / the after hook to be executed

What do you see instead?

the after hook is not executed and therefore the test never terminates

Additional information

No response

[cjihrig]

I'm guessing this is because the test runner uses the beforeExit event, and the ref'ed handle prevents that from being emitted. We'll need to move to a different mechanism for detecting when all of the tests have run.

[mcollina]

I guessed as much, however the above pattern is really good.

[cjihrig]

I looked at this really quickly. The good news is that it's pretty simple to fix this case:

diff --git a/lib/internal/test_runner/harness.js b/lib/internal/test_runner/harness.js
index 36c36f2de1..e9076eb378 100644
--- a/lib/internal/test_runner/harness.js
+++ b/lib/internal/test_runner/harness.js
@@ -158,7 +158,6 @@ function setup(root) {
 
   process.on('uncaughtException', exceptionHandler);
   process.on('unhandledRejection', rejectionHandler);
-  process.on('beforeExit', exitHandler);
   // TODO(MoLow): Make it configurable to hook when isTestRunner === false.
   if (globalOptions.isTestRunner) {
     process.on('SIGINT', terminationHandler);
@@ -180,6 +179,7 @@ function setup(root) {
       topLevel: 0,
       suites: 0,
     },
+    exitHandler,
     shouldColorizeTestFiles: false,
   };
   root.startTime = hrtime();
diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index cc7c81cad8..fd22433a3d 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -687,6 +687,13 @@ class Test extends AsyncResource {
       this.parent.addReadySubtest(this);
       this.parent.processReadySubtestRange(false);
       this.parent.processPendingSubtests();
+
+      if (this.parent === this.root &&
+          this.root.activeSubtests === 0 &&
+          this.root.pendingSubtests.length === 0 &&
+          this.root.readySubtests.size === 0) {
+        this.root.harness.exitHandler();
+      }
     } else if (!this.reported) {
       if (!this.passed && failed === 0 && this.error) {
         this.reporter.fail(0, kFilename, this.subtests.length + 1, kFilename, {

The bad news is that a couple tests are failing. I think it's just related to handling asynchronous activity after the tests finish running like:

test('extraneous async activity test', () => {
  setTimeout(() => { throw new Error(); }, 100);
});

This is kind of expected since the test runner finishes ASAP now instead of once the process is getting ready to exit. I'll have to keep looking into how to best handle this case.

[MoLow]

CC @giltayar we might have discussed this use-case or a similar one in Node.TLV