-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
test_runner: write timestamp as first line in watch mode #59309
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
base: main
Are you sure you want to change the base?
test_runner: write timestamp as first line in watch mode #59309
Conversation
Review requested:
|
this.#tryPush(null); | ||
} | ||
|
||
// This event emitter logs a timestamp whenever a test restarts because of changes in any related files. |
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.
no need comment here?
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 have taken it out
lib/internal/test_runner/runner.js
Outdated
|
||
await runningSubtests.get(file); | ||
|
||
// Emit the 'test:restarted' event if a timeStamp reporter is available. |
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.
same
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.
This has been taken out also.
The goal: To always print a timestamp as the first line in watch mode to make test restarts clearer and logs easier to read. Continuation from https://github.com/nodejs/node/pull/57903/files solution, i simply created a new event 'timestamp' and then handled it. Also modified the test to directly test my changes from runner.js instead of test_streams.js file. Refs: nodejs#57206 Authors: https://github.com/JacopoPatroclo, https://github.com/OkunadeNaheem
dcf91d8
to
fac4f07
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.
In this PR we’re not introducing the timestamp as stated in the title.
What we should do instead is leverage the existing event so that the various reporters can print a restart message followed by the date.
The tests should then verify this behaviour!
|
||
await runningSubtests.get(file); | ||
|
||
opts.root.reporter[kEmitMessage]('test:watch:restarted'); |
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.
Hey @OkunadeNaheem, it looks like this branch was created from a very outdated main branch.
This addition was already made two months ago in this PR: #57903
Here's the line: https://github.com/nodejs/node/blob/main/lib/internal/test_runner/runner.js#L526
test_runner: write timestamp as first line in watch mode
The goal: To always print a timestamp as the first line in watch mode to make test restarts clearer and logs easier to read.
Continuation from https://github.com/nodejs/node/pull/57903/files solution, i simply created a new event 'timestamp' and then handled it. Also modified the test to directly test my changes from runner.js instead of test_streams.js file.
Refs: #57206