Skip to content
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

Merged
merged 1 commit into from
Nov 15, 2016

Conversation

Fishrock123
Copy link
Contributor

@Fishrock123 Fishrock123 commented Nov 11, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected 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)

@Fishrock123 Fishrock123 added process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests. labels Nov 11, 2016
@Fishrock123
Copy link
Contributor Author

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 Fishrock123 merged commit f65a48f into nodejs:master Nov 15, 2016
@Fishrock123 Fishrock123 deleted the nexttick-coverage branch November 15, 2016 14:26
@Trott
Copy link
Member

Trott commented Nov 15, 2016

I notice this and the other PR merged by @Fishrock123 around the same time does not have the Landed in <commit hash> comment. Is that unnecessary because GitHub provides the hash in the "merged commit" notification? Or is that an oversight and there ought to be a Landed in ... comment?

@Fishrock123
Copy link
Contributor Author

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

@sam-github
Copy link
Contributor

Landed in ... isn't part of https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#technical-howto, I often do it, it seems traditional, but its nice to have less steps if they aren't necessary.

@Fishrock123
Copy link
Contributor Author

You do need to link to the commits somehow, whether stated or not (and should be stated).

@gibfahn
Copy link
Member

gibfahn commented Nov 15, 2016

@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.

@sam-github
Copy link
Contributor

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.

@Fishrock123
Copy link
Contributor Author

A large problem is that the COLLABORATOR_GUIDE is simply too verbose.

addaleax pushed a commit that referenced this pull request Nov 22, 2016
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>
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
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>
MylesBorins pushed a commit that referenced this pull request Dec 20, 2016
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>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
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>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
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>
This was referenced Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants