-
Notifications
You must be signed in to change notification settings - Fork 30k
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: added a test comment #16003
test: added a test comment #16003
Conversation
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 CI passes
92ef77a
to
895e8c6
Compare
This doesn't seem to be about migrating string concatenations anymore after the update. It's more about adding a comment explaining what this test does now? |
@joyeecheung I would agree, would you suggest I rebase and update the comment or just edit the pr description? |
const fixtures = require('../common/fixtures'); | ||
|
||
// This test ensures that the debug-brk flag will spin up a new process and | ||
// wait, aka not quit. |
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.
Micro-nit that you can ignore if you wish: aka not quit
may be a bit unclear to people unfamiliar with a.k.a. Might be better as ...and wait, rather than exit.
@earobinson I'd say update both if it's not a hassle for you. (Whoever lands the commit can update the commit message, but you'd be saving them a bit of work if you do it yourself, plus that eliminates the possibility that they'll forget to do it.) |
Added comments to the tests to better describe what the test is doing.
895e8c6
to
c912eaa
Compare
@Trott rebased, and updated the pr. |
Landed in 7205e0a |
Added comments to the tests to better describe what the test is doing. PR-URL: nodejs#16003 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
Added comments to the tests to better describe what the test is doing. PR-URL: nodejs/node#16003 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
Added comments to the tests to better describe what the test is doing. PR-URL: #16003 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
Added comments to the tests to better describe what the test is doing. PR-URL: #16003 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>
Added comments to the tests to better describe what the test is doing.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
tests