doc: instructions for generating coverage reports

[ssbrewster]

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

[refack]

Windows (i.e. vcbuild) does not have a counterpart, so you can remove this section

[ssbrewster]

Thanks for your review @refack I will update the doc.

[refack]

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]

@ssbrewster Thanks for this.

[TimothyGu]

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

[ssbrewster]

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]

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.

[TimothyGu]

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

[ssbrewster]

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

[ssbrewster]

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]

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.

[mhdawson]

LGTM

[jasnell]

Landed in 6e27fd7