Skip to content
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

test: add windows and c++ coverage #35670

Closed
wants to merge 12 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Oct 16, 2020

Experiment to see if collecting coverage for Windows will "just work".

Refs: #35653
Refs: #35646
Fixes: #35696

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

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 16, 2020
@bcoe bcoe added the wip Issues and PRs that are still a work in progress. label Oct 16, 2020
@bcoe bcoe changed the title test: windows coverage GitHub action test: add windows and C++ coverage Oct 17, 2020
@bcoe bcoe force-pushed the windows-coverage-action branch from aaa926e to df981ea Compare October 17, 2020 16:13
@nodejs nodejs deleted a comment from codecov-io Oct 18, 2020
@@ -23,14 +21,22 @@ jobs:
python-version: ${{ env.PYTHON_VERSION }}
- name: Environment Information
run: npx envinfo
- name: Clone gcovr reporter
run: git clone -b 3.4 --depth=1 --single-branch https://github.com/gcovr/gcovr.git
- name: Clone patch for gcovr
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson do you remember why we're running gcovr from a branch and floating a patch? It would be nice to simplify this.

Copy link
Member

Choose a reason for hiding this comment

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

It been long enough that I don't remember the specifics. I assume it failed to run properly with out that patch. It's possible a later version of gcovr may have fixed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhdawson I've managed to get gcovr@4.2 working, installed directly from pip.

The trick was to continue running from the out/ folder, but to specify a --root pointing to the root project directory.

codecov.yml Show resolved Hide resolved
@bcoe bcoe removed the wip Issues and PRs that are still a work in progress. label Oct 18, 2020
@bcoe bcoe requested review from Trott, mhdawson, richardlau and watilde and removed request for mhdawson October 18, 2020 19:41
@bcoe
Copy link
Contributor Author

bcoe commented Oct 18, 2020

@richardlau @Trott @watilde @mhdawson , I think this is ready for review:

  • coverage is now collected for C++ using gcov, and for Windows.
  • coverage reports won't show up until both the Windows and linux coverage runs finish.
  • I've added codecov configuration that removes the graph from the report (the default message on the PR took up a lot of space.).

Why the drop in coverage?

This is the first time we've combined C++ and JavaScript coverage, and our C++ coverage is a bit below the JavaScript thresholds -- I also noticed that codecov.io's numbers are a bit lower than our uploaded reports, I think this is because it treats partial line misses the same as misses.

What's next

Once this has been running for a week or two, and we're happy, I think we should:

  • remove some of the coverage logic from our Makefile, I'd be happy to do this.
  • it would be nice to stop floating a patch for gcovr (it looks like we have been for years.).
  • should we consider redirecting coverage.nodejs.org, to the reports on codecov.io, and remove a section of the website we maintain?

@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 18, 2020
@bcoe bcoe changed the title test: add windows and C++ coverage test: add windows and c++ coverage Oct 18, 2020
@mhdawson
Copy link
Member

Is it expected that I have to log in to see what under and 266 more ?

image

It seems to want quite a bit of access to be authorized when I go to do that...

@mhdawson
Copy link
Member

Does remove some of the coverage logic from our Makefile, mean that you can no longer run coverage locally?

@bcoe bcoe requested a review from mhdawson October 20, 2020 05:09
@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 20, 2020
.github/workflows/coverage-linux.yml Show resolved Hide resolved
.github/workflows/coverage-linux.yml Show resolved Hide resolved
.github/workflows/coverage-windows.yml Show resolved Hide resolved
.github/workflows/coverage-windows.yml Show resolved Hide resolved
@bcoe
Copy link
Contributor Author

bcoe commented Oct 20, 2020

@mhdawson did you manage to open the reports? perhaps I can open an issue with codecov.io independently of this PR, since I don't believe this behavior is new.

I've noticed myself in the past, that when looking at Node.js' nightly reports, I sometimes get prompted to authenticate -- I wonder if it happens to folks who have a prior session with codecov.io that's expired.

@@ -9,7 +9,7 @@
"reporter": [
"html",
"text",
"lcov"
"cobertura"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping this gives us better partial coverage (I noticed C++ reports branch level coverage).

@nodejs nodejs deleted a comment from codecov-io Oct 20, 2020
@targos
Copy link
Member

targos commented Oct 20, 2020

@bcoe It also asks me to authenticate, even in an incognito window.

@bcoe
Copy link
Contributor Author

bcoe commented Oct 20, 2020

@thomasrockhu sorry to keep bothering you (I think we're on the right track to getting this configured appropriately for the project).

Folks are reporting that codecov.io is prompting them to authenticate to see the public reports:

@bcoe It also asks me to authenticate, even in an incognito window.
Is it expected that I have to log in to see what under and 266 more ?

Ideally there'd never be an authentication step for our users to view detailed reports, as the Node.js project is open source.

Is there a configuration setting I'm missing?

@bcoe
Copy link
Contributor Author

bcoe commented Oct 20, 2020

@targos @mhdawson is the authentication issue a blocker, or is it okay if I open a tracking issue on codecov.io?

I seem to not be getting the authentication screen myself, either in an incognito window, or in another browser -- but, I have seen the behavior you're describing before -- I was hoping it was a bug that had been addressed, since I haven't been seeing a login prompt myself.

@targos
Copy link
Member

targos commented Oct 20, 2020

It's not a blocker to me

@mhdawson
Copy link
Member

Thanks for following up on the auth. As long as its not something that will be required long term it's not a blocker for me.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@bcoe
Copy link
Contributor Author

bcoe commented Oct 22, 2020

Landed in 7657f62

@bcoe bcoe closed this Oct 22, 2020
@bcoe bcoe deleted the windows-coverage-action branch October 22, 2020 02:46
bcoe added a commit that referenced this pull request Oct 22, 2020
Collect Windows and C++ coverage. Configure codecov so that
comments are more concise and are only left when coverage
varies.

PR-URL: #35670
Fixes: #35696
Refs: #35653
Refs: #35646
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@thomasrockhu
Copy link

thomasrockhu commented Oct 22, 2020

Hi @bcoe, I noticed you opened this ticket so I'll address the issue there. Users shouldn't be seeing any problems viewing open source reports, so there's definitely an issue. Thanks for raising it up!

targos pushed a commit that referenced this pull request Nov 3, 2020
Collect Windows and C++ coverage. Configure codecov so that
comments are more concise and are only left when coverage
varies.

PR-URL: #35670
Fixes: #35696
Refs: #35653
Refs: #35646
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos targos mentioned this pull request Nov 3, 2020
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
Collect Windows and C++ coverage. Configure codecov so that
comments are more concise and are only left when coverage
varies.

PR-URL: #35670
Fixes: #35696
Refs: #35653
Refs: #35646
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
Collect Windows and C++ coverage. Configure codecov so that
comments are more concise and are only left when coverage
varies.

PR-URL: #35670
Fixes: #35696
Refs: #35653
Refs: #35646
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coverage report left in comments is "noisy"