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: fix afterEach not running on test failures by MrJithil · Pull Request #45204 · nodejs/node

Conversation

@MrJithil

@MrJithil MrJithil commented Oct 27, 2022 •
edited by MoLow
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

test_runner: fix afterEach not running on test failures

Fixes: #45192

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss and tonivj5 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Oct 27, 2022
MoLow
MoLow reviewed Oct 27, 2022
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.

In general LGTM, a few comments:

  • describe should be adjusted as well and moved to finally: https://github.com/nodejs/node/blob/789b7eebff58d404ac8f060376a75e26b514f9af/lib/internal/test_runner/test.js#L749-L753
  • tests need to be added/adjusted

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MrJithil reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@MrJithil

MrJithil commented Oct 27, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

In general LGTM, a few comments:

  • describe should be adjusted as well and moved to finally: https://github.com/nodejs/node/blob/789b7eebff58d404ac8f060376a75e26b514f9af/lib/internal/test_runner/test.js#L749-L753
  • tests need to be added/adjusted

Could you please provide a test snippet or some more details?
All reactions

Sorry, something went wrong.

Uh oh!

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

MoLow commented Oct 27, 2022

Copy link
Copy Markdown
Member

Could you please provide a test snippet or some more details?

checkout the failing tests
MrJithil reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

MrJithil commented Oct 30, 2022

Copy link
Copy Markdown
Member Author

Working on it.
All reactions

Sorry, something went wrong.

Uh oh!

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

MrJithil commented Nov 5, 2022

Copy link
Copy Markdown
Member Author

help needed to fix the test case. I can see the test file is failing with the latest release as well.

I am confused, how to clean up or adjust the test case. Please provide some samples or feel free to commit to the PR.
All reactions

Sorry, something went wrong.

Uh oh!

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

MoLow commented Nov 5, 2022

Copy link
Copy Markdown
Member

@MrJithil I took a look.
prior to this PR, tests was marked as passed only in case the afterEach hook passed, https://github.com/nodejs/node/blob/789b7eebff58d404ac8f060376a75e26b514f9af/lib/internal/test_runner/test.js#L521
after this PR, if the afterEach hook failed there is no indication besides the parent test failing:
see the output for these tests:
https://github.com/nodejs/node/blob/789b7eebff58d404ac8f060376a75e26b514f9af/test/message/test_runner_hooks.js#L73-L77
image
MrJithil reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

MoLow commented Nov 5, 2022

Copy link
Copy Markdown
Member

@MrJithil I pushed a fix;
@nodejs/test_runner I would appreciate a review
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Nov 5, 2022
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 Nov 5, 2022
@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 Nov 5, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 5, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47721/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed Nov 5, 2022
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Nov 5, 2022
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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 Nov 6, 2022
@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 Nov 6, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47732/
All reactions

Sorry, something went wrong.

Uh oh!

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

MoLow commented Nov 6, 2022

Copy link
Copy Markdown
Member

Do we need the out files?

yeah, tools/test.py compares the output with the actual result
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Nov 6, 2022
@MrJithil

MrJithil commented Nov 6, 2022

Copy link
Copy Markdown
Member Author

@MrJithil I pushed a fix;
@nodejs/test_runner I would appreciate a review

Thank you so much for the help and it's a really great work. Do we need the out files?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47733/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 6, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47734/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aduh95
aduh95 reviewed Nov 6, 2022
View reviewed changes
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
MrJithil and others added 5 commits November 6, 2022 22:30
@MrJithil @MoLow
test_runner: fix afterEach not running on test failures
14af24b 
test_runner: fix afterEach not running on test failures
@MoLow
fix
30268cd
@MoLow
CR
c17bb82
@MoLow
CR
a9defa0
@MoLow
CR
5880f19
@nodejs-github-bot

nodejs-github-bot commented Nov 7, 2022

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47755/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 7, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Nov 7, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 7, 2022

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/45204
✔  Done loading data for nodejs/node/pull/45204
----------------------------------- PR info ------------------------------------
Title      test_runner: fix afterEach not running on test failures (#45204)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     MrJithil:fix-45192 -> nodejs:main
Labels     needs-ci, dont-land-on-v14.x, commit-queue-squash, test_runner
Commits    5
 - test_runner: fix afterEach not running on test failures
 - fix
 - CR
 - CR
 - CR
Committers 1
 - Moshe Atlow 
PR-URL: https://github.com/nodejs/node/pull/45204
Fixes: https://github.com/nodejs/node/issues/45192
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45204
Fixes: https://github.com/nodejs/node/issues/45192
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - test_runner: fix afterEach not running on test failures
   ⚠  - fix
   ⚠  - CR
   ⚠  - CR
   ⚠  - CR
   ℹ  This PR was created on Thu, 27 Oct 2022 09:31:19 GMT
   ✔  Approvals: 1
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/45204#pullrequestreview-1169503033
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-11-07T03:50:10Z: https://ci.nodejs.org/job/node-test-pull-request/47755/
- Querying data for job/node-test-pull-request/47755/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3407846700
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow approved these changes Nov 7, 2022
View reviewed changes
@MoLow MoLow added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Nov 7, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 7, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 3759935 into nodejs:main Nov 7, 2022
@nodejs-github-bot

nodejs-github-bot commented Nov 7, 2022

Copy link
Copy Markdown
Collaborator

Landed in 3759935
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Nov 8, 2022
CI Reliability 2022-11-08 nodejs/reliability#422
Open
CI Reliability 2022-11-09 nodejs/reliability#423
Open
@github-actions github-actions Bot mentioned this pull request Nov 10, 2022
CI Reliability 2022-11-10 nodejs/reliability#424
Open
20 tasks
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@MrJithil @RafaelGSS
test_runner: fix afterEach not running on test failures
8e7f9de 
test_runner: fix afterEach not running on test failures

PR-URL: #45204
Fixes: #45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 10, 2022
v19.1.0 proposal #45269
Merged
@MoLow MoLow mentioned this pull request Nov 24, 2022
[v16.x backport] backport multiple test runner features #45602
Closed
MoLow pushed a commit to MoLow/node that referenced this pull request Dec 9, 2022
@MrJithil @MoLow
test_runner: fix afterEach not running on test failures
94174b7 
test_runner: fix afterEach not running on test failures

PR-URL: nodejs#45204
Fixes: nodejs#45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@MrJithil @danielleadams
test_runner: fix afterEach not running on test failures
584792c 
test_runner: fix afterEach not running on test failures

PR-URL: #45204
Fixes: #45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@MrJithil @danielleadams
test_runner: fix afterEach not running on test failures
0f69b6c 
test_runner: fix afterEach not running on test failures

PR-URL: #45204
Fixes: #45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
@MrJithil @MoLow
test_runner: fix afterEach not running on test failures
3786f2e 
test_runner: fix afterEach not running on test failures

PR-URL: nodejs/node#45204
Fixes: nodejs/node#45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit 3759935ee29d8042d917d3ceaa768521c14413ff)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
@MrJithil @MoLow
fix: fix afterEach not running on test failures
68ef3a3 
PR-URL: nodejs/node#45204
Fixes: nodejs/node#45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit 3759935ee29d8042d917d3ceaa768521c14413ff)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
@MrJithil @MoLow
fix: fix afterEach not running on test failures
c01d739 
PR-URL: nodejs/node#45204
Fixes: nodejs/node#45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit 3759935ee29d8042d917d3ceaa768521c14413ff)
MoLow pushed a commit to MoLow/node-core-test that referenced this pull request Feb 2, 2023
@MrJithil @MoLow
fix: fix afterEach not running on test failures
f2815af 
PR-URL: nodejs/node#45204
Fixes: nodejs/node#45192
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
(cherry picked from commit 3759935ee29d8042d917d3ceaa768521c14413ff)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cjihrig cjihrig cjihrig left review comments
@aduh95 aduh95 aduh95 left review comments
@benjamingr benjamingr benjamingr approved these changes
@MoLow MoLow MoLow 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.

afterEach hook not executed when test fails

Uh oh!

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

6 participants

@MrJithil @MoLow @nodejs-github-bot @benjamingr @cjihrig @aduh95
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.