test_runner: ref test timeout timers

[cjihrig]

Timeouts associated with tests should keep the process alive for cases like this:

const { test } = require('node:test');

test({ timeout: 3000 }, (t, done) => {
  // done() is not called but there are no ref()'ed handles
  // so the process exits immediately.
});

Refs: #51381

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[nodejs-github-bot]

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

[MoLow]

Suggested change

	test('very long time out ref dosent freeze process', { timeout: 30 * 1e3 }, () => {});

[cjihrig]

I moved this to draft status because it introduces an inconsistency where tests with a timeout keep the event loop alive, but other tests do not. We could work around this a few ways:

1. Keep the status quo where the test timeout is unref'ed. However, this has the problem that I outlined in Test runner: spurious "Promise resolution is still pending but the event loop has already resolved" when a callback tests fails #51381 (comment).
2. Accept this inconsistency. I don't like it, and I think it will lead to bug reports.
3. Create a timeout for all tests. There was originally pushback to having a default test timeout (I disagree with that). However, a bigger issue is that creating a timer for each test introduces noticeable performance overhead.
4. Create the largest setInterval() possible (or anything else that will keep the event loop alive) and clear it when all known tests and hooks have finished. It feels a bit hacky to use an interval that way, but I think could lead to the best DX of those options.
5. Something else?

Thoughts?

Note that options 2, 3, and 4 would also help with issues like #51952 and #52146.

[MoLow]

I am actually ok with the status quo. I might not understand the issue though. why would you expect the example to keep the process alive?:

const { test } = require('node:test');

test({ timeout: 3000 }, (t, done) => {
  // done() is not called but there are no ref()'ed handles
  // so the process exits immediately.
});

I expect the timeout to be the maximum time this take should take, not the minimum. what am I missing?

[cjihrig]

At least with mocha, if you run:

it('test', (done) => {});

You get output like:

  1) test

  0 passing (2s)
  1 failing

  1) test:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/private/tmp/test.js)
      at listOnTimeout (node:internal/timers:573:17)
      at process.processTimers (node:internal/timers:514:7)

If people are fine with Node's current behavior though, I'm fine with closing this.

[MoLow]

Mocha has a default timeout. node:test doesn't really have one

[cjihrig]

I don't think it should make a difference if a timeout is default behavior or explicitly set. I did a quick test with mocha and vitest with the following test that never finishes:

it('test', () => {
  return new Promise(() => {});
});

If there is a test timeout, both mocha and vitest keep the event loop alive. If you disable test timeouts, mocha exits like Node does, while vitest keeps the event loop alive.

So, this PR would actually bring Node's behavior when a timeout is set in line with those two frameworks.