test_runner: fix global timeout propagation

[pmarchini]

This should address #59701!

While checking this implementation, I also came across this:

[https://github.com/nodejs/node/blob/737b42e4eeaf975ae11d15887898fb99f0e8da1c/lib/internal/test_runner/test.js#L1454-L1459](

node/lib/internal/test_runner/test.js

Lines 1454 to 1459 in 737b42e

	class Suite extends Test {
	reportedType = 'suite';
	constructor(options) {
	super(options);
	this.timeout = null;

)

I'm wondering if this behavior is well documented and working as intended.
Currently, this option is a no-op even if provided.

cc @jakecastelli

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.92%. Comparing base (8692e60) to head (e901721).
⚠️ Report is 56 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59732      +/-   ##
==========================================
+ Coverage   89.83%   89.92%   +0.09%     
==========================================
  Files         667      667              
  Lines      196175   196772     +597     
  Branches    38548    38410     -138     
==========================================
+ Hits       176228   176946     +718     
+ Misses      12408    12264     -144     
- Partials     7539     7562      +23     

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	97.35% <100.00%> (-0.01%)	⬇️

... and 92 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jakecastelli]

Hi Pietro! Thanks a lot for taking the time to investigate and patch!

I will need to take some time to look more deeply into the root cause either tonight or the end of this week, just being too swamped lately ;p

With my quick look, I think this current patch might not be the solution yet - as

import { test, suite } from "node:test";

suite("baa", { timeout: 20 }, () => {
    test("tee", {timeout: 3000},  async () => {
        console.log("tee");
        await new Promise((res) => { setTimeout(res, 1500) });
    })
})

Would fail the test, seems timeout 3000 missed its chance.

[pmarchini]

Hey @jakecastelli 🚀

With my quick look, I think this current patch might not be the solution yet - as

import { test, suite } from "node:test";

suite("baa", { timeout: 20 }, () => {
    test("tee", {timeout: 3000},  async () => {
        console.log("tee");
        await new Promise((res) => { setTimeout(res, 1500) });
    })
})

Would fail the test, seems timeout 3000 missed its chance

The problem here is that, for the suite, timeout is a no-op and is overridden to null by default.
Honestly, I’m not sure why we changed the suite’s behavior this way.

Note:

suite("baa", { timeout: 20 }, () => {
    test("tee", {timeout: 3000},  async () => {
        console.log("tee");
        await new Promise((res) => { setTimeout(res, 1500) });
    })
})

In this case, I would expect the test to fail after 20ms, as the root test has a timeout of 20ms.

blocker has been resolved

[MoLow]

please see my comment in the original issue. I think the behavior is expected, but lets discuss that in the issue.