test_runner,cli: --test-timeout should be used with --test

[jakecastelli]

When --test-timeout used without --test it does not behave what the user would expect.

Refs: #50431 (comment)

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[nodejs-github-bot]

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

[rluvaton]

Isn't it breaking change? (nobody should use test timeout without test but it would break now)

[jakecastelli]

Hi @rluvaton, yes it would break, on the other hand this feels more like a bug or at least false positive to me. As without --test then --test-timeout does not behave like what it is supposed to be.

[cjihrig]

I don't think this is the correct fix. The problem is that --test-timeout as implemented simply does not work properly (IMO). The flag itself should be supported with or without the --test flag.

[jakecastelli]

Hi @cjihrig thanks for your input, I thought the origin implementation meant to be used with --test.

The flag itself should be supported with or without the --test flag

Now I see your point, in that case we will need to grab the --test-timeout flag around here to ensure this.timeout is not null.

Let me know your thoughts, cheers.

[jakecastelli]

I have another question which is when timeout is passed in as an option along with --test-timeout cli flag, which one should take priority?

consider the following code snippet:

import { test } from "node:test";

test("test timeout", { timeout: 100 }, (t, done) => {
  setTimeout(done, 3000);
});

node --test-timeout=2000

Should 100 be applied or 2000?

[rluvaton]

I tend to lean to

Test timeout > cli flag > run config

[cjihrig]

Test timeout > cli flag > run config

I think local config should take precedence over more global config. In other words test timeout > run() config > CLI flag

[nodejs-github-bot]

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

[cjihrig]

Explicitly requesting changes. This is technically a breaking change. If we are going to introduce a breaking change on this flag, we should do so in a way that actually makes it better.

[jakecastelli]

If we are going to introduce a breaking change on this flag, we should do so in a way that actually makes it better.

Could you explain in which area we could make it better?

My understanding is that:

• This PR should make it slightly better I think as it prevents the false positive result but it will be a breaking change as --test-timeout could only be used with --test through test_runner in the future.
• We want to support --test-timeout even without --test. as per comment.

Anything else I could've missed?

[cjihrig]

Let me try to explain with an example. Consider the following file named test.js:

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

test('delay', () => {
  return new Promise((resolve) => {
    setTimeout(resolve, 10_000);
  });
});

test('pass');

Someone could run this in a few ways relevant to this PR:

• node test.js - Everything passes after ~10 seconds. This is correct behavior.
• node --test test.js - Everything passes after ~10 seconds. This is correct behavior.
• node --test-timeout=500 test.js - This currently passes after ~10 seconds. The entire file should timeout after 500ms if we consider the current behavior of --test-timeout correct (I do not consider it correct).
• node --test --test-timeout=500 test.js - This currently times out after 500ms. The test results show a single cancelled test corresponding to the entire test.js file.

I am proposing that the behavior of the first two cases stay the same, while the final two cases should both run to completion (the setTimeout() will keep the process alive for longer, but the tests should run) after ~500ms and the results should show one timed out test followed by one passing test.

[jakecastelli]

Oh I see what you mean!! Thanks for explaining it in great details.

I agree, the current implementation of --test-timeout is not correct, it does not make too much sense to apply timeout on the entire file. Therefore my fix is not actually an improvement (consider we want to support the 3rd case).

[jakecastelli]

Let me try to explain with an example. Consider the following file named test.js:

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

test('delay', () => {
  return new Promise((resolve) => {
    setTimeout(resolve, 10_000);
  });
});

test('pass');

Someone could run this in a few ways relevant to this PR:

• node test.js - Everything passes after ~10 seconds. This is correct behavior.
• node --test test.js - Everything passes after ~10 seconds. This is correct behavior.
• node --test-timeout=500 test.js - This currently passes after ~10 seconds. The entire file should timeout after 500ms if we consider the current behavior of --test-timeout correct (I do not consider it correct).
• node --test --test-timeout=500 test.js - This currently times out after 500ms. The test results show a single cancelled test corresponding to the entire test.js file.

I am proposing that the behavior of the first two cases stay the same, while the final two cases should both run to completion (the setTimeout() will keep the process alive for longer, but the tests should run) after ~500ms and the results should show one timed out test followed by one passing test.

Hi Colin, I think there is a 5th case: UPDATE: This is the same as the 4th case.

• node --test --test-timeout=500 This currently cancels the rest of the tests when any of the test timeout. Is it the behaviour we wanted? UPDATE: This is the same as the 4th case.

Consider the following example:

// test-1.mjs
import { test } from "node:test";

test("test timeout", () => {
  return new Promise((resolve) => {
    setTimeout(resolve, 5_000);
  });
});

// test-2.mjs
"use strict";
import { test } from "node:test";

test("delay 10s", () => {
  return new Promise((resolve) => {
    setTimeout(resolve, 10_000);
  });
});

test("delay 1s", () => {
  return new Promise((resolve) => {
    setTimeout(resolve, 1_000);
  });
});

test("pass");

when we run node --test --test-timeout=2000 it will say:

✖ test-2.mjs (2002.938084ms)
  'test timed out after 2000ms'

✖ test.mjs (2002.376083ms)
  'test timed out after 2000ms'

ℹ tests 2
ℹ suites 0
ℹ pass 0
ℹ fail 0
ℹ cancelled 2
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 2011.548291

As you can see, test cases delay 1s and pass didn't get a chance to run and on a side note the summary seems not that helpful as it lacks the test case names.

[cjihrig]

@jakecastelli yes, that is the same as the fourth case I mentioned above. If you're interested in working on this, I would suggest starting with the third case above and then working backward from there to the CLI parts (which will be significantly simpler to fix).

[jakecastelli]

Thanks Colin, yes I am interested in working on this and I think I may close to get a draft PR up for feedback soon.