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:

Lines 749 to 753 in 789b7ee

    
           await this[kRunHook]('after', hookArgs); 
        
           if (this.parent?.hooks.afterEach.length > 0) { 
        
             await this.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

await this[kRunHook]('after', hookArgs);

if (this.parent?.hooks.afterEach.length > 0) {

await this.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(() => { throw new Error('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#45204 Fixes: nodejs#45192 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

test_runner: fix afterEach not running on test failures PR-URL: #45204 Fixes: #45192 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

test_runner: fix afterEach not running on test failures PR-URL: nodejs#45204 Fixes: nodejs#45192 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

test_runner: fix afterEach not running on test failures PR-URL: #45204 Fixes: #45192 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

test_runner: fix afterEach not running on test failures PR-URL: nodejs/node#45204 Fixes: nodejs/node#45192 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> (cherry picked from commit 3759935ee29d8042d917d3ceaa768521c14413ff)

PR-URL: nodejs/node#45204 Fixes: nodejs/node#45192 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> (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-actions bot 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 resolved Hide resolved

aduh95 reviewed Nov 5, 2022

View reviewed changes

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

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

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

github-actions bot 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 resolved Hide resolved

MrJithil and 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:main Nov 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-actions bot 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: fix afterEach not running on test failures #45204

test_runner: fix afterEach not running on test failures #45204

MrJithil commented Oct 27, 2022 •

edited by MoLow

Loading

MoLow left a comment

MrJithil commented Oct 27, 2022 •

edited

Loading

MoLow commented Oct 27, 2022

MrJithil commented Oct 30, 2022

MrJithil commented Nov 5, 2022

MoLow commented Nov 5, 2022

MoLow commented Nov 5, 2022

nodejs-github-bot commented Nov 5, 2022

nodejs-github-bot commented Nov 6, 2022

MoLow commented Nov 6, 2022

MrJithil commented Nov 6, 2022

nodejs-github-bot commented Nov 6, 2022

nodejs-github-bot commented Nov 6, 2022

nodejs-github-bot commented Nov 7, 2022

nodejs-github-bot commented Nov 7, 2022

nodejs-github-bot commented Nov 7, 2022

	await this[kRunHook]('after', hookArgs);

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

test_runner: fix afterEach not running on test failures #45204

test_runner: fix afterEach not running on test failures #45204

Conversation

MrJithil commented Oct 27, 2022 • edited by MoLow Loading

MoLow left a comment

Choose a reason for hiding this comment

MrJithil commented Oct 27, 2022 • edited Loading

MoLow commented Oct 27, 2022

MrJithil commented Oct 30, 2022

MrJithil commented Nov 5, 2022

MoLow commented Nov 5, 2022

MoLow commented Nov 5, 2022

nodejs-github-bot commented Nov 5, 2022

nodejs-github-bot commented Nov 6, 2022

MoLow commented Nov 6, 2022

MrJithil commented Nov 6, 2022

nodejs-github-bot commented Nov 6, 2022

nodejs-github-bot commented Nov 6, 2022

nodejs-github-bot commented Nov 7, 2022

nodejs-github-bot commented Nov 7, 2022

nodejs-github-bot commented Nov 7, 2022

MrJithil commented Oct 27, 2022 •

edited by MoLow

Loading

MrJithil commented Oct 27, 2022 •

edited

Loading