GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: refactor coverage logic by bcoe · Pull Request #35767 · nodejs/node

Conversation

@bcoe

@bcoe bcoe commented Oct 23, 2020

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
bcoe
test: refactor coverage logic
5877a15 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.
@bcoe
bcoe requested review from Trott, addaleax and mhdawson October 23, 2020 03:09
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. doc Issues and PRs related to Node.js documentation. labels Oct 23, 2020
bcoe
bcoe commented Oct 23, 2020
View reviewed changes
Comment thread BUILDING.md

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

@bcoe bcoe Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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

@bcoe bcoe Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Makefile
| sed 's/<[^>]*>//g'| sed 's/ //g'

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

@bcoe bcoe Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

these options are encapsulated in the .nycrc file.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@codecov-io

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Oct 23, 2020
View reviewed changes
Comment thread BUILDING.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Oct 23, 2020
View reviewed changes
Comment thread Makefile Resolved

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Oct 23, 2020
View reviewed changes

@Trott Trott left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM with or without my suggestions/comments addressed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs nodejs deleted a comment Oct 23, 2020
@nodejs nodejs deleted a comment Oct 23, 2020
@nodejs nodejs deleted a comment Oct 23, 2020
@nodejs nodejs deleted a comment Oct 23, 2020
@nodejs nodejs deleted a comment Oct 23, 2020
@nodejs nodejs deleted a comment Oct 23, 2020
@nodejs nodejs deleted a comment Oct 23, 2020
Benjamin E. Coe and others added 2 commits October 23, 2020 09:59
Benjamin E. Coe @Trott
Apply suggestions from code review
31359a5 
Co-authored-by: Rich Trott <[email protected]>
bcoe
chore: address code review
025a363
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@bcoe bcoe added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Oct 23, 2020
@Trott

Trott commented Oct 23, 2020

Copy link
Copy Markdown
Member

@nodejs/build-files
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott reviewed Oct 23, 2020
View reviewed changes
Comment thread Makefile Resolved

Uh oh!

There was an error while loading. Please reload this page.
Benjamin E. Coe @Trott
Update Makefile
cd901bb 
Co-authored-by: Rich Trott <[email protected]>
Trott
Trott approved these changes Oct 23, 2020
View reviewed changes
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott

Trott commented Oct 25, 2020

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Oct 25, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Oct 25, 2020

Copy link
Copy Markdown
Member

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....
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bcoe

bcoe commented Oct 25, 2020

Copy link
Copy Markdown
Contributor Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bcoe

bcoe commented Oct 25, 2020

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung approved these changes Oct 25, 2020
View reviewed changes
@Trott

Trott commented Oct 25, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bcoe pushed a commit that referenced this pull request Oct 25, 2020
bcoe
test: refactor coverage logic
ba907ff 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.

PR-URL: #35767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@bcoe

bcoe commented Oct 25, 2020

Copy link
Copy Markdown
Contributor Author

Landed in ba907ff
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bcoe bcoe closed this Oct 25, 2020
@bcoe
bcoe deleted the coverage-refactor branch October 25, 2020 14:24
targos pushed a commit that referenced this pull request Nov 3, 2020
bcoe @targos
test: refactor coverage logic
110ef8a 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.

PR-URL: #35767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos targos mentioned this pull request Nov 3, 2020
v15.1.0 release proposal #35948
Merged
@richardlau 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
bcoe @BethGriggs
test: refactor coverage logic
92da13f 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.

PR-URL: #35767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
bcoe @BethGriggs
test: refactor coverage logic
22e8076 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.

PR-URL: #35767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@BethGriggs 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
bcoe @targos
test: refactor coverage logic
c5a61d3 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.

PR-URL: #35767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 6, 2021
bcoe @MylesBorins
test: refactor coverage logic
48e3f59 
Cleanup logic in Makefile for coverage. Update BUILDING.md
accordingly.

PR-URL: #35767
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes
@addaleax addaleax Awaiting requested review from addaleax
@mhdawson mhdawson Awaiting requested review from mhdawson

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. doc Issues and PRs related to Node.js documentation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@bcoe @codecov-io @nodejs-github-bot @Trott @joyeecheung
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.