test: exclude write-coverage from coverage report #15194

bcoe · 2017-09-05T05:39:57Z

With a goal of pitching in/familiarizing myself with the Node.js codebase, I wanted to help inch test coverage towards 100%.

With this in mind, I noticed that write-coverage.js is not tested. Since this is a utility used for outputting test coverage reports it seemed appropriate to add this to a list of excluded files (this just became possible).

In this pull request:

I've added a .nycrc configuration file, which can be used to configure the nyc test coverage tool.
I've added an exclude rule that removes write-coverage.js from coverage reports.
I've pulled reporter configuration into the .nycrc config file (I also added the additional text reporter, which I find provides useful high-level information for folks playing the test-coverage MMORPG).

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test, coverage

TimothyGu

Thanks for stopping by!

While these changes mostly LGTM, I would encourage you to look at our commit message guidelines, and it would be awesome if you could modify the commit messages to conform to those guidelines. If not, we'll fix them upon landing.

TimothyGu · 2017-09-05T05:48:29Z

.gitignore

@@ -13,6 +13,7 @@
 !.gitkeep
 !.mailmap
 !.remarkrc
+!.nycrc


alphabetical order

TimothyGu · 2017-09-05T05:49:12Z

.nycrc

@@ -0,0 +1,6 @@
+{
+  "exclude": [
+    "**/internal/process/write-coverage.js"


Is the wildcard necessary? It'll always be in lib/internal/process, unless the absolute path is used for comparison?

@TimothyGu because the Makefile passes an absolute path for the test coverage's working directory:

--temp-directory "/Users/benjamincoe/bcoe/node/.cov_tmp"

nyc ends up working with the path:

/Users/benjamincoe/bcoe/node/lib_/internal/process/write-coverage.js when the file is processed by the exclude glob.

tldr; we definitely don't want to bake /Users/benjamincoe/bcoe into the exclude rule, and it seemed like prefixing with the ** was the easiest solution.

@bcoe Makes sense 👍

Added a .nyrc configuration file that can be used to configure test coverage. Added an exclude rule that removes write-coverage.js from coverage reports. Pulled reporter configuration into .nycrc and added an additional text reporter.

bcoe · 2017-09-05T06:46:32Z

@TimothyGu I think I've amended my commits appropriately; Is the test prefix reasonable for this? Wasn't quite sure what was appropriate given I'm not modifying code within a specific subsystem).

TimothyGu · 2017-09-05T07:17:11Z

@bcoe Either test or build should be appropriate for this, build more so probably though.

mhdawson · 2017-09-05T19:45:21Z

.nycrc

+  "exclude": [
+    "**/internal/process/write-coverage.js"
+  ],
+  "reporter": ["html", "text"]


Does adding the "text" reporter here affect the default output ? Just wondering if results on https://coverage.nodejs.org/ will be affected.

@mhdawson I don't think it should; text will result in a nice summary being written to the terminal, for the user who just ran tests. html results in an index.html (etc) being written to the ./coverage folder, which I believe is what is in turn used on coverage.nodejs.org.

@addaleax I think I'm correct on this one?

Yes, that sounds right to me :)

mhdawson

LGTM

mhdawson · 2017-09-06T18:53:48Z

@bcoe thanks for improving the coverage output :)

mhdawson · 2017-09-06T18:55:13Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/9965/

BridgeAR · 2017-09-08T03:34:01Z

@bcoe congratulations to your first commit to Node.js! 🎉

Landed in e9442d1

Added a .nyrc configuration file that can be used to configure test coverage. Added an exclude rule that removes write-coverage.js from coverage reports. Pulled reporter configuration into .nycrc and added an additional text reporter. PR-URL: #15194 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Added a .nyrc configuration file that can be used to configure test coverage. Added an exclude rule that removes write-coverage.js from coverage reports. Pulled reporter configuration into .nycrc and added an additional text reporter. PR-URL: nodejs#15194 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins · 2017-09-20T20:10:21Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

bcoe · 2017-09-20T23:21:07Z

@MylesBorins my plan was to continue working on a few coverage related issues in my spare time; the answer to your question depends; are we comfortable with moving the main branch towards higher coverage, without inching up the coverage on v6? This might be my temptation, just because it will become a bit of a hassle to keep back-porting all these minor changes to the test/build process, I could be convinced otherwise though (CC: @addaleax, @TimothyGu?).

I don't yet have the commit bit on Node, so I think I might need to lean on the kindness of strangers to add that additional label 😛

MylesBorins · 2017-09-21T14:34:54Z

@bcoe backporting the minor changes are not a huge hassel as long as we maintain a small delta.

MylesBorins · 2018-01-11T18:21:15Z

ping re: backport

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. labels Sep 5, 2017

bcoe mentioned this pull request Sep 5, 2017

Add possibility to filter coveragemaps istanbuljs/nyc#637

Merged

TimothyGu reviewed Sep 5, 2017

View reviewed changes

bcoe changed the title ~~test: exclude write-coverage.js from test-coverage report~~ test: exclude write-coverage.js from coverage report Sep 5, 2017

bcoe changed the title ~~test: exclude write-coverage.js from coverage report~~ test: exclude write-coverage from coverage report Sep 5, 2017

bcoe force-pushed the nyc-config branch 2 times, most recently from 3a1e6fe to 13f6400 Compare September 5, 2017 06:34

TimothyGu approved these changes Sep 5, 2017

View reviewed changes

test: exclude write-coverage from coverage report

cd3ddc2

Added a .nyrc configuration file that can be used to configure test coverage. Added an exclude rule that removes write-coverage.js from coverage reports. Pulled reporter configuration into .nycrc and added an additional text reporter.

bcoe force-pushed the nyc-config branch from 13f6400 to cd3ddc2 Compare September 5, 2017 06:42

mhdawson reviewed Sep 5, 2017

View reviewed changes

refack approved these changes Sep 5, 2017

View reviewed changes

lpinca approved these changes Sep 5, 2017

View reviewed changes

addaleax approved these changes Sep 5, 2017

View reviewed changes

bcoe mentioned this pull request Sep 5, 2017

NYC v11.2.0 exclude file with sourcemap from coverage istanbuljs/nyc#666

Closed

mhdawson approved these changes Sep 6, 2017

View reviewed changes

jasnell approved these changes Sep 6, 2017

View reviewed changes

BridgeAR approved these changes Sep 8, 2017

View reviewed changes

BridgeAR closed this Sep 8, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the lts-watch-v6.x label Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: exclude write-coverage from coverage report #15194

test: exclude write-coverage from coverage report #15194

bcoe commented Sep 5, 2017 •

edited

TimothyGu left a comment

TimothyGu Sep 5, 2017

TimothyGu Sep 5, 2017

bcoe Sep 5, 2017

TimothyGu Sep 5, 2017

bcoe commented Sep 5, 2017

TimothyGu commented Sep 5, 2017

mhdawson Sep 5, 2017

bcoe Sep 5, 2017

addaleax Sep 5, 2017

mhdawson left a comment

mhdawson commented Sep 6, 2017

mhdawson commented Sep 6, 2017

BridgeAR commented Sep 8, 2017

MylesBorins commented Sep 20, 2017

bcoe commented Sep 20, 2017 •

edited

MylesBorins commented Sep 21, 2017

MylesBorins commented Jan 11, 2018

test: exclude write-coverage from coverage report #15194

test: exclude write-coverage from coverage report #15194

Conversation

bcoe commented Sep 5, 2017 • edited

Checklist

Affected core subsystem(s)

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Sep 5, 2017

Choose a reason for hiding this comment

TimothyGu Sep 5, 2017

Choose a reason for hiding this comment

bcoe Sep 5, 2017

Choose a reason for hiding this comment

TimothyGu Sep 5, 2017

Choose a reason for hiding this comment

bcoe commented Sep 5, 2017

TimothyGu commented Sep 5, 2017

mhdawson Sep 5, 2017

Choose a reason for hiding this comment

bcoe Sep 5, 2017

Choose a reason for hiding this comment

addaleax Sep 5, 2017

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Sep 6, 2017

mhdawson commented Sep 6, 2017

BridgeAR commented Sep 8, 2017

MylesBorins commented Sep 20, 2017

bcoe commented Sep 20, 2017 • edited

MylesBorins commented Sep 21, 2017

MylesBorins commented Jan 11, 2018

bcoe commented Sep 5, 2017 •

edited

bcoe commented Sep 20, 2017 •

edited