test_runner: validate timeout option

[aduh95]

/cc @nodejs/test_runner

[tniessen]

Is there a reason for requiring an uint32 here instead of any positive (in particular, non-NaN) number?

For example, because timeouts are inherently flaky, it is often necessary to specify a factor depending on the platform to reduce flakiness (as we do for many platforms). It might be surprising if 1.5 * timeout suddenly fails because it's not an uint32.

[MoLow]

the reason is that is what timeouts support

node/lib/internal/timers.js

Lines 167 to 172 in 9256728

	if (after > TIMEOUT_MAX) {
	process.emitWarning(`${after} does not fit into` +
	' a 32-bit signed integer.' +
	'\nTimeout duration was set to 1.',
	'TimeoutOverflowWarning');
	}

[tniessen]

@MoLow But timeouts allow non-integers in the range [0, TIMEOUT_MAX]. For example, let's say a timeout is 9999 and someone pre-scales it with a factor of 1.5 on some slow platform because the test is flaky. The result is 14998.5, which is a valid timeout, but not an uint32.

[tniessen]

I realize that this is a ridiculously long timeout (roughly 25 days) for most purposes, but should we at least emit a warning otherwise? And/or set the timeout to TIMEOUT_MAX? Or throw?

[aduh95]

Ah yes, I didn't realize it was that long. In that case, I think that throwing an ERR_OUT_OF_RANGE would be the most helpful thing to do here (implemented in 5d2b348), but no strong feelings, setting overflow values as TIMEOUT_MAX and emitting a warning also sounds fine if someone prefers that.

[F3n67u]

Nice work. So great to see how this pr evolved into a better solution.

[cjihrig]

I think we should add a test runner test for this - note that this is the first piece of input validation that throws instead of coercing.

[aduh95]

I think we should add a test runner test for this

Not sure what you mean by that, doesn't https://github.com/nodejs/node/blob/da22723f3beafe4f5629b939d5a5c902ddea8b9d/test/parallel/test-runner-option-validation.js already cover that?

note that this is the first piece of input validation that throws instead of coercing.

In the test_runner maybe, in Node.js that's not uncommon.

[cjihrig]

Sorry, I meant something that shows the test runner TAP output. IMO if the user passes a bad timeout value, the test runner should still generate valid TAP and only fail the test with the bad input.

In the test_runner maybe, in Node.js that's not uncommon.

I'm well aware. But I don't think we have any tests for the test runner that show that throwing is properly handled.

[MoLow]

I'm well aware. But I don't think we have any tests for the test runner that show that throwing is properly handled.

FYI, added one here

node/test/parallel/test-runner-misc.js

Lines 11 to 14 in d7b97a2

	assert.throws(() => test({ signal: {} }), {
	code: 'ERR_INVALID_ARG_TYPE',
	name: 'TypeError'
	});

[aduh95]

@cjihrig can we get to a consensus here? My position is that it would be OK for the test runner to throw and not produce a valid TAP output when it's provided invalid parameters – consumers that need valid TAP output can use the --test CLI flag, which should always produce a valid TAP output. Is this a reasonable position?
I'm open to alternatives, as long as we don't keep the current behavior where the test runner silently ignores any value that's not a Uint32.

[cjihrig]

I still think producing TAP on invalid options is a better user experience and not very hard to implement, but that can be addressed later and doesn't need to block this.

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/43843
✔  Done loading data for nodejs/node/pull/43843
----------------------------------- PR info ------------------------------------
Title      test_runner: validate `timeout` option (#43843)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:validate-timeout-test-runner -> nodejs:main
Labels     experimental, author ready, needs-ci, dont-land-on-v14.x, dont-land-on-v16.x, test_runner
Commits    1
 - test_runner: validate `timeout` option
Committers 1
 - Antoine du Hamel 
PR-URL: https://github.com/nodejs/node/pull/43843
Reviewed-By: Feng Yu 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43843
Reviewed-By: Feng Yu 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - test_runner: validate `timeout` option
   ℹ  This PR was created on Fri, 15 Jul 2022 09:45:39 GMT
   ✔  Approvals: 1
   ✔  - Feng Yu (@F3n67u): https://github.com/nodejs/node/pull/43843#pullrequestreview-1041212763
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-07-24T16:51:16Z: https://ci.nodejs.org/job/node-test-pull-request/45658/
- Querying data for job/node-test-pull-request/45658/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2728234645

[aduh95]

Landed in d83446b