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: make end of work check stricter #52326

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 2, 2024

This commit updates the logic that checks for the end of the test run. Prior to this change, it was possible for root.run() to be called multiple times because of the way pending subtests were tracked. The extra calls to root.run() were harmless, but could trigger an EventEmitter leak warning due to 'abort' listeners being created.

This commit updates the logic that checks for the end of the
test run. Prior to this change, it was possible for root.run() to
be called multiple times because of the way pending subtests
were tracked. The extra calls to root.run() were harmless, but
could trigger an EventEmitter leak warning due to 'abort'
listeners being created.
@cjihrig cjihrig requested review from MoLow and atlowChemi April 2, 2024 15:23
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Apr 2, 2024
@cjihrig cjihrig added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Apr 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 5, 2024

@MoLow do you know if there is anything that can be done on the build side? It seems like macOS has been having a tough time in the CI the past day or so. I keep seeing test/parallel/test-macos-app-sandbox.js fail while trying to spawn a codesign process.

FWIW, this commit seems to help with some test runner flakiness in the CI.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig cjihrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 11, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 11, 2024
@nodejs-github-bot nodejs-github-bot merged commit 52f8dcf into nodejs:main Apr 11, 2024
63 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 52f8dcf

@cjihrig cjihrig deleted the root-rerun branch April 11, 2024 02:44
@marco-ippolito
Copy link
Member

This needs a backport on v20.x

@marco-ippolito marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label May 23, 2024
atlowChemi pushed a commit to atlowChemi/node that referenced this pull request May 23, 2024
This commit updates the logic that checks for the end of the
test run. Prior to this change, it was possible for root.run() to
be called multiple times because of the way pending subtests
were tracked. The extra calls to root.run() were harmless, but
could trigger an EventEmitter leak warning due to 'abort'
listeners being created.

PR-URL: nodejs#52326
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@atlowChemi atlowChemi added the backport-open-v20.x Indicate that the PR has an open backport label May 23, 2024
@atlowChemi atlowChemi removed the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label May 23, 2024
marco-ippolito pushed a commit to atlowChemi/node that referenced this pull request Jun 12, 2024
This commit updates the logic that checks for the end of the
test run. Prior to this change, it was possible for root.run() to
be called multiple times because of the way pending subtests
were tracked. The extra calls to root.run() were harmless, but
could trigger an EventEmitter leak warning due to 'abort'
listeners being created.

PR-URL: nodejs#52326
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
marco-ippolito pushed a commit to atlowChemi/node that referenced this pull request Aug 8, 2024
This commit updates the logic that checks for the end of the
test run. Prior to this change, it was possible for root.run() to
be called multiple times because of the way pending subtests
were tracked. The extra calls to root.run() were harmless, but
could trigger an EventEmitter leak warning due to 'abort'
listeners being created.

PR-URL: nodejs#52326
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-open-v20.x Indicate that the PR has an open backport test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants