test_runner: cancel on termination

[MoLow]

refs: #43505 (comment)
this PR marks all tests that did not end as canceled and reports when the process is terminated

CC @tniessen@benjamingr @nodejs/test_runner

[MoLow]

Cc @cjihrig

[aduh95]

/cc @nodejs/test_runner

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[tniessen]

Sorry, I didn't have time to review this. Looks like I cannot cancel synchronous tests using SIGINT (ctrl+c) anymore with this change 😕

[MoLow]

Sorry, I didn't have time to review this. Looks like I cannot cancel synchronous tests using SIGINT (ctrl+c) anymore with this change 😕

@tniessen can you help me reproduce? I assume your test is not blocking the eventloop?

[tniessen]

I assume your test is not blocking the eventloop?

It is, that's why I said "synchronous tests". But without a SIGINT handler, it should still be possible to ctrl+c the script.

[MoLow]

@tniessen I am not sure there is an easy way to allow both handling when the process is blocked and reporting the test tree on termination.
any solution I can think of (workers, vm) has it penalties

[tniessen]

I am not sure there is an easy way to allow both handling when the process is blocked and reporting the test tree on termination.

@MoLow But "reporting the test tree on termination" does not seem to work either, at least not when the event loop is blocked. If I put while(1) in a test, the test_runner ignores SIGINT and SIGTERM with this PR and the new timeout option does not help either, so I have to forcefully terminate the process using SIGKILL, in which case reporting does not work.

any solution I can think of (workers, vm) has it penalties

Yes, my concern here is very similar to some of my concerns in #43490, which also seem difficult to address within the same application thread.

[MoLow]

@tniessen IL give it a few more days of think.
My main concern is ending up with invalid tap output. Your example suffers from that as well.

Like other discussions, I'd be glad if it was configurable, Wich raises some questions about what the default should be and what would be the API for global configuration.

[benjamingr]

One thing to explore is adding the handler in C++ land, but even if we do that the core issue is that the test JS and the test framework are running in the same context on the same isolate so a while(1) would freeze it (and all the objects of the report live there too).

I'm not sure the infinite loop case is worth porting the test runner logic to C++ or spinning up a worker either.

[MoLow]

My biggest concern here is that an ungraceful exit will result in invalid/incomplete TAP output,
I think the default should be graceful exit is on - and add the ability to turn it off.
@tniessen does that sound ok for you?

[tniessen]

I think the default should be graceful exit is on - and add the ability to turn it off.
@tniessen does that sound ok for you?

I don't think I would call the current behavior graceful given that it makes ctrl+c / SIGINT / kill / SIGTERM unreliable.

As a user, I think a test runner should be simple and reliable, and it should only interfere with my script and workflow where that is absolutely necessary or a clear improvement, at least by default. Fancy features are nice if and only if they are reliable and don't introduce flakiness or disturb my workflow (which is also why I was against short default timeouts in #43490). But that's just my personal opinion and there are far more knowledgeable people involved.

[Linkgoron]

IMO if you ctrl+c I don't think Node should offer a "graceful exit" solution if it's at the cost of actually killing the process in some cases.

[MoLow]

I don't think I would call the current behavior graceful given that it makes ctrl+c / SIGINT / kill / SIGTERM unreliable.

the alternative is making the TAP output unreliable - which I think is much more important and more users expect to be valid

[tniessen]

My biggest concern here is that an ungraceful exit will result in invalid/incomplete TAP output,

I don't think I would call the current behavior graceful given that it makes ctrl+c / SIGINT / kill / SIGTERM unreliable.

the alternative is making the TAP output unreliable - which I think is much more important and more users expect to be valid

@MoLow Could you elaborate on this? When would not catching SIGINT / SIGKILL make TAP output unreliable in a way that is relevant? I happily use tape, which does not interfere with signals at all, as far as I know.

[MoLow]

@tniessen here is an example:

./node<<EOFconsttest=require('node:test');test('finite test',()=>{});test('infinite test',(t,done)=>setTimeout(done,1000000000));EOF

when hitting ctr+c on this program - it produces a valid TAP output only when the SIGINT handler is attached

BTW, this tape example also results in a truncated TAP output:

./node<<EOFconsttest=require('tape');test('finite test',(t)=>t.end());test('infinite test',async(t)=>{t.plan(1)returnnewPromise(r=>setTimeout(r,1000000000))});EOF

[ljharb]

I don’t think anyone has ever filed an issue on tape about this. If you hit control-C, why would you have any expectation of valid output? Test suites (the user code) aren’t generally cleanly cancellable anyways.

[MoLow]

I don’t think anyone has ever filed an issue on tape about this. If you hit control-C, why would you have any expectation of valid output? Test suites (the user code) aren’t generally cleanly cancellable anyways.

it doesnt have to be ctr+c, it can also be a ci system killing the process

[ljharb]

Right - I’m saying that valid output is only expected when the process completes normally.

[MoLow]

Discussed this with @Linkgoron - I will open a PR to make this opt-in

[tniessen]

the test JS and the test framework are running in the same context on the same isolate so a while(1) would freeze it

I just learned (thanks to #43941 (comment)) that the CLI test runner starts tests as child processes. That should allow us to work around this restriction (albeit not if a test runs as node my-test.js). This could allow useful timeouts and proper handling of signals.

Edit: this appears to have been implemented at least in part in #43977.