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

doc: instructions for generating coverage reports #15190

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@ssbrewster
Contributor

ssbrewster commented Sep 4, 2017

This PR adds instructions for generating coverage reports. I only added the instructions to BUILDING.md but not CONTRIBUTING.md because that defers to BUILDING.md in the Test section (See the BUILDING.md for more details.), but I'm happy to add the instructions to that document too if people think they're needed there.

Checklist
  • make -j4 test
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@refack

LGTM % a wording suggestion & Windows fix

Show outdated Hide outdated BUILDING.md
$ make coverage
```
This will generate coverage reports for both JavaScript and C++ tests (if you

This comment has been minimized.

@refack

refack Sep 4, 2017

Member

Suggestion:

... (if you
only want to generate a coverage report for the JavaScript tests then you do
not need to run the first command, i.e. `configure`).
@refack

refack Sep 4, 2017

Member

Suggestion:

... (if you
only want to generate a coverage report for the JavaScript tests then you do
not need to run the first command, i.e. `configure`).
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Sep 4, 2017

Member

@ssbrewster Thanks for this.

Member

refack commented Sep 4, 2017

@ssbrewster Thanks for this.

@@ -126,6 +126,17 @@ To run the tests:
$ make test
```
To run the tests and generate code coverage reports:

This comment has been minimized.

@TimothyGu

TimothyGu Sep 5, 2017

Member

Hey, thanks for writing it up!

From my limited experience with make coverage, it can have some quite surprising behavior. I'd suggest noting the following caveats in this section of the docs. Whether to note some or all of them is up to you!

  • It doesn't work on macOS (I think? /cc @Trott) It does!
  • The lib/ directory is overwritten during make coverage, and the original copied to lib_/. I'd always suggest checking in the lib/ directory to Git before running coverage reports in case something awry happens. See

    node/Makefile

    Lines 151 to 152 in 86e7c61

    if [ -d lib_ ]; then $(RM) -r lib; mv lib_ lib; fi
    mv lib lib_
    .
  • The initial make coverage run downloads some tools to the project root folder that are not .gitignored, which can be a nuisance.
  • All effects of make coverage (lib/ renaming, downloaded tools, raw and processed output) can be reverted with make coverage-clean
  • make coverage both builds and runs (i.e. make + make test); no way to split them properly unfortunately (one could coverage-build first, but when coverage-testing the JS files will get instrumented again, and the node binary rebuilt based on the instrumented JS files).
@TimothyGu

TimothyGu Sep 5, 2017

Member

Hey, thanks for writing it up!

From my limited experience with make coverage, it can have some quite surprising behavior. I'd suggest noting the following caveats in this section of the docs. Whether to note some or all of them is up to you!

  • It doesn't work on macOS (I think? /cc @Trott) It does!
  • The lib/ directory is overwritten during make coverage, and the original copied to lib_/. I'd always suggest checking in the lib/ directory to Git before running coverage reports in case something awry happens. See

    node/Makefile

    Lines 151 to 152 in 86e7c61

    if [ -d lib_ ]; then $(RM) -r lib; mv lib_ lib; fi
    mv lib lib_
    .
  • The initial make coverage run downloads some tools to the project root folder that are not .gitignored, which can be a nuisance.
  • All effects of make coverage (lib/ renaming, downloaded tools, raw and processed output) can be reverted with make coverage-clean
  • make coverage both builds and runs (i.e. make + make test); no way to split them properly unfortunately (one could coverage-build first, but when coverage-testing the JS files will get instrumented again, and the node binary rebuilt based on the instrumented JS files).

This comment has been minimized.

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

Thanks for your review @TimothyGu. I will document make coverage-clean. I'm also wondering if we should handle setting clean as an option on the command line i.e. make coverage --clean=true make coverage CLEAN=true so that the coverage reports can be generated and the clean-up done in one run.

I think this would be good but coverage-clean also removes the coverage/ dir with the reports on so if passing the option --clean CLEAN=true then we would want to leave the coverage/ dir intact. I don't think I see this as an issue as long as that dir is added to .gitignore. What do you think?

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

Thanks for your review @TimothyGu. I will document make coverage-clean. I'm also wondering if we should handle setting clean as an option on the command line i.e. make coverage --clean=true make coverage CLEAN=true so that the coverage reports can be generated and the clean-up done in one run.

I think this would be good but coverage-clean also removes the coverage/ dir with the reports on so if passing the option --clean CLEAN=true then we would want to leave the coverage/ dir intact. I don't think I see this as an issue as long as that dir is added to .gitignore. What do you think?

This comment has been minimized.

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

Also, I just noticed that on the install of istanbul-merge and nyc, npm generates a package-lock.json which isn't removed by make coverage-clean.

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

Also, I just noticed that on the install of istanbul-merge and nyc, npm generates a package-lock.json which isn't removed by make coverage-clean.

This comment has been minimized.

@TimothyGu

TimothyGu Sep 5, 2017

Member

I'm also wondering if we should handle setting clean as an option on the command line i.e. make coverage --clean=true

That would be nice, but GNU Make doesn't support doing that AFAIK. But GNU Make does support environment variables so you can do make coverage CLEAN=true. PR welcome!

Also, I just noticed that on the install of istanbul-merge and nyc, npm generates a package-lock.json which isn't removed by make coverage-clean.

Good catch! You can submit another pull request to change that if you'd like to, or if not please file an issue about it so we don't forget :)

@TimothyGu

TimothyGu Sep 5, 2017

Member

I'm also wondering if we should handle setting clean as an option on the command line i.e. make coverage --clean=true

That would be nice, but GNU Make doesn't support doing that AFAIK. But GNU Make does support environment variables so you can do make coverage CLEAN=true. PR welcome!

Also, I just noticed that on the install of istanbul-merge and nyc, npm generates a package-lock.json which isn't removed by make coverage-clean.

Good catch! You can submit another pull request to change that if you'd like to, or if not please file an issue about it so we don't forget :)

This comment has been minimized.

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

Great I will raise two other PRs and yes you're right my syntax was wrong on make coverage CLEAN=true :)

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

Great I will raise two other PRs and yes you're right my syntax was wrong on make coverage CLEAN=true :)

This comment has been minimized.

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

I'll wait to this to land before submitting the PR for handling passing clean as an option so that I can update the docs for that.

@ssbrewster

ssbrewster Sep 5, 2017

Contributor

I'll wait to this to land before submitting the PR for handling passing clean as an option so that I can update the docs for that.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 5, 2017

Member

It doesn't work on macOS (I think? /cc @Trott)

Seems to work fine for me on macOS so no need for that caveat as far as I can tell.

Member

Trott commented Sep 5, 2017

It doesn't work on macOS (I think? /cc @Trott)

Seems to work fine for me on macOS so no need for that caveat as far as I can tell.

ssbrewster added some commits Sep 4, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md
@lpinca

lpinca approved these changes Sep 5, 2017

@mhdawson

LGTM

@jasnell

jasnell approved these changes Sep 7, 2017

jasnell added a commit that referenced this pull request Sep 7, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: #15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 7, 2017

Member

Landed in 6e27fd7

Member

jasnell commented Sep 7, 2017

Landed in 6e27fd7

@jasnell jasnell closed this Sep 7, 2017

antoine-amara pushed a commit to antoine-amara/node that referenced this pull request Sep 7, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: nodejs#15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@ssbrewster ssbrewster deleted the ssbrewster:doc-test-coverage branch Sep 8, 2017

MylesBorins added a commit that referenced this pull request Sep 10, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: #15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 11, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: #15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 12, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: #15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit to addaleax/node that referenced this pull request Sep 13, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: nodejs#15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 20, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: #15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 20, 2017

Merged

v6.11.4 proposal #15506

MylesBorins added a commit that referenced this pull request Sep 26, 2017

doc: instructions for generating coverage reports
Add instructions for generating code coverage reports to BUILDING.md

PR-URL: #15190
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment