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 coverage GitHub action #35653

Merged
merged 1 commit into from
Oct 17, 2020
Merged

test: add coverage GitHub action #35653

merged 1 commit into from
Oct 17, 2020

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Oct 15, 2020

This is a first attempt at adding a GitHub Action that pushes coverage results to codecov.

Fixes #35646

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

CC: @nodejs/testing

@bcoe bcoe requested a review from Trott October 15, 2020 02:39
@nodejs-github-bot 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 Oct 15, 2020
@watilde
Copy link
Member

watilde commented Oct 15, 2020

It's almost working, but the below three errors on CI seem to be a cause of the workflow failure:

file: internal/error-serdes.js error: Error: ENOENT: no such file or directory, open '/home/runner/work/node/node/lib/internal/error-serdes.js'
file: internal/process/main_thread_only.js error: Error: ENOENT: no such file or directory, open '/home/runner/work/node/node/lib/internal/process/main_thread_only.js'
file: internal/process/stdio.js error: Error: ENOENT: no such file or directory, open '/home/runner/work/node/node/lib/internal/process/stdio.js'

@codecov-io
Copy link

codecov-io commented Oct 15, 2020

Codecov Report

Merging #35653 into master will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35653      +/-   ##
==========================================
- Coverage   96.87%   96.40%   -0.48%     
==========================================
  Files         212      220       +8     
  Lines       69641    73672    +4031     
==========================================
+ Hits        67466    71024    +3558     
- Misses       2175     2648     +473     
Impacted Files Coverage Δ
lib/internal/crypto/scrypt.js 28.33% <0.00%> (-71.67%) ⬇️
lib/internal/crypto/diffiehellman.js 92.47% <0.00%> (-6.50%) ⬇️
lib/internal/crypto/keys.js 94.99% <0.00%> (-4.22%) ⬇️
lib/internal/crypto/util.js 96.79% <0.00%> (-3.21%) ⬇️
lib/internal/crypto/sig.js 97.54% <0.00%> (-2.46%) ⬇️
lib/internal/source_map/prepare_stack_trace.js 94.95% <0.00%> (-1.69%) ⬇️
lib/internal/crypto/keygen.js 99.20% <0.00%> (-0.49%) ⬇️
lib/_http_server.js 98.34% <0.00%> (-0.19%) ⬇️
lib/fs.js 94.13% <0.00%> (-0.13%) ⬇️
lib/internal/modules/esm/translators.js 95.14% <0.00%> (-0.12%) ⬇️
... and 49 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2002d90...48de3fc. Read the comment docs.

@targos
Copy link
Member

targos commented Oct 15, 2020

I don't know if it's an issue. Codecov found many extra "reports":

==> Reading reports
    + ./tools/doc/coverage/tmp/coverage-5164-1602748881271-0.json bytes=890994
    + ./.github/workflows/coverage-linux.yml bytes=903
    + ./out/Release/.deps/home/runner/work/node/node/out/Release/obj.target/v8_initializers/gen/deps/v8/src/builtins/internal-coverage-tq-csa.o.d bytes=81699
    + ./out/Release/.deps/home/runner/work/node/node/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/debug/debug-coverage.o.d bytes=31478
    + ./out/Release/obj/gen/deps/v8/src/builtins/internal-coverage-tq-csa.cc bytes=23084
    + ./coverage/lcov.info bytes=1279096
    + ./deps/v8/test/inspector/cpu-profiler/coverage-expected.txt bytes=24142
    + ./deps/v8/test/inspector/cpu-profiler/coverage-block-expected.txt bytes=30502
    + ./deps/v8/test/inspector/debugger/side-effect-free-coverage-enabled-expected.txt bytes=92
    + ./deps/v8/src/builtins/internal-coverage.tq bytes=1480
    + ./deps/v8/src/debug/debug-coverage.cc bytes=28807

@Trott
Copy link
Member

Trott commented Oct 15, 2020

Non-blocking question: IIRC, the current nightly job combines Windows and POSIX coverage at codecov. Is the long-term plan to try to get that happening with this too? If so, does it make sense to add a "TODO" or something?

@bcoe
Copy link
Contributor Author

bcoe commented Oct 15, 2020

@thomasrockhu before we land this, I wanted to see if you have any feedback. My main questions being:

  1. our coverage report is quite large for the Node.js codebase, is it any concern on your end that we'd be uploading it for each PR?
  2. any recommendations on how we approach our configuration, keeping in mind we'd like to eventually combine Windows and Linux reports (so the upload will come in two parts) -- the concrete question I have about this, is how we avoid spamming the PR thread with multiple Codecov Reports, since the uploads will arrive out of sync.

@thomasrockhu
Copy link

thomasrockhu commented Oct 15, 2020

Hi @bcoe

  1. The current upload is only about 5MB, this is below any concerning upload size, so I think we should be good here
  2. As for configuration, this looks pretty standard, as I'm guessing there will be other jobs that will be running Windows builds. The only concern I have is if you start running a significant number of matrix jobs (which would result in uploading dozens and dozens of reports). My suggestion there would be to batch coverage reports in one Codecov upload job, but I don't think this is going to be a problem in the near future.
    As far as multiple Codecov reports, we generally post one comment and update it as we ingest coverage reports. This means you should only get one Codecov PR comment per PR.
    If you are concerned about getting a comment prematurely, there is the after_n_builds flag that will prevent notifications until we receive n number of uploads.

@bcoe bcoe requested a review from richardlau October 15, 2020 17:16
# TODO(bcoe): fix the couple tests that fail with the inspector enabled.
# The cause is most likely coverage's use of the inspector.
- name: Test
run: NODE_V8_COVERAGE=coverage/tmp make run-ci -j2 V=1 TEST_CI_ARGS="-p dots" || exit 0
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 a little flummoxed as to why having NODE_V8_COVERAGE inline works great, whereas having it in the global env: stanza resulted in bad counts ... but there you have it, it's worked twice in a row with the environment variable here.

"text",
"lcov"
],
"lines": 95,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can ratchet this number up as we improve the linux coverage #s, and it will result in a failure to the action if it drops below the threshold.

@bcoe
Copy link
Contributor Author

bcoe commented Oct 15, 2020

If you are concerned about getting a comment prematurely, there is the after_n_builds flag that will prevent notifications until we receive n number of uploads.

@thomasrockhu thank you 👏 ,

Sounds like we'll just turn this setting on if the delayed merging becomes an annoyance.

I'm really excited to get coverage as part of our immediate feedback loop, and appreciate the help you've given.

I don't know if it's an issue. Codecov found many extra "reports"

@targos I've fixed it so we should only be uploading one report from the coverage/ folder.

@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 15, 2020
@nodejs-github-bot
Copy link
Collaborator

@bcoe bcoe mentioned this pull request Oct 16, 2020
4 tasks
@bcoe
Copy link
Contributor Author

bcoe commented Oct 16, 2020

It's almost working, but the below three errors on CI seem to be a cause of the workflow failure:

@watilde I believe those errors you mentioned are false positives negatives in the c8 reporter, from file paths it had trouble opening (I'm betting they're just warnings).

It would be good to dig into these warnings, and into the three tests that fail under coverage. If we land this, and make coverage one of the signals on PRs, perhaps it will help motivate us to dig into these problems.

@bcoe bcoe added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 17, 2020
@github-actions

This comment has been minimized.

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 17, 2020
@bcoe bcoe removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 17, 2020
PR-URL: nodejs#35653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@Trott Trott merged commit bb2945e into nodejs:master Oct 17, 2020
@Trott
Copy link
Member

Trott commented Oct 17, 2020

Landed in bb2945e

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>
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>
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
PR-URL: #35653
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
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
@BethGriggs
Copy link
Member

@bcoe is this PR expected to land on the v14.x release line?

I cherry-picked it into the v14.15.2-proposal, but the action is failing on the release proposal PR (refs: #36476 (comment)). Just trying to figure out if there's real failure we need to address, or the PR should not land on v14.x.

@bcoe
Copy link
Contributor Author

bcoe commented Dec 18, 2020

@BethGriggs I think it's okay to keep this action on the main branch, and not back-port. My hope is that it gives us insights into our test coverage going forward.

@targos targos added dont-land-on-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-blocked-v14.x labels Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. 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.

Fix Node.js' code coverage/code coverage job
10 participants