test_runner: fix afterEach not running on test failures#45204

MrJithil · 2022-10-27T09:31:19Z

test_runner: fix afterEach not running on test failures

MoLow

In general LGTM, a few comments:

describe should be adjusted as well and moved to finally:
node/lib/internal/test_runner/test.js
Lines 749 to 753 in 789b7ee
awaitthis[kRunHook]('after',hookArgs);
if(this.parent?.hooks.afterEach.length>0){
awaitthis.parent[kRunHook]('afterEach',hookArgs);
}
tests need to be added/adjusted

MrJithil · 2022-10-27T12:04:37Z

In general LGTM, a few comments:
describe should be adjusted as well and moved to finally:
node/lib/internal/test_runner/test.js
Lines 749 to 753 in 789b7ee
awaitthis[kRunHook]('after',hookArgs);
if(this.parent?.hooks.afterEach.length>0){
awaitthis.parent[kRunHook]('afterEach',hookArgs);
}
tests need to be added/adjusted

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

MoLow · 2022-10-27T13:37:25Z

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

checkout the failing tests

MrJithil · 2022-10-30T11:41:39Z

Working on it.

MrJithil · 2022-11-05T03:01:19Z

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.

MoLow · 2022-11-05T17:52:33Z

@MrJithil I took a look.
prior to this PR, tests was marked as passed only in case the afterEach hook passed,

node/lib/internal/test_runner/test.js

Line 521 in 789b7ee

this.pass();

after this PR, if the afterEach hook failed there is no indication besides the parent test failing:
see the output for these tests:

node/test/message/test_runner_hooks.js

Lines 73 to 77 in 789b7ee

describe('afterEach throws',()=>{
afterEach(()=>{thrownewError('afterEach');});
it('1',()=>{});
it('2',()=>{});
});

MoLow · 2022-11-05T18:47:37Z

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

nodejs-github-bot · 2022-11-05T21:06:57Z

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

lib/internal/test_runner/test.js

nodejs-github-bot · 2022-11-06T07:54:03Z

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

MoLow · 2022-11-06T09:08:39Z

Do we need the out files?

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

MrJithil · 2022-11-06T09:53:11Z

@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?

nodejs-github-bot · 2022-11-06T10:14:35Z

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

nodejs-github-bot · 2022-11-06T11:51:01Z

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

lib/internal/test_runner/test.js

test_runner: fix afterEach not running on test failures

nodejs-github-bot · 2022-11-07T03:50:10Z

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

nodejs-github-bot · 2022-11-07T05:42:58Z

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

nodejs-github-bot · 2022-11-07T05:58:41Z

Landed in 3759935

test_runner: fix afterEach not running on test failures PR-URL: nodejs#45204Fixes: nodejs#45192 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

test_runner: fix afterEach not running on test failures PR-URL: #45204Fixes: #45192 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

test_runner: fix afterEach not running on test failures PR-URL: nodejs#45204Fixes: nodejs#45192 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

test_runner: fix afterEach not running on test failures PR-URL: #45204Fixes: #45192 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>

test_runner: fix afterEach not running on test failures PR-URL: nodejs/node#45204Fixes: nodejs/node#45192 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> (cherry picked from commit 3759935ee29d8042d917d3ceaa768521c14413ff)

PR-URL: nodejs/node#45204Fixes: nodejs/node#45192 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> (cherry picked from commit 3759935ee29d8042d917d3ceaa768521c14413ff)

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 reviewed Oct 27, 2022
View reviewed changes

benjamingr approved these changes Nov 5, 2022
View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2022

cjihrig reviewed Nov 5, 2022
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

aduh95 reviewed Nov 5, 2022
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved
lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2022

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2022

MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 6, 2022

aduh95 reviewed Nov 6, 2022
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

MrJithiland others added 5 commits November 6, 2022 22:30

test_runner: fix afterEach not running on test failures
14af24b
test_runner: fix afterEach not running on test failures

fix
30268cd

CR
c17bb82

CR
a9defa0

CR
5880f19

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 7, 2022

MoLow approved these changes Nov 7, 2022
View reviewed changes

MoLow added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 7, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 7, 2022

nodejs-github-bot merged commit 3759935 into nodejs:mainNov 7, 2022

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-actionsbot mentioned this pull request Nov 10, 2022
CI Reliability 2022-11-10 nodejs/reliability#424
Open
20 tasks

RafaelGSS mentioned this pull request Nov 10, 2022
v19.1.0 proposal #45269
Merged

MoLow mentioned this pull request Nov 24, 2022
[v16.x backport] backport multiple test runner features #45602
Closed

danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged

	awaitthis[kRunHook]('after',hookArgs);

	if(this.parent?.hooks.afterEach.length>0){
	awaitthis.parent[kRunHook]('afterEach',hookArgs);
	}

Uh oh!

test_runner: fix afterEach not running on test failures#45204

test_runner: fix afterEach not running on test failures #45204

Uh oh!

Conversation

MrJithil commented Oct 27, 2022• edited by MoLow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

MrJithil commented Oct 27, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow commented Oct 27, 2022

Uh oh!

MrJithil commented Oct 30, 2022

Uh oh!

MrJithil commented Nov 5, 2022

Uh oh!

MoLow commented Nov 5, 2022

Uh oh!

MoLow commented Nov 5, 2022

Uh oh!

nodejs-github-bot commented Nov 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 6, 2022

Uh oh!

MoLow commented Nov 6, 2022

Uh oh!

MrJithil commented Nov 6, 2022

Uh oh!

nodejs-github-bot commented Nov 6, 2022

Uh oh!

nodejs-github-bot commented Nov 6, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 7, 2022

Uh oh!

nodejs-github-bot commented Nov 7, 2022

Uh oh!

nodejs-github-bot commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MrJithil commented Oct 27, 2022•
edited by MoLow
Loading

MrJithil commented Oct 27, 2022•
edited
Loading