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

coverage: make coverage work for Node.js #23941

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Oct 28, 2018

This pull request gets coverage working for Node.js' own internal libraries, I've:

  • moved enabling coverage to earlier in the bootstrap process, so that it loads before any other JavaScript dependencies .
  • I've switched to using the binding directly, to avoid loading JS dependencies.
  • coverage now turns off async hooks before writing, which prevents a bunch of test failures.
  • I've updated a few other tests that failed under coverage.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Oct 28, 2018
@bcoe
Copy link
Contributor Author

bcoe commented Oct 28, 2018

CC: @nodejs/testing

@bcoe
Copy link
Contributor Author

bcoe commented Oct 28, 2018

@bcoe
Copy link
Contributor Author

bcoe commented Nov 3, 2018

landed in 4df8c26

@bcoe bcoe closed this Nov 3, 2018
@Trott
Copy link
Member

Trott commented Nov 3, 2018

Force pushed out. No metadata, 'coverage' is not a valid subsystem.

@Trott Trott reopened this Nov 3, 2018
@Trott
Copy link
Member

Trott commented Nov 3, 2018

Commits were landed since the last CI run, so running CI again: https://ci.nodejs.org/job/node-test-pull-request/18321/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 3, 2018
@Trott
Copy link
Member

Trott commented Nov 3, 2018

(I'll re-land this once CI is good.)

@Trott
Copy link
Member

Trott commented Nov 4, 2018

(Also: One more data point for a commit queue!!! People who land commits once in a while shouldn't have to remember/re-learn the process!)

@refack
Copy link
Contributor

refack commented Nov 4, 2018

Big commit-queue fan here, but node-core-utils is so usful motivation has gone down.

@Trott
Copy link
Member

Trott commented Nov 4, 2018

Landed in 616fac9

@Trott Trott closed this Nov 4, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 4, 2018
PR-URL: nodejs#23941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
@bcoe
Copy link
Contributor Author

bcoe commented Nov 4, 2018

sorry about that, I used git node to land, but forgot that I still needed to amend the message if it’s one commit to add meta information (was thinking this was magic).

@joyeecheung
Copy link
Member

joyeecheung commented Nov 4, 2018

So I am thinking...what if we implement a git node land --push or something instead of asking people to git push..? (That must be explicitly run by users even if there is only one commit of course, so there is more room for manual validation...but it at least allow more validation than a bare git push at the end and is more flexible than the pre-push hook)

@refack
Copy link
Contributor

refack commented Nov 4, 2018

git node land --push

Personally I'm not a big fan of the -- flags. Maybe have --final prompt for user input:

$ git node land NJILK
...
Would you like to push upstream now. If you do please type NJILK
> NJILK
git push usptream
:tada:

@joyeecheung
Copy link
Member

joyeecheung commented Nov 4, 2018

@refack

Personally I'm not a big fan of the -- flags

I don't really like them either, but couldn't think of anything better :(

Would you like to push upstream now. If you do please type NJILK

Sounds like a good idea to me, although what does NJILK mean?

EDIT: oh, the PR ID?

@refack
Copy link
Contributor

refack commented Nov 4, 2018

Sounds like a good idea to me, although what does NJILK mean?

the PR_ID

@refack
Copy link
Contributor

refack commented Nov 4, 2018

See: nodejs/node-core-utils#299

@codebytere
Copy link
Member

@bcoe do you think this is a candidate for backporting to v10.x?

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants