Skip to content

test_runner: count nested tests - #47094

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:test-runner-count-nested
Mar 21, 2023
Merged

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

Conversation

@MoLow

@MoLowMoLow commented Mar 14, 2023•
edited
Loading

Copy link
Copy Markdown
Member

Fixes: #46762

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

@MoLow
MoLow requested a review from cjihrigMarch 14, 2023 19:41
@nodejs-github-bot

nodejs-github-bot commented Mar 14, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-botnodejs-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
ljharb reviewed Mar 14, 2023
View reviewed changes
Comment threadlib/internal/test_runner/runner.js Outdated
Comment threadlib/internal/test_runner/test.js Outdated
MoLow
MoLow commented Mar 14, 2023
View reviewed changes
Comment threadlib/internal/test_runner/reporter/tap.js Outdated
@MoLowMoLow added the test_runner Issues and PRs related to the test runner subsystem. label Mar 15, 2023
@MoLow
MoLowforce-pushed the test-runner-count-nested branch from 7955d04 to d493a9bCompareMarch 15, 2023 20:37
@MoLow

MoLow commented Mar 15, 2023

Copy link
Copy Markdown
MemberAuthor

ping @nodejs/test_runner can someone review this please?

cjihrig
cjihrig reviewed Mar 15, 2023
View reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment threadlib/internal/test_runner/test.js Outdated

@cjihrigcjihrigMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@MoLowMoLowMar 15, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that suggestion very much

Comment threadlib/internal/test_runner/test.js Outdated

@cjihrigcjihrigMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MoLowMoLowMar 16, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cjihrigcjihrigMar 16, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MoLowMoLowMar 16, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadlib/internal/test_runner/test.js Outdated

@cjihrigcjihrigMar 15, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MoLow
MoLowforce-pushed the test-runner-count-nested branch from d493a9b to 831ab97CompareMarch 16, 2023 07:14
@MoLow
MoLowforce-pushed the test-runner-count-nested branch from 831ab97 to d98c46eCompareMarch 16, 2023 10:01
@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 18, 2023

Copy link
Copy Markdown
Collaborator

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

@MoLowMoLow 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
MoLow requested a review from benjamingrMarch 19, 2023 13:08
@MoLowMoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2023
@nodejs-github-bot
nodejs-github-bot merged commit d1eaded into nodejs:mainMar 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in d1eaded

@MoLow
MoLow deleted the test-runner-count-nested branch March 21, 2023 08:20
@RafaelGSSRafaelGSS mentioned this pull request Apr 3, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
@MoLow@RafaelGSS
test_runner: count nested tests
9808a4b
PR-URL: #47094Fixes: #46762
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Apr 6, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
@MoLow@RafaelGSS
test_runner: count nested tests
4530582
PR-URL: #47094Fixes: #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
@MoLow@danielleadams
test_runner: count nested tests
460bcc0
PR-URL: #47094Fixes: #46762
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Jul 10, 2023
Merged
@philnashphilnash mentioned this pull request Aug 4, 2023
Merged
philnash added a commit to philnash/DefinitelyTyped that referenced this pull request Aug 4, 2023
@philnash
[node] adds "type" to test runner event details
358bd51
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.
@philnashphilnash mentioned this pull request Aug 4, 2023
Merged
7 tasks
philnash added a commit to philnash/DefinitelyTyped that referenced this pull request Aug 7, 2023
@philnash
[node] adds "type" to test runner event details
77c6dde
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 added a commit to philnash/DefinitelyTyped that referenced this pull request Aug 7, 2023
@philnash
[node] adds "type" to test runner event details
fac7aea
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.
typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Aug 11, 2023
@philnash
🤖 Merge PR#66282[node] adds "type" to test runner event details by@…
15ee4f9
…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.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharbljharbljharb left review comments
@benjamingrbenjamingrbenjamingr approved these changes
@cjihrigcjihrigcjihrig approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.test_runnerIssues and PRs related to the test runner subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

describe should not be counted as a single test

5 participants

@MoLow@nodejs-github-bot@ljharb@benjamingr@cjihrig