test_runner: improve --test-timeout to be per test

Previously `--test-timeout` is set on per test execution, this is
obviously a bug as per test execution is hard to be expected, this patch
addresses the issue by setting `timeout` from per execution to per test.

This patch also fixes a minor issue that `--test-timeout` is not being
respected when running without `--test`.

PR-URL: https://github.com/nodejs/node/pull/57672
Fixes: https://github.com/nodejs/node/issues/57656
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lib/internal/test_runner/runner.js

@@ -191,6 +191,7 @@ class FileTest extends Test {
       column: 1,
       file: resolve(this.name),
     };
+    this.timeout = null;
   }
 
   #skipReporting() {

lib/internal/test_runner/test.js

@@ -608,6 +608,12 @@ class Test extends AsyncResource {
     if (timeout != null && timeout !== Infinity) {
       validateNumber(timeout, 'options.timeout', 0, TIMEOUT_MAX);
       this.timeout = timeout;
+    } else if (timeout == null) {
+      const cliTimeout = this.config.timeout;
+      if (cliTimeout != null && cliTimeout !== Infinity) {
+        validateNumber(cliTimeout, 'this.config.timeout', 0, TIMEOUT_MAX);
+        this.timeout = cliTimeout;
+      }
     }
 
     if (skip) {
@@ -1394,6 +1400,7 @@ class Suite extends Test {
   reportedType = 'suite';
   constructor(options) {
     super(options);
+    this.timeout = null;
 
     if (this.config.testNamePatterns !== null &&
         this.config.testSkipPatterns !== null &&

lib/internal/test_runner/utils.js

@@ -196,6 +196,7 @@ function parseCommandLine() {
   const sourceMaps = getOptionValue('--enable-source-maps');
   const updateSnapshots = getOptionValue('--test-update-snapshots');
   const watch = getOptionValue('--watch');
+  const timeout = getOptionValue('--test-timeout') || Infinity;
   const isChildProcess = process.env.NODE_TEST_CONTEXT === 'child';
   const isChildProcessV8 = process.env.NODE_TEST_CONTEXT === 'child-v8';
   let concurrency;
@@ -211,7 +212,6 @@ function parseCommandLine() {
   let shard;
   let testNamePatterns = mapPatternFlagToRegExArray('--test-name-pattern');
   let testSkipPatterns = mapPatternFlagToRegExArray('--test-skip-pattern');
-  let timeout;
 
   if (isChildProcessV8) {
     kBuiltinReporters.set('v8-serializer', 'internal/test_runner/reporter/v8-serializer');
@@ -242,7 +242,6 @@ function parseCommandLine() {
 
   if (isTestRunner) {
     isolation = getOptionValue('--test-isolation');
-    timeout = getOptionValue('--test-timeout') || Infinity;
 
     if (isolation === 'none') {
       concurrency = 1;
@@ -271,7 +270,6 @@ function parseCommandLine() {
       };
     }
   } else {
-    timeout = Infinity;
     concurrency = 1;
     const testNamePatternFlag = getOptionValue('--test-name-pattern');
     only = getOptionValue('--test-only');

test/fixtures/test-runner/output/test-timeout-flag.js

@@ -0,0 +1,19 @@
+// Flags: --test-timeout=20
+'use strict';
+const { describe, test } = require('node:test');
+const { setTimeout } = require('node:timers/promises');
+
+describe('--test-timeout is set to 20ms', () => {
+  test('should timeout after 20ms', async () => {
+    await setTimeout(200000, undefined, { ref: false });
+  });
+  test('should timeout after 5ms', { timeout: 5 }, async () => {
+    await setTimeout(200000, undefined, { ref: false });
+  });
+
+  test('should not timeout', { timeout: 50000 }, async () => {
+    await setTimeout(1);
+  });
+
+  test('should pass', async () => {});
+});

test/fixtures/test-runner/output/test-timeout-flag.snapshot

@@ -0,0 +1,55 @@
+TAP version 13
+# Subtest: --test-timeout is set to 20ms
+    # Subtest: should timeout after 20ms
+    not ok 1 - should timeout after 20ms
+      ---
+      duration_ms: *
+      type: 'test'
+      location: '/test/fixtures/test-runner/output/test-timeout-flag.js:(LINE):3'
+      failureType: 'testTimeoutFailure'
+      error: 'test timed out after 20ms'
+      code: 'ERR_TEST_FAILURE'
+      stack: |-
+        async Promise.all (index 0)
+      ...
+    # Subtest: should timeout after 5ms
+    not ok 2 - should timeout after 5ms
+      ---
+      duration_ms: *
+      type: 'test'
+      location: '/test/fixtures/test-runner/output/test-timeout-flag.js:(LINE):3'
+      failureType: 'testTimeoutFailure'
+      error: 'test timed out after 5ms'
+      code: 'ERR_TEST_FAILURE'
+      ...
+    # Subtest: should not timeout
+    ok 3 - should not timeout
+      ---
+      duration_ms: *
+      type: 'test'
+      ...
+    # Subtest: should pass
+    ok 4 - should pass
+      ---
+      duration_ms: *
+      type: 'test'
+      ...
+    1..4
+not ok 1 - --test-timeout is set to 20ms
+  ---
+  duration_ms: *
+  type: 'suite'
+  location: '/test/fixtures/test-runner/output/test-timeout-flag.js:(LINE):1'
+  failureType: 'subtestsFailed'
+  error: '2 subtests failed'
+  code: 'ERR_TEST_FAILURE'
+  ...
+1..1
+# tests 4
+# suites 1
+# pass 2
+# fail 0
+# cancelled 2
+# skipped 0
+# todo 0
+# duration_ms *

test/parallel/test-runner-filetest-location.js

@@ -4,7 +4,7 @@ const fixtures = require('../common/fixtures');
 const { strictEqual } = require('node:assert');
 const { relative } = require('node:path');
 const { run } = require('node:test');
-const fixture = fixtures.path('test-runner', 'never_ending_sync.js');
+const fixture = fixtures.path('test-runner', 'index.js');
 const relativePath = relative(process.cwd(), fixture);
 const stream = run({
   files: [relativePath],
@@ -13,7 +13,7 @@ const stream = run({
 
 stream.on('test:fail', common.mustCall((result) => {
   strictEqual(result.name, relativePath);
-  strictEqual(result.details.error.failureType, 'testTimeoutFailure');
+  strictEqual(result.details.error.failureType, 'testCodeFailure');
   strictEqual(result.line, 1);
   strictEqual(result.column, 1);
   strictEqual(result.file, fixture);

test/parallel/test-runner-output.mjs

@@ -132,6 +132,15 @@ const tests = [
     name: 'test-runner/output/timeout_in_before_each_should_not_affect_further_tests.js',
     flags: ['--test-reporter=tap'],
   },
+  {
+    name: 'test-runner/output/test-timeout-flag.js',
+    flags: ['--test-reporter=tap'],
+  },
+  // --test-timeout should work with or without --test flag
+  {
+    name: 'test-runner/output/test-timeout-flag.js',
+    flags: ['--test-reporter=tap', '--test'],
+  },
   {
     name: 'test-runner/output/hooks-with-no-global-test.js',
     flags: ['--test-reporter=tap'],

test/parallel/test-runner-run.mjs

@@ -70,10 +70,9 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
 
   it('should support timeout', async () => {
     const stream = run({ timeout: 50, files: [
-      fixtures.path('test-runner', 'never_ending_sync.js'),
-      fixtures.path('test-runner', 'never_ending_async.js'),
+      fixtures.path('test-runner', 'timeout-basic.mjs'),
     ] });
-    stream.on('test:fail', common.mustCall(2));
+    stream.on('test:fail', common.mustCall(1));
     stream.on('test:pass', common.mustNotCall());
     // eslint-disable-next-line no-unused-vars
     for await (const _ of stream);