GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test_runner: count nested tests by MoLow · Pull Request #47094 · nodejs/node

Conversation

@MoLow

@MoLow MoLow commented Mar 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tonivj5 and bricss reacted with thumbs up emoji meyfa reacted with heart emoji
All reactions
  • 👍 2 reactions
  • ❤️ 1 reaction
@MoLow
MoLow requested a review from cjihrig March 14, 2023 19:41
@nodejs-github-bot

nodejs-github-bot commented Mar 14, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Mar 14, 2023
ljharb
ljharb reviewed Mar 14, 2023
View reviewed changes
Comment thread lib/internal/test_runner/runner.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread lib/internal/test_runner/test.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
MoLow
MoLow commented Mar 14, 2023
View reviewed changes
Comment thread lib/internal/test_runner/reporter/tap.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Mar 15, 2023
@MoLow
MoLow force-pushed the test-runner-count-nested branch from 7955d04 to d493a9b Compare March 15, 2023 20:37
@MoLow

MoLow commented Mar 15, 2023

Copy link
Copy Markdown
Member Author

ping @nodejs/test_runner can someone review this please?
cjihrig reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cjihrig
cjihrig reviewed Mar 15, 2023
View reviewed changes

@cjihrig cjihrig left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/test_runner/test.js Outdated

@cjihrig cjihrig Mar 15, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MoLow reacted with thumbs up emoji tonivj5 reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction

@MoLow MoLow Mar 15, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/test_runner/test.js Outdated

@cjihrig cjihrig Mar 15, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Mar 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cjihrig cjihrig Mar 16, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@MoLow MoLow Mar 16, 2023

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/test_runner/test.js Outdated

@cjihrig cjihrig Mar 15, 2023

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@MoLow
MoLow force-pushed the test-runner-count-nested branch from d493a9b to 831ab97 Compare March 16, 2023 07:14
@MoLow
test_runner: count nested tests
d98c46e
@MoLow
MoLow force-pushed the test-runner-count-nested branch from 831ab97 to d98c46e Compare March 16, 2023 10:01
cjihrig
cjihrig approved these changes Mar 18, 2023
View reviewed changes
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Mar 18, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow MoLow added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Mar 19, 2023
@MoLow
MoLow requested a review from benjamingr March 19, 2023 13:08
benjamingr
benjamingr approved these changes Mar 21, 2023
View reviewed changes
@MoLow MoLow added the commit-queue PRs queued for automated landing through the Commit Queue. label Mar 21, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Mar 21, 2023
@nodejs-github-bot
nodejs-github-bot merged commit d1eaded into nodejs:main Mar 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in d1eaded
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MoLow
MoLow deleted the test-runner-count-nested branch March 21, 2023 08:20
@RafaelGSS 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
@MoLow @RafaelGSS
test_runner: count nested tests
9808a4b 
PR-URL: #47094
Fixes: #46762
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@RafaelGSS 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
@MoLow @RafaelGSS
test_runner: count nested tests
4530582 
PR-URL: #47094
Fixes: #46762
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@MoLow @danielleadams
test_runner: count nested tests
460bcc0 
PR-URL: #47094
Fixes: #46762
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
v18.17.0 release proposal #48694
Merged
@philnash philnash mentioned this pull request Aug 4, 2023
doc: add "type" to test runner event details #49014
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.
@philnash philnash mentioned this pull request Aug 4, 2023
[node] adds "type" to test runner event details DefinitelyTyped/DefinitelyTyped#66282
Merged
7 tasks done
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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@benjamingr benjamingr benjamingr approved these changes
@cjihrig cjihrig cjihrig approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. test_runner Issues 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

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@MoLow @nodejs-github-bot @ljharb @benjamingr @cjihrig
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.