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: #57672Fixes: #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>

Author: jakecastelli

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;
+}elseif(timeout==null){
+constcliTimeout=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() {
 constsourceMaps=getOptionValue('--enable-source-maps');
 constupdateSnapshots=getOptionValue('--test-update-snapshots');
 constwatch=getOptionValue('--watch');
+consttimeout=getOptionValue('--test-timeout')||Infinity;
 constisChildProcess=process.env.NODE_TEST_CONTEXT==='child';
 constisChildProcessV8=process.env.NODE_TEST_CONTEXT==='child-v8';
 letconcurrency;
@@ -211,7 +212,6 @@ function parseCommandLine() {
 letshard;
 lettestNamePatterns=mapPatternFlagToRegExArray('--test-name-pattern');
 lettestSkipPatterns=mapPatternFlagToRegExArray('--test-skip-pattern');
-lettimeout;
 
 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;
 consttestNamePatternFlag=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()=>{
+awaitsetTimeout(200000,undefined,{ref: false});
+});
+test('should timeout after 5ms',{timeout: 5},async()=>{
+awaitsetTimeout(200000,undefined,{ref: false});
+});
+
+test('should not timeout',{timeout: 50000},async()=>{
+awaitsetTimeout(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');
-constfixture=fixtures.path('test-runner','never_ending_sync.js');
+constfixture=fixtures.path('test-runner','index.js');
 constrelativePath=relative(process.cwd(),fixture);
 conststream=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()=>{
 conststream=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
 forawait(const_ofstream);