FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test_runner: watch mode improvments by MoLow · Pull Request #48259 · nodejs/node · GitHub

/ node Public

test_runner: watch mode improvments - #48259

Closed
MoLow wants to merge 2 commits into
nodejs:mainfrom
MoLow:test-runner-watch-mode-additions
ConversationCommits (2)ChecksFiles changed
Closed

test_runner: watch mode improvments#48259
MoLow wants to merge 2 commits into
nodejs:mainfrom
MoLow:test-runner-watch-mode-additions

Conversation

MoLow commented May 30, 2023

Copy link
Copy Markdown
Member

some fixes for using run with watch: true:

  • emit an event when no more queued tests
  • the queued tests were in a global state preventing the event ^ from working correctly when using multiple run's
  • respect AbortSignal

MoLow requested review from benjamingr and cjihrig May 30, 2023 23:58

nodejs-github-bot commented May 30, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

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 May 30, 2023
MoLow added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label May 31, 2023
cjihrig approved these changes May 31, 2023
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code LGTM with a few minor comments. I do have two questions:

  1. What was the motivation for this new event?
  2. Why do we only care about this event in the context of watch mode? Is there value in having a generic test:drain event?

Comment thread doc/api/test.md Outdated
Comment thread lib/internal/test_runner/runner.js Outdated

MoLow commented May 31, 2023

Copy link
Copy Markdown
Member Author

Code LGTM with a few minor comments. I do have two questions:

  1. What was the motivation for this new event?
  2. Why do we only care about this event in the context of watch mode? Is there value in having a generic test:drain event?

I have started experimenting with an npm package that runs tests in interactive mode. just a feedback/feature request from friends.
I don't think a generic test:drain is needed - since when not in watch mode you can simply listen to the testsStream end event

Screen.Recording.2023-05-31.at.11.13.02.mov

MoLow force-pushed the test-runner-watch-mode-additions branch from 39bc086 to a52fb04 Compare May 31, 2023 09:08
benjamingr reviewed May 31, 2023
View reviewed changes
Comment thread lib/internal/test_runner/runner.js Outdated

benjamingr May 31, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

need to remove this listener if abort never happens but the watcher is already cleared or is this always before the process dies and "hanging" this if we never abort is fine?

MoLow May 31, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

it is ok - if you don't pass a signal the watcher will never clear/close

benjamingr May 31, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The concern was about leaking the listener on the signal after run finished.

MoLow May 31, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

indeed - in watch mode run does not finish besides this new flow

MoLow added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels May 31, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023

nodejs-github-bot commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow force-pushed the test-runner-watch-mode-additions branch from a52fb04 to d55ee1e Compare May 31, 2023 21:00

MoLow commented May 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

@benjamingr I pushed a fix for a race condition (the test finishes before the watch mode ipc message is processed) - so the abortSignal is now passed into fs.watch
PTAL (and re-approve for convenience of the commit queue)

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023

nodejs-github-bot commented May 31, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
github-actions Bot mentioned this pull request Jun 2, 2023
Open
34 tasks
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 2, 2023

nodejs-github-bot commented Jun 2, 2023

Copy link
Copy Markdown
Collaborator

Landed in 0cf8bbe...5d685e4

nodejs-github-bot pushed a commit that referenced this pull request Jun 2, 2023
test_runner: stop watch mode when abortSignal aborted
ca71ae6 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Jun 2, 2023
test_runner: emit test:watch:drained event
5d685e4 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
github-actions Bot mentioned this pull request Jun 3, 2023
Open
33 tasks
MoLow deleted the test-runner-watch-mode-additions branch June 3, 2023 19:25
github-actions Bot mentioned this pull request Jun 4, 2023
Open
30 tasks
targos pushed a commit that referenced this pull request Jun 4, 2023
test_runner: stop watch mode when abortSignal aborted
c9f8e8c 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Jun 4, 2023
test_runner: emit test:watch:drained event
3f259b7 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos mentioned this pull request Jun 4, 2023
Merged
This was referenced Jun 5, 2023
Open
Open
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
test_runner: stop watch mode when abortSignal aborted
97aad16 
PR-URL: #48259
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
test_runner: emit test:watch:drained event
68f2c49 
PR-URL: #48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
test_runner: stop watch mode when abortSignal aborted
618a9e1 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
test_runner: emit test:watch:drained event
033d0bb 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams mentioned this pull request Jul 10, 2023
Merged
philnash mentioned this pull request Aug 2, 2023
Merged
7 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
test_runner: stop watch mode when abortSignal aborted
40a500d 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
test_runner: emit test:watch:drained event
6ba2949 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
test_runner: stop watch mode when abortSignal aborted
7082c47 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
test_runner: emit test:watch:drained event
a27c30f 
PR-URL: nodejs#48259
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

benjamingr benjamingr approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. 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.

4 participants

Back | FazBrowse Home | New Git URL