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.

Sign up for GitHub

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

Jump to bottom

doc: instructions for generating coverage reports #15190

Closed
wants to merge 2 commits into from
Closed

doc: instructions for generating coverage reports #15190

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0408aff
doc: instructions for generating coverage reports
ssbrewster Sep 4, 2017
1a560f1
doc: Document make coverage-clean
ssbrewster Sep 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions BUILDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,25 @@ To run the tests:
$ make test
```

To run the tests and generate code coverage reports:
Copy link
Member

@TimothyGu TimothyGu Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ssbrewster ssbrewster Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@ssbrewster ssbrewster Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@TimothyGu TimothyGu Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


```console
$ ./configure --coverage
$ make coverage
```

This will generate coverage reports for both JavaScript and C++ tests (if you
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`).

only want to run the JavaScript tests then you do not need to run the first
command `./configure --coverage`).

The `make coverage` command downloads some tools to the project root directory
and overwrites the `lib/` directory. To clean up after generating the coverage
reports:

```console
make coverage-clean
```

To build the documentation:

This will build Node.js first (if necessary) and then use it to build the docs:
Expand Down