GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||
GeezerVison is showing github.com — test_runner: call abort on test finish by rluvaton · Pull Request #48827 · nodejs/node Conversation
Member
Fix #48736 (comment)
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
| |||||||||||||||||||||||
There was an error while loading. Please reload this page.
|
Review requested:
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, let's make sure to run benchmarks before landing because we recreate the controller each time now
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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(); } }
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
dismissed changes request, but I would still prefer if the abort is done after hooks run
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
@rluvaton let's do that? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
fixed |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,19 @@ | |||
| module.exports = { | |||
| waitForAbort: function ({ testNumber, signal }) { | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
why not await Promise.race(once(signal, 'abort'), setTimeout(1000)?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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(); |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
so doing await will always fail...
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Moshe Atlow <[email protected]>
|
Landed in 24c3d8a |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This need to be backported, right? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
I can backport to v20 at the beginning of next week 🙂 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This shouldn't need any manual backport, or am I missing something? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I actually don't really know 😅 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Backporting is only needed when there is a git conflict, or if tests break or some other reason like that |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: nodejs#48827 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
PR-URL: nodejs#48827 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
PR-URL: nodejs#48827 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
PR-URL: #48827 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
PR-URL: #48827 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
PR-URL: nodejs/node#48827 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
PR-URL: nodejs/node#48827 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Test runner executes after() in declared order
There was an error while loading. Please reload this page.