Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_runner: count nested tests #47094

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Mar 14, 2023

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 cjihrig March 14, 2023 19:41
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot 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
lib/internal/test_runner/runner.js Outdated Show resolved Hide resolved
lib/internal/test_runner/test.js Outdated Show resolved Hide resolved
@MoLow MoLow added the test_runner Issues and PRs related to the test runner subsystem. label Mar 15, 2023
@MoLow
Copy link
Member Author

MoLow commented Mar 15, 2023

ping @nodejs/test_runner can someone review this please?

Copy link
Contributor

@cjihrig cjihrig left a 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.

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

const completedTests = new SafeSet();
Copy link
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.

Copy link
Member Author

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

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

class Suite extends Test {
omitFromCounters = true;
reportedType = 'suite';
Copy link
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).

Copy link
Member Author

@MoLow MoLow Mar 16, 2023

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?

Copy link
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.

Copy link
Member Author

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

cancelled: 0,
skipped: 0,
todo: 0,
topLevel: 0,
Copy link
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 MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2023
@nodejs-github-bot
Copy link
Collaborator

@MoLow 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 MoLow requested a review from benjamingr March 19, 2023 13:08
@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 21, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit d1eaded into nodejs:main Mar 21, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in d1eaded

@MoLow MoLow deleted the test-runner-count-nested branch March 21, 2023 08:20
RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
PR-URL: #47094
Fixes: #46762
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
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
PR-URL: #47094
Fixes: #46762
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
philnash added a commit to philnash/DefinitelyTyped that referenced this pull request Aug 4, 2023
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
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
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

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
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

describe should not be counted as a single test
5 participants