test_runner: count nested tests by MoLow · Pull Request #47094 · nodejs/node · GitHub

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

Review requested:

@nodejs/test_runner

ping @nodejs/test_runner can someone review this please?

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.

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

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

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

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

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

Comment thread lib/internal/test_runner/runner.js Outdated

Comment thread lib/internal/test_runner/test.js Outdated

MoLow commented Mar 14, 2023

View reviewed changes

Comment thread lib/internal/test_runner/reporter/tap.js Outdated

MoLow added the test_runner Issues and PRs related to the test runner subsystem. 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 outstanding review comments, 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

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

test_runner: count nested tests by MoLow · Pull Request #47094 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: count nested tests - #47094

test_runner: count nested tests#47094nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom MoLow:test-runner-count-nestedMoLow/node:test-runner-count-nestedCopy head branch name to clipboard

Conversation

MoLow commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 14, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MoLow commented Mar 15, 2023

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

MoLow Mar 16, 2023

Choose a reason for hiding this comment

Uh oh!

cjihrig Mar 15, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 18, 2023

Uh oh!

nodejs-github-bot commented Mar 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test_runner: count nested tests#47094
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:test-runner-count-nested

MoLow commented Mar 14, 2023 •
edited
Loading

MoLow Mar 16, 2023 •
edited
Loading