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: remove root tracking set #46961

Merged
merged 1 commit into from
Mar 7, 2023
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Mar 5, 2023

The wasRootSetup Set in the test harness appears to be redundant, since the startTime field can be used interchangeably. This commit removes wasRootSetup.

@cjihrig cjihrig requested a review from MoLow March 5, 2023 16:59
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 5, 2023

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Mar 5, 2023
@@ -105,7 +103,7 @@ function collectCoverage(rootTest, coverage) {
}

function setup(root) {
if (wasRootSetup.has(root)) {
if (root.startTime !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn’t this change mean that someone could create a Test themselves, and be able to monkey with the harness? By using a weakest you’re ensuring that only things you create are treated as such.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think userland has access to any of the necessary APIs (the Test class or any of the harness internals) to do that.

@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 5, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 6, 2023

@cjihrig cjihrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Mar 6, 2023
The wasRootSetup Set in the test harness appears to be
redundant, since the startTime field can be used interchangeably.
This commit removes wasRootSetup.

PR-URL: nodejs#46961
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@cjihrig cjihrig merged commit d4c3e3b into nodejs:main Mar 7, 2023
@cjihrig cjihrig deleted the rootsetup branch March 7, 2023 19:03
targos pushed a commit that referenced this pull request Mar 13, 2023
The wasRootSetup Set in the test harness appears to be
redundant, since the startTime field can be used interchangeably.
This commit removes wasRootSetup.

PR-URL: #46961
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos pushed a commit that referenced this pull request Mar 14, 2023
The wasRootSetup Set in the test harness appears to be
redundant, since the startTime field can be used interchangeably.
This commit removes wasRootSetup.

PR-URL: #46961
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
The wasRootSetup Set in the test harness appears to be
redundant, since the startTime field can be used interchangeably.
This commit removes wasRootSetup.

PR-URL: #46961
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
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. commit-queue Add this label to land a pull request using GitHub Actions. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants