test_runner: differentiate test types in enqueue dequeue events #54049

EddieAbbondanzio · 2024-07-26T02:59:40Z

The test:enqueue and test:dequeue events now include a type in details to allow reporters to distinguish if it was a suite or test.

Fixes #51235

nodejs-github-bot · 2024-07-26T02:59:45Z

Review requested:

@nodejs/test_runner

mcollina

Thanks for opening a PR! Can you please add a unit test?

EddieAbbondanzio · 2024-07-27T01:20:49Z

Sure, I added one.

mcollina

lgtm

nodejs-github-bot · 2024-07-27T15:35:09Z

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

cjihrig · 2024-07-28T03:24:33Z

doc/api/test.md

@@ -2850,6 +2850,7 @@ The corresponding declaration ordered events are `'test:pass'` and `'test:fail'`
    `undefined` if the test was run through the REPL.
  * `name` {string} The test name.
  * `nesting` {number} The nesting level of the test.
+  * `type` {string} The test type.


I don't think these docs are currently descriptive enough. Since there are only two possible values, maybe we should list them out.

I updated the docs, but I had some trouble thinking of how to phrase it.

cjihrig · 2024-07-28T03:26:29Z

lib/internal/test_runner/test.js

@@ -570,7 +570,8 @@ class Test extends AsyncResource {
    while (this.pendingSubtests.length > 0 && this.hasConcurrency()) {
      const deferred = ArrayPrototypeShift(this.pendingSubtests);
      const test = deferred.test;
-      test.reporter.dequeue(test.nesting, test.loc, test.name);
+      const type = test.reportedType ?? 'test';


I think we should make reportedType a property on Test like it is on Suite and drop this.

Do you think we should also a reported type on the TestHook class since it extends Test? I don't think it'll cause any issues, but could lead to future confusion.

I set reportedType on TestHook to 'hook'.

cjihrig · 2024-07-28T03:32:53Z

test/parallel/test-runner-run.mjs

+
+    stream.on('test:enqueue', common.mustCall((data) => {
+      if (data.name === 'this is a suite') {
+        assert.strictEqual(data.type, 'suite');


There is no guarantee that any of these assertions will run, even with the common.mustCall() (for example, if the test fixture was changed in the future). I would suggest adding a test plan.

Sounds good, but I'm not familiar with what a test plan is. Is there an example I could reference?

Oh, you meant this: https://nodejs.org/api/test.html#contextplancount

Good call. I added a plan and it uncovered two of the assertions weren't working due to a leading space in the name.

nodejs-github-bot · 2024-07-29T06:25:47Z

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

nodejs-github-bot · 2024-07-30T08:26:00Z

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

EddieAbbondanzio · 2024-08-06T22:15:34Z

This PR seems like it may have stagnated. Is there anything needed to get it merged?

nodejs-github-bot · 2024-08-07T07:02:14Z

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

pmarchini · 2024-12-04T22:05:13Z

Hey @EddieAbbondanzio, I noticed this PR has been stale since August and some files now have conflicts.
Could you please resolve them?

EddieAbbondanzio · 2024-12-16T13:02:43Z

Hey @pmarchini, I can work on bring it back but do you know why it went stale? Was there something else I needed to do to get it merged after all the approvals it got back in July?

pmarchini · 2024-12-28T17:57:13Z

Hey @EddieAbbondanzio, honestly, I can't see any reason why this PR should remain stalled.
I would suggest fixing the conflicts and then landing it 😊

codecov · 2025-01-01T17:34:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (35742a2) to head (214b59e).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54049      +/-   ##
==========================================
- Coverage   88.54%   88.54%   -0.01%     
==========================================
  Files         657      657              
  Lines      190742   190746       +4     
  Branches    36606    36603       -3     
==========================================
- Hits       168901   168888      -13     
- Misses      15027    15039      +12     
- Partials     6814     6819       +5

Files with missing lines	Coverage Δ
lib/internal/test_runner/test.js	`96.96% <100.00%> (+<0.01%)`	⬆️
lib/internal/test_runner/tests_stream.js	`91.71% <100.00%> (+0.09%)`	⬆️

... and 23 files with indirect coverage changes

nodejs-github-bot · 2025-01-02T09:00:56Z

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

nodejs-github-bot · 2025-01-02T15:00:28Z

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

nodejs-github-bot · 2025-01-02T18:45:37Z

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

EddieAbbondanzio · 2025-01-03T12:53:50Z

Hey @pmarchini, is there anything on my end I need to do to merge this?

I'm still new to contributing to Node and don't have permissions to add the merge-queue label (seems like that's what this PR needs based on the docs) but I'm also not 100% certain why some of the CI jobs are intermittently failing. Appreciate your help!

nodejs-github-bot · 2025-01-03T13:01:44Z

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

pmarchini · 2025-01-03T13:01:52Z

Hey @EddieAbbondanzio, you can't do anything else on your side atm 😁
We have some problems with flaky tests, and that's the reason why the CI keeps failing.
I'm restarting it again now!

As soon as the CI is green, and if @cjihrig and @MoLow are still okay with this change, we will land it 🚀

Thank you very much for your patience and your support to Node 🚀

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

mcollina requested changes Jul 26, 2024

View reviewed changes

EddieAbbondanzio requested a review from mcollina July 27, 2024 01:21

mcollina approved these changes Jul 27, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 27, 2024

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

cjihrig reviewed Jul 28, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024

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

EddieAbbondanzio requested a review from cjihrig July 30, 2024 02:22

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 30, 2024

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

cjihrig approved these changes Jul 31, 2024

View reviewed changes

MoLow approved these changes Aug 2, 2024

View reviewed changes

aymen94 approved these changes Aug 7, 2024

View reviewed changes

pmarchini approved these changes Dec 4, 2024

View reviewed changes

jasnell approved these changes Dec 28, 2024

View reviewed changes

EddieAbbondanzio added 2 commits January 1, 2025 10:34

test_runner: differentiate test types in enqueue dequeue events

2f8ed8c

add test

76305b3

EddieAbbondanzio added 4 commits January 1, 2025 10:35

add reported type to test

5dceec6

be more explicit in docs on type values

2140900

regenerate snapshots for tests

ba3e525

add test plan

56c63e1

EddieAbbondanzio force-pushed the differentiate-test-types-in-events branch from 32c6955 to 56c63e1 Compare January 1, 2025 16:20

regenerate snapshots for tests

214b59e

pmarchini added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2025

This comment was marked as spam.

Sign in to view

pmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: differentiate test types in enqueue dequeue events #54049

test_runner: differentiate test types in enqueue dequeue events #54049

EddieAbbondanzio commented Jul 26, 2024

nodejs-github-bot commented Jul 26, 2024

mcollina left a comment

EddieAbbondanzio commented Jul 27, 2024

mcollina left a comment

nodejs-github-bot commented Jul 27, 2024

cjihrig Jul 28, 2024

EddieAbbondanzio Jul 29, 2024 •

edited

Loading

cjihrig Jul 28, 2024

EddieAbbondanzio Jul 29, 2024

EddieAbbondanzio Jul 30, 2024

cjihrig Jul 28, 2024

EddieAbbondanzio Jul 29, 2024

EddieAbbondanzio Jul 30, 2024

nodejs-github-bot commented Jul 29, 2024

nodejs-github-bot commented Jul 30, 2024

EddieAbbondanzio commented Aug 6, 2024 •

edited

Loading

nodejs-github-bot commented Aug 7, 2024

pmarchini commented Dec 4, 2024

EddieAbbondanzio commented Dec 16, 2024

pmarchini commented Dec 28, 2024

codecov bot commented Jan 1, 2025 •

edited

Loading

nodejs-github-bot commented Jan 2, 2025

nodejs-github-bot commented Jan 2, 2025

nodejs-github-bot commented Jan 2, 2025

EddieAbbondanzio commented Jan 3, 2025

nodejs-github-bot commented Jan 3, 2025

pmarchini commented Jan 3, 2025 •

edited

Loading

This comment was marked as spam.

test_runner: differentiate test types in enqueue dequeue events #54049

Are you sure you want to change the base?

test_runner: differentiate test types in enqueue dequeue events #54049

Conversation

EddieAbbondanzio commented Jul 26, 2024

nodejs-github-bot commented Jul 26, 2024

mcollina left a comment

Choose a reason for hiding this comment

EddieAbbondanzio commented Jul 27, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 27, 2024

cjihrig Jul 28, 2024

Choose a reason for hiding this comment

EddieAbbondanzio Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

cjihrig Jul 28, 2024

Choose a reason for hiding this comment

EddieAbbondanzio Jul 29, 2024

Choose a reason for hiding this comment

EddieAbbondanzio Jul 30, 2024

Choose a reason for hiding this comment

cjihrig Jul 28, 2024

Choose a reason for hiding this comment

EddieAbbondanzio Jul 29, 2024

Choose a reason for hiding this comment

EddieAbbondanzio Jul 30, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 29, 2024

nodejs-github-bot commented Jul 30, 2024

EddieAbbondanzio commented Aug 6, 2024 • edited Loading

nodejs-github-bot commented Aug 7, 2024

pmarchini commented Dec 4, 2024

EddieAbbondanzio commented Dec 16, 2024

pmarchini commented Dec 28, 2024

codecov bot commented Jan 1, 2025 • edited Loading

Codecov Report

nodejs-github-bot commented Jan 2, 2025

nodejs-github-bot commented Jan 2, 2025

nodejs-github-bot commented Jan 2, 2025

EddieAbbondanzio commented Jan 3, 2025

nodejs-github-bot commented Jan 3, 2025

pmarchini commented Jan 3, 2025 • edited Loading

This comment was marked as spam.

EddieAbbondanzio Jul 29, 2024 •

edited

Loading

EddieAbbondanzio commented Aug 6, 2024 •

edited

Loading

codecov bot commented Jan 1, 2025 •

edited

Loading

pmarchini commented Jan 3, 2025 •

edited

Loading