-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: support watch mode #45214
Conversation
91df6e5
to
49ad862
Compare
@MoLow is this ready for review? At a minimum, it looks like there are linter issues. |
not yet, will work on it today/tomorrow |
49ad862
to
9be722d
Compare
@nodejs/test_runner ready for review |
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.
We should probably update the doc of --watch-path
and --test
to clarify they can't be used together
Lines 1214 to 1216 in f1e9382
Starts the Node.js command line test runner. This flag cannot be combined with | |
`--check`, `--eval`, `--interactive`, or the inspector. See the documentation | |
on [running tests from the command line][] for more details. |
Lines 1610 to 1611 in f1e9382
This flag cannot be combined with | |
`--check`, `--eval`, `--interactive`, or the REPL. |
We could add an entry to the history of --watch
and --test
to say that now they can be used together.
Could we add a test to ensure watching ESM works? And maybe even ESM+CJS?
@ErickWendel you were asking about this |
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 don't know if you missed it in my previous review, I was asking if it would be possible to add tests for ESM (i.e. does dependencies loaded though import()
from a CJS files are picked up)
Going to rebase on top of #45348 to avoid conflicts |
bf37536
to
28f8da3
Compare
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.
LGTM if the CI is happy
Landed in 97355c7 |
PR-URL: #45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #45214 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
TODO:
clearFileFilters
for specific child process