test_runner: cancel on termination #43549
Conversation
|
Cc @cjihrig |
|
/cc @nodejs/test_runner |
c37ca32 to
9f56f6f
Compare
ca3ad9a to
db19e9a
Compare
7bf3ba6 to
e50bff8
Compare
PR-URL: nodejs/node#43549 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#43549 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #43549 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
|
Sorry, I didn't have time to review this. Looks like I cannot cancel synchronous tests using |
@tniessen can you help me reproduce? I assume your test is not blocking the eventloop? |
It is, that's why I said "synchronous tests". But without a |
|
@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
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. |
|
@tniessen IL give it a few more days of think. 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. |
|
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 I'm not sure the infinite loop case is worth porting the test runner logic to C++ or spinning up a worker either. |
|
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 / 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. |
|
IMO if you |
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 |
|
@tniessen here is an example: ./node <<EOF
const test = require('node:test');
test('finite test', () => {});
test('infinite test', (t, done) => setTimeout(done, 1000000000));
EOFwhen hitting BTW, this ./node <<EOF
const test = require('tape');
test('finite test', (t) => t.end());
test('infinite test', async (t) => {
t.plan(1)
return new Promise(r => setTimeout(r, 1000000000))
});
EOF |
|
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 |
|
Right - I’m saying that valid output is only expected when the process completes normally. |
|
Discussed this with @Linkgoron - I will open a PR to make this opt-in |
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 Edit: this appears to have been implemented at least in part in #43977. |
PR-URL: #43549 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#43549 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Nitzan Uziely <linkgoron@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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