test_runner: better error handing for test hook #52401

himself65 · 2024-04-07T07:56:26Z

Fixes: #52399

nodejs-github-bot · 2024-04-07T07:56:31Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/harness.js

test/parallel/test-runner-extraneous-async-activity.js

test/fixtures/test-runner/async-error-in-test-hook.mjs

lib/internal/test_runner/harness.js

atlowChemi · 2024-04-09T02:37:27Z

test/fixtures/test-runner/output/junit_reporter.snapshot

@@ -514,13 +514,13 @@ Error [ERR_TEST_FAILURE]: test could not be started because its parent finished
 }
 		</failure>
 	</testcase>
-	<!-- Warning: Test "unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. -->
-	<!-- Warning: Test "async unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. -->
+	<!-- Warning: Test "unhandled rejection - passes but warns" at test/fixtures/test-runner/output/output.js:72:1 generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event. -->


Probably should also have the replaceTestLocationLine transformer?

I am ok with these being a number. it shouldn't change much

atlowChemi · 2024-04-09T02:37:45Z

test/fixtures/test-runner/output/spec_reporter.snapshot

@@ -297,13 +297,13 @@
 invalid subtest fail (*ms)
  'test could not be started because its parent finished'

- Warning: Test "unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.
- Warning: Test "async unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.
+ Warning: Test "unhandled rejection - passes but warns" at test/fixtures/test-runner/output/output.js:72:1 generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.


atlowChemi · 2024-04-09T02:37:58Z

test/fixtures/test-runner/output/spec_reporter_cli.snapshot

@@ -297,13 +297,13 @@
 invalid subtest fail (*ms)
  'test could not be started because its parent finished'

- Warning: Test "unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.
- Warning: Test "async unhandled rejection - passes but warns" generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.
+ Warning: Test "unhandled rejection - passes but warns" at test/fixtures/test-runner/output/output.js:72:1 generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.


atlowChemi · 2024-04-09T02:39:24Z

test/parallel/test-runner-extraneous-async-activity.js

@@ -10,7 +10,7 @@ const { spawnSync } = require('child_process');
    fixtures.path('test-runner', 'extraneous_set_immediate_async.mjs'),
  ]);
  const stdout = child.stdout.toString();
-  assert.match(stdout, /^# Warning: Test "extraneous async activity test" generated asynchronous activity after the test ended/m);
+  assert.match(stdout, /^# Warning: Test "extraneous async activity test" at .+extraneous_set_immediate_async\.mjs:3:1 generated asynchronous activity after the test ended/m);


Suggested change

assert.match(stdout, /^# Warning: Test "extraneous async activity test" at .+extraneous_set_immediate_async\.mjs:3:1 generated asynchronous activity after the test ended/m);

assert.match(stdout, /^# Warning: Test "extraneous async activity test" at .+extraneous_set_immediate_async\.mjs:\d+:\d+ generated asynchronous activity after the test ended/m);

(same for rest of the file)

I am ok with this being a number. it shouldn't change much

We can leave col/row number here, because fixtures are won't change too much, it's good to keep it here

MoLow

Nice improvement

lib/internal/test_runner/harness.js

cjihrig · 2024-04-09T13:24:57Z

lib/internal/test_runner/harness.js

-        msg = `Warning: Test "${test.name}" generated asynchronous ` +
+        const name = test instanceof TestHook ? `Test Hook "${test.hookType}"` : `Test "${test.name}"`;
+        const relPath = relative(process.cwd(), test.loc.file);
+        msg = `Warning: ${name} at ${relPath}:${test.loc.line}:${test.loc.column} generated asynchronous ` +
          'activity after the test ended. This activity created the error ' +
          `"${err}" and would have caused the test to fail, but instead ` +
          `triggered an ${eventName} event.`;


There is another warning on line 70 that we should change to an error. We should also remove the part that says "after the test ended" since we have seen that it can happen before a test even starts.

We should also remove the part that says "after the test ended" since we have seen that it can happen before a test even starts.

I will keep this comment because it seems out of the context of this PR. And perhaps we can open another PR and add such test case

lib/internal/test_runner/harness.js

cjihrig

LGTM. Thanks, I think this is a nice change.

nodejs-github-bot · 2024-04-10T18:51:23Z

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

nodejs-github-bot · 2024-04-11T00:25:57Z

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

atlowChemi · 2024-04-11T05:33:09Z

@himself65 CI seems to have failures related to the snapshots

himself65 · 2024-04-11T07:13:50Z

@himself65 CI seems to have failures related to the snapshots

How to fix it?

MoLow · 2024-04-11T07:20:42Z

see https://ci.nodejs.org/job/node-test-binary-windows-js-suites/27045/RUN_SUBSET=1,nodes=win11-COMPILED_BY-vs2022/console for example:

07:37:37             +   ' Error: Test "unhandled rejection - passes but warns" at test\\fixtures\\test-runner\\output\\output.js:72:1 generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.\n' +
07:37:37             +   ' Error: Test "async unhandled rejection - passes but warns" at test\\fixtures\\test-runner\\output\\output.js:76:1 generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.\n' +
07:37:37             -   ' Error: Test "unhandled rejection - passes but warns" at test/fixtures/test-runner/output/output.js:72:1 generated asynchronous activity after the test ended. This activity created the error "Error: rejected from unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.\n' +
07:37:37             -   ' Error: Test "async unhandled rejection - passes but warns" at test/fixtures/test-runner/output/output.js:76:1 generated asynchronous activity after the test ended. This activity created the error "Error: rejected from async unhandled rejection fail" and would have caused the test to fail, but instead triggered an unhandledRejection event.\n' +

snapshot.replaceWindowsPaths needs to be added to junitTransform and to specTransform. if you need help LMK and I will push that to this branch

atlowChemi · 2024-04-11T13:30:53Z

@himself65 CI seems to have failures related to the snapshots

How to fix it?

Once you fix the transfrormers @MoLow mentioned, run the following command to update the snapshots:
NODE_REGENERATE_SNAPSHOTS=1 out/Release/node test/parallel/test-runner-output.mjs. see:

node/test/common/README.md

Lines 690 to 694 in 5b33f9d

    
           ### `NODE_REGENERATE_SNAPSHOTS` 
        
           If set, test snapshots for a the current test are regenerated. 
        
           for example `NODE_REGENERATE_SNAPSHOTS=1 out/Release/node test/parallel/test-runner-output.mjs` 
        
           will update all the test runner output snapshots.

himself65 · 2024-04-14T00:34:57Z

working on this now

nodejs-github-bot · 2024-04-14T21:52:57Z

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

Co-authored-by: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot · 2024-04-14T23:36:44Z

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

nodejs-github-bot · 2024-04-15T02:41:42Z

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

nodejs-github-bot · 2024-04-15T04:44:14Z

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

nodejs-github-bot · 2024-04-15T06:11:21Z

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

nodejs-github-bot · 2024-04-15T07:42:58Z

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

nodejs-github-bot · 2024-04-15T10:18:21Z

Commit Queue failed

- Loading data for nodejs/node/pull/52401
✔  Done loading data for nodejs/node/pull/52401
----------------------------------- PR info ------------------------------------
Title      test_runner: better error handing for test hook (#52401)
Author     Alex Yang  (@himself65)
Branch     himself65:himself65/20240407/better-error-handler -> nodejs:main
Labels     needs-ci, test_runner
Commits    1
 - test_runner: better error handing for test hook
Committers 1
 - Alex Yang 
PR-URL: https://github.com/nodejs/node/pull/52401
Fixes: https://github.com/nodejs/node/issues/52399
Reviewed-By: Moshe Atlow 
Reviewed-By: Chemi Atlow 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Colin Ihrig 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/52401
Fixes: https://github.com/nodejs/node/issues/52399
Reviewed-By: Moshe Atlow 
Reviewed-By: Chemi Atlow 
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Colin Ihrig 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - test_runner: better error handing for test hook
   ℹ  This PR was created on Sun, 07 Apr 2024 07:56:26 GMT
   ✔  Approvals: 4
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/52401#pullrequestreview-1990080998
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/52401#pullrequestreview-1990786703
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/52401#pullrequestreview-1990921524
   ✔  - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/52401#pullrequestreview-1991730905
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-04-15T07:42:58Z: https://ci.nodejs.org/job/node-test-pull-request/58391/
- Querying data for job/node-test-pull-request/58391/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/8687402947

nodejs-github-bot · 2024-04-15T10:27:19Z

Landed in f098b7a

Co-authored-by: Colin Ihrig <cjihrig@gmail.com> PR-URL: #52401 Fixes: #52399 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 Apr 7, 2024

himself65 marked this pull request as ready for review April 7, 2024 09:18

atlowChemi reviewed Apr 7, 2024

View reviewed changes

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

test/parallel/test-runner-extraneous-async-activity.js Outdated Show resolved Hide resolved

test/fixtures/test-runner/async-error-in-test-hook.mjs Show resolved Hide resolved

MoLow reviewed Apr 8, 2024

View reviewed changes

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

himself65 requested a review from MoLow April 8, 2024 19:42

himself65 force-pushed the himself65/20240407/better-error-handler branch 2 times, most recently from be37e0d to 0e35b75 Compare April 9, 2024 00:45

atlowChemi reviewed Apr 9, 2024

View reviewed changes

MoLow approved these changes Apr 9, 2024

View reviewed changes

cjihrig reviewed Apr 9, 2024

View reviewed changes

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

MoLow approved these changes Apr 9, 2024

View reviewed changes

himself65 requested a review from cjihrig April 9, 2024 21:27

himself65 changed the title ~~test_runner: better warning inside test hook~~ test_runner: better error handing for test hook Apr 9, 2024

atlowChemi approved these changes Apr 10, 2024

View reviewed changes

benjamingr approved these changes Apr 10, 2024

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024

cjihrig approved these changes Apr 10, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 10, 2024

himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 11, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 11, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2024

himself65 force-pushed the himself65/20240407/better-error-handler branch from 074683d to 16f2eef Compare April 14, 2024 23:02

test_runner: better error handing for test hook

ebaa222

Co-authored-by: Colin Ihrig <cjihrig@gmail.com>

himself65 force-pushed the himself65/20240407/better-error-handler branch from 16f2eef to ebaa222 Compare April 14, 2024 23:02

himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 14, 2024

himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 15, 2024

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2024

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 Apr 15, 2024

MoLow approved these changes Apr 15, 2024

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 Apr 15, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 15, 2024

nodejs-github-bot merged commit f098b7a into nodejs:main Apr 15, 2024
67 checks passed

himself65 deleted the himself65/20240407/better-error-handler branch April 22, 2024 16:33

aduh95 mentioned this pull request Apr 30, 2024

2024-05-02, Version 22.1.0 (Current) #52768

Merged

marco-ippolito mentioned this pull request May 2, 2024

v20.13.0 proposal #52793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: better error handing for test hook #52401

test_runner: better error handing for test hook #52401

himself65 commented Apr 7, 2024

nodejs-github-bot commented Apr 7, 2024

atlowChemi Apr 9, 2024

MoLow Apr 9, 2024

atlowChemi Apr 9, 2024

atlowChemi Apr 9, 2024

atlowChemi Apr 9, 2024

MoLow Apr 9, 2024

himself65 Apr 9, 2024

MoLow left a comment

cjihrig Apr 9, 2024

himself65 Apr 9, 2024 •

edited

Loading

cjihrig left a comment

nodejs-github-bot commented Apr 10, 2024

nodejs-github-bot commented Apr 11, 2024

atlowChemi commented Apr 11, 2024

himself65 commented Apr 11, 2024

MoLow commented Apr 11, 2024

atlowChemi commented Apr 11, 2024

himself65 commented Apr 14, 2024

nodejs-github-bot commented Apr 14, 2024

nodejs-github-bot commented Apr 14, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

	assert.match(stdout, /^# Warning: Test "extraneous async activity test" at .+extraneous_set_immediate_async\.mjs:3:1 generated asynchronous activity after the test ended/m);
	assert.match(stdout, /^# Warning: Test "extraneous async activity test" at .+extraneous_set_immediate_async\.mjs:\d+:\d+ generated asynchronous activity after the test ended/m);

test_runner: better error handing for test hook #52401

test_runner: better error handing for test hook #52401

Conversation

himself65 commented Apr 7, 2024

nodejs-github-bot commented Apr 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoLow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himself65 Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 10, 2024

nodejs-github-bot commented Apr 11, 2024

atlowChemi commented Apr 11, 2024

himself65 commented Apr 11, 2024

MoLow commented Apr 11, 2024

atlowChemi commented Apr 11, 2024

himself65 commented Apr 14, 2024

nodejs-github-bot commented Apr 14, 2024

nodejs-github-bot commented Apr 14, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

nodejs-github-bot commented Apr 15, 2024

himself65 Apr 9, 2024 •

edited

Loading