test_runner: call abort on test finish

[rluvaton]

Fix #48736 (comment)

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[benjamingr]

Generally looks good, let's make sure to run benchmarks before landing because we recreate the controller each time now

[MoLow]

I was able to avoid recreation of the controller by testing for this.parent, wich is the case of hooks and the root test:

diff --git a/lib/internal/test_runner/test.js b/lib/internal/test_runner/test.js
index cc56eec948..b81fc80bbf 100644
--- a/lib/internal/test_runner/test.js
+++ b/lib/internal/test_runner/test.js
@@ -586,13 +586,17 @@ class Test extends AsyncResource {
         return;
       }
 
-      this.#abortController.abort();
 
+      if (this.parent !== null) {
+        this.#abortController.abort();
+      }
       await afterEach();
       await after();
       this.pass();
     } catch (err) {
-      this.#abortController.abort();
+      if (this.parent !== null) {
+        this.#abortController.abort();
+      }
       try { await afterEach(); } catch { /* test is already failing, let's ignore the error */ }
       try { await after(); } catch { /* Ignore error. */ }
       if (isTestFailureError(err)) {
@@ -747,10 +751,6 @@ class Test extends AsyncResource {
     this.reporter.start(this.nesting, kFilename, this.name);
   }
 
-  recreateAbortController() {
-    this.#abortController = new AbortController();
-    this.signal = this.#abortController.signal;
-  }
 }
 
 class TestHook extends Test {
@@ -773,8 +773,6 @@ class TestHook extends Test {
     return true;
   }
   postRun() {
-    // Need to recreate the abort controller because we abort each time in the end
-    super.recreateAbortController();
   }
 }

requested changes addressed

[MoLow]

dismissed changes request, but I would still prefer if the abort is done after hooks run

[benjamingr]

dismissed changes request, but I would still prefer if the abort is done after hooks run

@rluvaton let's do that?

[rluvaton]

fixed

[MoLow]

why not await Promise.race(once(signal, 'abort'), setTimeout(1000)?

[rluvaton]

Because we wait for the afterEach in the test runner...

node/lib/internal/test_runner/test.js

Lines 589 to 590 in f5f36b6

	await afterEach();
	await after();

[rluvaton]

so doing await will always fail...

[nodejs-github-bot]

Landed in 24c3d8a

[rluvaton]

This need to be backported, right?

[atlowChemi]

This need to be backported, right?

I can backport to v20 at the beginning of next week 🙂

[aduh95]

This shouldn't need any manual backport, or am I missing something?

[rluvaton]

I actually don't really know 😅

[MoLow]

Backporting is only needed when there is a git conflict, or if tests break or some other reason like that