-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: ensure nextTick is not scheduled in exit #9555
Conversation
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>
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)