test: ensure nextTick is not scheduled in exit #9555
Conversation
Fishrock123
added
process
Fishrock123
force-pushed
the
nexttick-coverage
branch
from
3685de1 to
3ad531b
Compare
|
CI again cuz I think it was having some issues? https://ci.nodejs.org/job/node-test-pull-request/4840/ |
Previously our tests did not check this codepath as seen at coverage.nodejs.org PR-URL: nodejs#9555 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Fishrock123
force-pushed
the
nexttick-coverage
branch
from
3ad531b to
f65a48f
Compare
|
I notice this and the other PR merged by @Fishrock123 around the same time does not have the |
|
Uh, I'm not the only one that does this for our own commits which we can make github see as "merged", also this is in no way new. it is still important if you merged more than one commit though |
|
|
|
You do need to link to the commits somehow, whether stated or not (and should be stated). |
|
@sam-github It's actually in https://github.com/nodejs/node/blob/master/doc/onboarding.md#landing-prs-details (look for "landed in"). I think @BethGriggs is going to submit a PR to clarify that bit of the docs. On another note, it's a little odd that COLLABORATOR_GUIDE and onboarding.md seem to cover much of the same ground, epecially as you have onboarding-extras.md as well. |
|
Ouph, yes, maybe we can unify the 3 docs... :-(, I always follow the COLLABORATOR_GUIDE, now I wonder if I've been doing it wrong. |
|
A large problem is that the |
Previously our tests did not check this codepath as seen at coverage.nodejs.org PR-URL: #9555 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Previously our tests did not check this codepath as seen at coverage.nodejs.org PR-URL: nodejs#9555 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Previously our tests did not check this codepath as seen at coverage.nodejs.org PR-URL: #9555 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Previously our tests did not check this codepath as seen at coverage.nodejs.org PR-URL: #9555 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Previously our tests did not check this codepath as seen at coverage.nodejs.org PR-URL: #9555 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test, process
Description of change
Previously our tests did not check this codepath as seen at
coverage.nodejs.org
See https://coverage.nodejs.org/coverage-fb05e31466ac0bad/root/internal/process/next_tick.js.html (scroll down to the bottom).
CI: https://ci.nodejs.org/job/node-test-pull-request/4824/
(This patch was made live during https://www.twitch.tv/nodesource/v/100431274 if you'd like to see me working on this in retrospect. :P)