test_runner: count nested tests #47094

MoLow · 2023-03-14T19:41:21Z

Fixes: #46762

this also introduces a property on the TAP yaml output to allow distinction between a suite and a regular test

nodejs-github-bot · 2023-03-14T19:41:25Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/runner.js

lib/internal/test_runner/test.js

lib/internal/test_runner/reporter/tap.js

MoLow · 2023-03-15T20:43:19Z

ping @nodejs/test_runner can someone review this please?

cjihrig

I think this is a change we should make. I left a couple high level comments that I think could make the implementation a bit cleaner.

cjihrig · 2023-03-15T21:33:38Z

lib/internal/test_runner/test.js

@@ -71,6 +71,8 @@ const kUnwrapErrors = new SafeSet()
  .add('uncaughtException').add('unhandledRejection');
 const { testNamePatterns, testOnlyFlag } = parseCommandLine();

+const completedTests = new SafeSet();


I would like to get rid of this if possible. Each Test object now has a harness field that can be used to hold this "process level state." It is only used on the root test, and is currently initialized in harness.js but there is no reason it couldn't be initialized in the Test constructor. I have considered giving every Test a reference to the root test for a while now - I think that could simplify some things, including this.

We could put the counters object in the harness since it has the same scope as the other pieces of data that are stored in it. Then, instead of tracking all of the tests in a Set, every test could update the counters directly via this.root.harness.counters.

I think this would also get rid of the need for omitFromCounters because each test/suite would be doing its own updating (or not updating) of the counters.

I like that suggestion very much

cjihrig · 2023-03-15T21:45:25Z

lib/internal/test_runner/test.js

@@ -746,6 +769,8 @@ class TestHook extends Test {
 }

 class Suite extends Test {
+  omitFromCounters = true;
+  reportedType = 'suite';


Maybe we should have a type field on Test and everything that extends from it. We could differentiate between tests, suites, hooks, etc.

We could also use Symbol.toStringTag instead of a normal property or instanceof (although that one is probably a bigger perf hit).

isnt this what I did? not sure what else are you suggesting?

I was suggesting adding it to Test and all of the classes that extend Test. Right now it's only on Suite.

I do not currently see any usage for that, but even if we want that just for the sake of simplifying code it should probably happen in separate PR

cjihrig · 2023-03-15T21:50:09Z

lib/internal/test_runner/test.js

+        cancelled: 0,
+        skipped: 0,
+        todo: 0,
+        topLevel: 0,


I think it's worth keeping a count of the suites as well.

nodejs-github-bot · 2023-03-18T21:11:10Z

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

nodejs-github-bot · 2023-03-21T08:16:40Z

Landed in d1eaded

PR-URL: #47094 Fixes: #46762 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

In [version 20.0.0 (and backported to 19.9.0 and 18.7.0)](nodejs/node#47094) the test runner started reporting on whether a test was a suite. This was exposed to reporters in the `details` object of a `test:pass` or `test:fail` event but this hasn't been documented. This adds the `type` property to both event's `details` object.

…philnash

…philnash In [version 20.0.0 (and backported to 19.9.0 and 18.7.0)](nodejs/node#47094) the test runner started reporting on whether a test was a suite. This was exposed to reporters in the `details` object of a `test:pass` or `test:fail` event but this hasn't been documented. This adds the `type` property to both event's `details` object.

MoLow requested a review from cjihrig March 14, 2023 19:41

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 14, 2023

ljharb reviewed Mar 14, 2023

View reviewed changes

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

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

MoLow commented Mar 14, 2023

View reviewed changes

lib/internal/test_runner/reporter/tap.js Outdated Show resolved Hide resolved

MoLow added the test_runner label Mar 15, 2023

MoLow force-pushed the test-runner-count-nested branch from 7955d04 to d493a9b Compare March 15, 2023 20:37

cjihrig reviewed Mar 15, 2023

View reviewed changes

MoLow force-pushed the test-runner-count-nested branch from d493a9b to 831ab97 Compare March 16, 2023 07:14

test_runner: count nested tests

d98c46e

MoLow force-pushed the test-runner-count-nested branch from 831ab97 to d98c46e Compare March 16, 2023 10:01

cjihrig approved these changes Mar 18, 2023

View reviewed changes

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2023

MoLow added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 19, 2023

MoLow requested a review from benjamingr March 19, 2023 13:08

benjamingr approved these changes Mar 21, 2023

View reviewed changes

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2023

nodejs-github-bot merged commit d1eaded into nodejs:main Mar 21, 2023
27 checks passed

MoLow deleted the test-runner-count-nested branch March 21, 2023 08:20

RafaelGSS mentioned this pull request Apr 3, 2023

2023-04-18, Version 20.0.0 (Current) #47381

Merged

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023

test_runner: count nested tests

9808a4b

PR-URL: #47094 Fixes: #46762 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

RafaelGSS mentioned this pull request Apr 6, 2023

v19.9.0 proposal #47441

Merged

RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023

test_runner: count nested tests

4530582

PR-URL: #47094 Fixes: #46762 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

danielleadams pushed a commit that referenced this pull request Jul 6, 2023

test_runner: count nested tests

460bcc0

PR-URL: #47094 Fixes: #46762 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

danielleadams mentioned this pull request Jul 10, 2023

v18.17.0 release proposal #48694

Merged

philnash mentioned this pull request Aug 4, 2023

doc: add "type" to test runner event details #49014

Merged

philnash mentioned this pull request Aug 4, 2023

[node] adds "type" to test runner event details DefinitelyTyped/DefinitelyTyped#66282

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: count nested tests #47094

test_runner: count nested tests #47094

MoLow commented Mar 14, 2023 •

edited

nodejs-github-bot commented Mar 14, 2023

MoLow commented Mar 15, 2023

cjihrig left a comment

cjihrig Mar 15, 2023

MoLow Mar 15, 2023

cjihrig Mar 15, 2023

MoLow Mar 16, 2023 •

edited

cjihrig Mar 16, 2023

MoLow Mar 16, 2023

cjihrig Mar 15, 2023

nodejs-github-bot commented Mar 18, 2023

nodejs-github-bot commented Mar 21, 2023

test_runner: count nested tests #47094

test_runner: count nested tests #47094

Conversation

MoLow commented Mar 14, 2023 • edited

nodejs-github-bot commented Mar 14, 2023

MoLow commented Mar 15, 2023

cjihrig left a comment

Choose a reason for hiding this comment

cjihrig Mar 15, 2023

Choose a reason for hiding this comment

MoLow Mar 15, 2023

Choose a reason for hiding this comment

cjihrig Mar 15, 2023

Choose a reason for hiding this comment

MoLow Mar 16, 2023 • edited

Choose a reason for hiding this comment

cjihrig Mar 16, 2023

Choose a reason for hiding this comment

MoLow Mar 16, 2023

Choose a reason for hiding this comment

cjihrig Mar 15, 2023

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 18, 2023

nodejs-github-bot commented Mar 21, 2023

MoLow commented Mar 14, 2023 •

edited

MoLow Mar 16, 2023 •

edited