GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: call abort on test finish by rluvaton · Pull Request #48827 · nodejs/node

Conversation

@rluvaton

@rluvaton rluvaton commented Jul 18, 2023

Copy link
Copy Markdown
Member

Fix #48736 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji jaydenseric reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction
rluvaton added 2 commits July 18, 2023 17:21
@rluvaton
test_runner: call abort on test finish
21a9e6d
@rluvaton
format
0ed4cb4
@nodejs-github-bot

nodejs-github-bot commented Jul 18, 2023 •
edited by MoLow
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton
format
c4710a8
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 18, 2023
@rluvaton
rluvaton marked this pull request as ready for review July 18, 2023 14:24
rluvaton
rluvaton commented Jul 18, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr approved these changes Jul 18, 2023
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
MoLow
MoLow previously requested changes Jul 18, 2023
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig and benjamingr reacted with hooray emoji rluvaton reacted with rocket emoji
All reactions
  • 🎉 2 reactions
  • 🚀 1 reaction
MoLow
MoLow reviewed Jul 18, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton
do not recreate on each test
eeabcf5
MoLow
MoLow reviewed Jul 18, 2023
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

MoLow
MoLow reviewed Jul 18, 2023
View reviewed changes
Comment thread test/parallel/test-runner-run.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton
cleanup
f93f38e
@MoLow
MoLow dismissed their stale review July 18, 2023 15:38

requested changes addressed

MoLow
MoLow reviewed Jul 18, 2023
View reviewed changes

@MoLow MoLow left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
rluvaton added 2 commits July 18, 2023 18:40
@rluvaton
cleanup
5017fe5
@rluvaton
cleanup
58da4d2
MoLow
MoLow reviewed Jul 18, 2023
View reviewed changes
Comment thread test/parallel/test-runner-run.mjs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@benjamingr

benjamingr commented Jul 19, 2023

Copy link
Copy Markdown
Member

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

@rluvaton let's do that?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton
abort after all cleanup
f5f36b6
@rluvaton

rluvaton commented Jul 19, 2023

Copy link
Copy Markdown
Member Author

fixed

benjamingr and MoLow reacted with heart emoji
All reactions
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr
benjamingr approved these changes Jul 19, 2023
View reviewed changes
MoLow
MoLow reviewed Jul 19, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/aborts/wait-for-abort-helper.js Resolved

Uh oh!

There was an error while loading. Please reload this page.

MoLow
MoLow reviewed Jul 19, 2023
View reviewed changes
Comment thread test/fixtures/test-runner/aborts/wait-for-abort-helper.js
@@ -0,0 +1,19 @@
module.exports = {
waitForAbort: function ({ testNumber, signal }) {

@MoLow MoLow Jul 19, 2023

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@rluvaton rluvaton Jul 19, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@rluvaton rluvaton Jul 19, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for 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.

Uh oh!

There was an error while loading. Please reload this page.

MoLow reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@rluvaton @MoLow
Update test/fixtures/test-runner/aborts/wait-for-abort-helper.js
0c7c812
Co-authored-by: Moshe Atlow <[email protected]>
MoLow
MoLow approved these changes Jul 20, 2023
View reviewed changes
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 20, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jul 20, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 24c3d8a into nodejs:main Jul 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in 24c3d8a

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton

rluvaton commented Jul 21, 2023

Copy link
Copy Markdown
Member Author

This need to be backported, right?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@atlowChemi

atlowChemi commented Jul 21, 2023

Copy link
Copy Markdown
Member

This need to be backported, right?

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton
rluvaton deleted the call-abort-on-test-finish branch July 21, 2023 13:06
@aduh95

aduh95 commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

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

MoLow and atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rluvaton

rluvaton commented Jul 21, 2023

Copy link
Copy Markdown
Member Author

I actually don't really know 😅

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Jul 22, 2023
CI Reliability 2023-07-22 nodejs/reliability#620
Open
12 tasks
@MoLow

MoLow commented Jul 22, 2023

Copy link
Copy Markdown
Member

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

benjamingr, rluvaton, and atlowChemi reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot mentioned this pull request Jul 23, 2023
CI Reliability 2023-07-23 nodejs/reliability#621
Open
12 tasks
@ghost ghost mentioned this pull request Jul 23, 2023
Test runner executes after() in declared order #48736
Closed
@github-actions github-actions Bot mentioned this pull request Jul 24, 2023
CI Reliability 2023-07-24 nodejs/reliability#622
Open
12 tasks
@github-actions github-actions Bot mentioned this pull request Jul 25, 2023
CI Reliability 2023-07-25 nodejs/reliability#623
Open
14 tasks
@rluvaton rluvaton mentioned this pull request Jul 25, 2023
test_runner: fix timeout in *Each hook failing further tests #48925
Merged
This was referenced Jul 26, 2023
CI Reliability 2023-07-26 nodejs/reliability#624
Open
CI Reliability 2023-07-27 nodejs/reliability#625
Open
CI Reliability 2023-07-28 nodejs/reliability#626
Open
CI Reliability 2023-07-29 nodejs/reliability#627
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@rluvaton @Ceres6
test_runner: call abort on test finish
352e9e3
PR-URL: nodejs#48827
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@rluvaton @Ceres6
test_runner: call abort on test finish
a488284
PR-URL: nodejs#48827
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
@rluvaton @UlisesGascon
test_runner: call abort on test finish
9ce2b19
PR-URL: nodejs#48827
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
@rluvaton @RafaelGSS
test_runner: call abort on test finish
41058be
PR-URL: #48827
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
v20.6.0 proposal #49185
Merged
targos pushed a commit that referenced this pull request Oct 28, 2023
@rluvaton @targos
test_runner: call abort on test finish
2d4511b
PR-URL: #48827
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: call abort on test finish
4edd778
PR-URL: nodejs/node#48827
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
test_runner: call abort on test finish
1f26fff
PR-URL: nodejs/node#48827
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@MoLow MoLow MoLow approved these changes
@atlowChemi atlowChemi atlowChemi approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Test runner executes after() in declared order

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@rluvaton @nodejs-github-bot @benjamingr @atlowChemi @aduh95 @MoLow
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.