Skip to content

/ node

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

Jump to bottom

test: refactor coverage logic #35767

Closed
wants to merge 4 commits into from
Closed

test: refactor coverage logic #35767

wants to merge 4 commits into from
+27 −40

Conversation

@bcoe
Copy link
Member

@bcoe bcoe commented Oct 23, 2020

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
@bcoe
test: refactor coverage logic
Loading status checks…
5877a15 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.
@bcoe bcoe requested review from Trott, addaleax and mhdawson Oct 23, 2020
@bcoe
bcoe reviewed Oct 23, 2020
View changes
BUILDING.md

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

This comment has been minimized.

Sign in to view
@bcoe

bcoe Oct 23, 2020
Author Member

You can just set NODE_V8_COVERAGE, and then run tests any which way.
Makefile Outdated
$(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

This comment has been minimized.

Sign in to view
@bcoe

bcoe Oct 23, 2020
Author Member

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.
Makefile
@@ -259,17 +250,10 @@ coverage-test: coverage-build
@grep -A3 Lines coverage/cxxcoverage.html | grep style \
| sed 's/<[^>]*>//g'| sed 's/ //g'

COV_REPORT_OPTIONS = --reporter=html \
--temp-directory=out/$(BUILDTYPE)/.coverage --omit-relative=false \

This comment has been minimized.

Sign in to view
@bcoe

bcoe Oct 23, 2020
Author Member

these options are encapsulated in the .nycrc file.
@codecov-io
Copy link

@codecov-io codecov-io commented Oct 23, 2020 •
edited

Codecov Report

Merging #35767 into master will decrease coverage by 8.49%.
The diff coverage is 73.80%.

@@            Coverage Diff             @@
##           master   #35767      +/-   ##
==========================================
- Coverage   96.40%   87.91%   -8.50%     
==========================================
  Files         223      477     +254     
  Lines       73685   113090   +39405     
  Branches        0    24628   +24628     
==========================================
+ Hits        71038    99419   +28381     
- Misses       2647     7956    +5309     
- Partials        0     5715    +5715
Impacted Files Coverage Δ
src/inspector_profiler.cc 76.17% <69.44%> (ø)
lib/v8.js 99.65% <100.00%> (-0.35%) ⬇️
src/inspector_profiler.h 86.95% <100.00%> (ø)
lib/internal/idna.js 55.55% <0.00%> (-11.12%) ⬇️
lib/internal/blocklist.js 88.70% <0.00%> (-10.49%) ⬇️
lib/internal/crypto/mac.js 73.45% <0.00%> (-9.48%) ⬇️
lib/internal/modules/esm/get_format.js 84.72% <0.00%> (-8.34%) ⬇️
lib/internal/crypto/aes.js 84.21% <0.00%> (-6.44%) ⬇️
lib/internal/crypto/dsa.js 85.28% <0.00%> (-6.04%) ⬇️
lib/internal/crypto/webcrypto.js 84.60% <0.00%> (-5.40%) ⬇️
... and 392 more
@Trott
Trott reviewed Oct 23, 2020
View changes
BUILDING.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Oct 23, 2020
View changes
Makefile Outdated Show resolved Hide resolved
@Trott
Trott approved these changes Oct 23, 2020
View changes
Copy link
Member

@Trott Trott left a comment

LGTM with or without my suggestions/comments addressed
@nodejs nodejs deleted a comment from mutza974 Oct 23, 2020
@nodejs nodejs deleted a comment from mutza974 Oct 23, 2020
@nodejs nodejs deleted a comment from mutza974 Oct 23, 2020
@nodejs nodejs deleted a comment from mutza974 Oct 23, 2020
@nodejs nodejs deleted a comment from mutza974 Oct 23, 2020
@nodejs nodejs deleted a comment from mutza974 Oct 23, 2020
@nodejs nodejs deleted a comment from mutza974 Oct 23, 2020
bcoe and others added 2 commits Oct 23, 2020
@bcoe @Trott
Apply suggestions from code review
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
31359a5 
Co-authored-by: Rich Trott <rtrott@gmail.com>
@bcoe
chore: address code review
Loading status checks…
025a363
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@bcoe bcoe added the author ready label Oct 23, 2020
@Trott
Copy link
Member

@Trott Trott commented Oct 23, 2020

@nodejs/build-files
@Trott
Trott reviewed Oct 23, 2020
View changes
Makefile Outdated Show resolved Hide resolved
@bcoe @Trott
Update Makefile
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
cd901bb 
Co-authored-by: Rich Trott <rtrott@gmail.com>
@Trott
Trott approved these changes Oct 23, 2020
View changes
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@nodejs-github-bot

This comment has been hidden.

Sign in to view
@Trott
Copy link
Member

@Trott Trott commented Oct 25, 2020

It sure seems as if the Raspberry Pi build failures are related to the Makefile changes here, but I'm not sure how?
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Oct 25, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/33848/
@Trott
Copy link
Member

@Trott Trott commented Oct 25, 2020

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....
@bcoe
Copy link
Member Author

@bcoe bcoe commented Oct 25, 2020

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?
@nodejs-github-bot
Copy link

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

CI: https://ci.nodejs.org/job/node-test-pull-request/33849/ ✅
@bcoe
Copy link
Member Author

@bcoe bcoe commented Oct 25, 2020

@Trott restarting worked, but I'm slightly concerned it took so many restarts?
@joyeecheung
joyeecheung approved these changes Oct 25, 2020
View changes
@Trott
Copy link
Member

@Trott Trott commented Oct 25, 2020 •
edited

@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?
bcoe added a commit that referenced this pull request Oct 25, 2020
@bcoe
test: refactor coverage logic
Verified
This commit was signed with a verified signature.
bcoe Benjamin E. Coe
GPG key ID: 84D97FAF3C07DF69 Learn about signing commits
Loading status checks…
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
Copy link
Member Author

@bcoe bcoe commented Oct 25, 2020

Landed in ba907ff
@bcoe bcoe closed this Oct 25, 2020
@bcoe bcoe deleted the bcoe:coverage-refactor branch Oct 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott

@joyeecheung joyeecheung

@addaleax addaleax

@mhdawson mhdawson

Assignees
No one assigned
Labels
author ready build doc
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@bcoe @codecov-io @nodejs-github-bot @Trott @joyeecheung
You can’t perform that action at this time.