test: refactor coverage logic by bcoe · Pull Request #35767 · nodejs/node · GitHub

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly.

The coverage logic in the Makefile had become a bit crufty:

it had logic specific to nyc, the coverage tool we were using prior to c8.
the instructions for collecting coverage reports locally were more complex than necessary (given improvements we've made).

CC: @nodejs/testing

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly.

You can just set NODE_V8_COVERAGE, and then run tests any which way.

I was having trouble on OSX with left over gcno and gcda files, I believe it's safe to simply remove all files generated by gcov.

these options are encapsulated in the .nycrc file.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.91%. Comparing base (f44029e) to head (cd901bb).

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #35767    +/-   ##
========================================
  Coverage   87.90%   87.91%            
========================================
  Files         477      477            
  Lines      113172   113090    -82     
  Branches    25428    24628   -800     
========================================
- Hits        99480    99419    -61     
+ Misses       7991     7956    -35     
- Partials     5701     5715    +14

see 89 files with indirect coverage changes

LGTM with or without my suggestions/comments addressed

Co-authored-by: Rich Trott <rtrott@gmail.com>

@nodejs/build-files

Co-authored-by: Rich Trott <rtrott@gmail.com>

It sure seems as if the Raspberry Pi build failures are related to the Makefile changes here, but I'm not sure how?

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

In the same vein as "quit and restart the program to see if the problem goes away", I've kicked off a CI with Rebuild rather than Resume Build to see if that fixes it....

It sure seems as if the Raspberry Pi build failures are related to the Makefile changes here, but I'm not sure how?

@Trott odd, I don't think any of the coverage rules should be being executed right?

CI: https://ci.nodejs.org/job/node-test-pull-request/33849/ ✅

@Trott restarting worked, but I'm slightly concerned it took so many restarts?

@Trott restarting worked, but I'm slightly concerned it took so many restarts?

Perhaps the other times were all Resume Builds and not Rebuilds and that is why the results were the same?

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Landed in ba907ff

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

test: refactor coverage logic

5877a15

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly.

bcoe requested review from Trott, addaleax and mhdawson October 23, 2020 03:09

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Oct 23, 2020

bcoe commented Oct 23, 2020

View reviewed changes

Trott reviewed Oct 23, 2020

View reviewed changes

Comment thread BUILDING.md Outdated

Trott reviewed Oct 23, 2020

View reviewed changes

Comment thread Makefile Outdated

Trott approved these changes Oct 23, 2020

View reviewed changes

nodejs deleted a comment Oct 23, 2020

Benjamin E. Coe and others added 2 commits October 23, 2020 09:59

Apply suggestions from code review

31359a5

Co-authored-by: Rich Trott <rtrott@gmail.com>

chore: address code review

025a363

This comment has been minimized.

Sign in to view

bcoe added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 23, 2020

Trott reviewed Oct 23, 2020

View reviewed changes

Comment thread Makefile Outdated

Update Makefile

cd901bb

Co-authored-by: Rich Trott <rtrott@gmail.com>

Trott approved these changes Oct 23, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

joyeecheung approved these changes Oct 25, 2020

View reviewed changes

bcoe pushed a commit that referenced this pull request Oct 25, 2020

test: refactor coverage logic

ba907ff

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

bcoe closed this Oct 25, 2020

bcoe deleted the coverage-refactor branch October 25, 2020 14:24

targos pushed a commit that referenced this pull request Nov 3, 2020

test: refactor coverage logic

110ef8a

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

targos mentioned this pull request Nov 3, 2020

v15.1.0 release proposal #35948

Merged

richardlau mentioned this pull request Nov 20, 2020

403 Forbidden for the most recent coverage reports #36040

Closed

BethGriggs pushed a commit that referenced this pull request Dec 8, 2020

test: refactor coverage logic

92da13f

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BethGriggs pushed a commit that referenced this pull request Dec 10, 2020

test: refactor coverage logic

22e8076

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

BethGriggs mentioned this pull request Dec 10, 2020

v14.15.2 proposal #36476

Merged

targos pushed a commit that referenced this pull request Mar 3, 2021

test: refactor coverage logic

c5a61d3

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins pushed a commit that referenced this pull request Apr 6, 2021

test: refactor coverage logic

48e3f59

Cleanup logic in Makefile for coverage. Update BUILDING.md accordingly. PR-URL: #35767 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

-              	$(RM) out/$(BUILDTYPE)/obj.target/embedtest/src/*.gcno
-              	$(RM) out/$(BUILDTYPE)/obj.target/embedtest/test/embedding/*.gcno
+              	$(RM) -r coverage/tmp
+              	$(FIND) out/$(BUILDTYPE)/obj.target -name "*.gcda" -type f -delete

+              ```text
+              $ make coverage-clean
+              $ NODE_V8_COVERAGE=coverage/tmp python tools/test.py test/parallel/test-stream2-transform.js

               		| sed 's/<[^>]*>//g'| sed 's/ //g'
-              COV_REPORT_OPTIONS = --reporter=html \
-              	--temp-directory=out/$(BUILDTYPE)/.coverage --omit-relative=false \

test: refactor coverage logic by bcoe · Pull Request #35767 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: refactor coverage logic - #35767

test: refactor coverage logic#35767bcoe wants to merge 4 commits intonodejs:masternodejs/node:masterfrom bcoe:coverage-refactorbcoe/node-1:coverage-refactorCopy head branch name to clipboard

Conversation

bcoe commented Oct 23, 2020

Checklist

Uh oh!

bcoe Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

bcoe Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

bcoe Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Oct 23, 2020 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Trott commented Oct 23, 2020

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Trott commented Oct 25, 2020

Uh oh!

nodejs-github-bot commented Oct 25, 2020

Uh oh!

Trott commented Oct 25, 2020

Uh oh!

bcoe commented Oct 25, 2020

Uh oh!

nodejs-github-bot commented Oct 25, 2020 • edited by bcoe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe commented Oct 25, 2020

Uh oh!

Trott commented Oct 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe commented Oct 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test: refactor coverage logic#35767
bcoe wants to merge 4 commits into
nodejs:masterfrom
bcoe:coverage-refactor

codecov-io commented Oct 23, 2020 •
edited by codecov Bot
Loading

nodejs-github-bot commented Oct 25, 2020 •
edited by bcoe
Loading

Trott commented Oct 25, 2020 •
edited
Loading