test_runner: pass signal on timeout

[MoLow]

follow up for #43554 (review)

[cjihrig]

Is there any reason not to do this in a message test to make sure the TAP output looks correct?

[MoLow]

that is already checked here https://github.com/nodejs/node/blob/389b7e138e89a339fabe4ad628bf09cd9748f957/test/message/test_runner_abort.js
the purpose of this test is to confirm timeout signals to the abort signal and that passed signal is validated

[cjihrig]

From what I can tell, that file doesn't test the case where input validation throws. I pulled down your branch and ran this:

const test = require('node:test');

test();

It generated correct TAP output. Next, I ran this (taken from this PR):

const test = require('node:test');

test({ signal: {} });

There was no TAP output generated, only an uncaught exception.

We need to decide what the test runner should do when input validation fails. The current behavior seems undesirable since we could fail the test with the exception instead of crashing.

[aduh95]

I think I would expect not to generate TAP output if a validation check fails, it's not a test failure, the test simply doesn't run because the test runner couldn't make sense of what the user is asking.
I don't feel strongly about that, maybe there's a good use case for wanting a TAP output even in this case, but a classic uncaught exception seems more useful to me.

[MoLow]

I agree the test runner should try to minimize the cases where the output is not valid TAP since tooling does depend on the output.

[aduh95]

I think I would expect not to generate TAP output if a validation check fails, it's not a test failure, the test simply doesn't run because the test runner couldn't make sense of what the user is asking.
I don't feel strongly about that, maybe there's a good use case for wanting a TAP output even in this case, but a classic uncaught exception seems more useful to me.

[aduh95]

Why use a subprocess here? Wouldn't it be simpler to test in the same process?

test(async () => {
  let testSignal;
  await assert.rejects(test({ timeout: 10 }, ({ signal }) => {
    testSignal = signal;
    assert.strictEqual(signal.aborted, false);
    return new Promise(() => {});
  }), { code: 'ERR_TEST_FAILURE' });

  setTimeout(50, common.mustCall(() => assert.strictEqual(testSignal?.aborted, true)));
});

Otherwise we might as well do a message test instead.

[MoLow]

test() never rejects for the same reason discussed above - the tap output will not be reliable otherwise

[aduh95]

I don't understand this argument, when running on the CI, users will use --test CLI flag, which wraps the files in a valid TAP output, I don't think it makes sense to try to force a valid TAP output in all circumstances if the flag is not there.

[MoLow]

let me rephrase: it is not just about the TAP output, but about what is the essential API of a test runner and what a test runner is expected to do,
by definition, a test catches errors/rejections and does not throw when it caught one - it reports a failure in a standard way so whoever is looking (ci/human) can know the test failed.
if you simply throw any error you catch - there is no real reason to use a test runner (correct me if I am missing anything?)

the only difference here is that this error is not generated within the test/by the test author - but by the test runner.

the question to be asked is "is a timeout considered a test failure?" if yes then it should behave the same as test(() => Promise.reject(new Error()) which definitely should not reject

when running on the CI, users will use --test CLI flag

I disagree about only relating to that as the test runner. users should be able to also run a single test file and get valid output, or run multiple files using any logic wanted.

[aduh95]

Would rejecting the promise be at odd with any of those goals really? I don't see a reason why we can't have both: test(Promise.reject(new Error()) returns a rejected promise, and we output valid TAP output on stdout.

[MoLow]

I tested this piece of code with a few frameworks (syntax adjusted a little per framework):

test('1', async() => {
    await test('1.1', Promise.reject(new Error()));
    await test('1.2', Promise.reject(new Error()));
})

What you suggest (unless I misunderstood) is that only tests 1, 1.1 need to run - and I think users expect all three 1, 1.1, 1.2 to run

• playwright - ran all three tests (test does not return a promise)
• mocha - ran all three tests (test does not return a promise)
• jest - this isn't even possible - you must define all tests synchronously (Returning a Promise from "describe" is not supported. Tests must be defined synchronously.) in addition test does not return a promise
• tape - ran all three tests (test does not return a promise)
• tap - ran all three tests (this is the only framework where test actually returns a promise)

[aduh95]

I can see why test runners that don't return a promise would do that, it's weird to me that a promise resolving when the previous test fails. Anyway, if that's the consensus, let's roll with it, although it makes testing the test runner a bit trickier.

[MoLow]

Yeah, the fact that a test framework returns a promise can be confusing, see this issue as well:
nodejs/help#3837

[MoLow]

this makes me think that a test runner should probably not return a promise since it kind of violates IOC

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 2e682f1