ci: Add test coverage job

aureleoules commented at 8:57 pm on April 30, 2023: member

This PR adds a new Cirrus job that generates test coverage reports for all pull requests and master using Codecov. It is free for open-source projects. We can now quickly determine whether a feature that a pull request adds is properly tested.

I’ve ran a few tests on my own fork, you can check it out here: https://app.codecov.io/gh/aureleoules/bitcoin and a test pull request: https://app.codecov.io/gh/aureleoules/bitcoin/pull/5.

Codecov has a nice feature that highlights the untested chunks of code directly in the GitHub’s “Files changed” tab of a pull request. See for example https://github.com/aureleoules/bitcoin/pull/5/files.

It can also generate a GitHub comment with a brief summary of the coverage: https://github.com/aureleoules/bitcoin/pull/5#issuecomment-1528823591. Though, that may be undesirable considering Drahtbot already comments on pulls. It can be turned off.

These are the permissions Codecov needs to work:

19212b413e518908fa73147b665d8599b7a2f016 also fixes #26736.

DrahtBot commented at 8:57 pm on April 30, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	dergoegge, pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27676 (macOS: Bump minimum required runtime version and prepare for building with upstream LLVM by theuni)
#21778 (build: LLVM 15 & LLD based macOS toolchain by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Apr 30, 2023

DrahtBot added the label CI failed on Apr 30, 2023

aureleoules force-pushed on Apr 30, 2023

aureleoules force-pushed on May 1, 2023

aureleoules commented at 12:49 pm on May 1, 2023: member

A backwards compatibility test seems to fail because of BDB, I will fix it if this PR gets Concept ACKed.

MarcoFalke commented at 1:30 pm on May 1, 2023: member

I think it is good to have coverage reports, but I am not sure if it is good to overlay them in the files tab. It might be too invasive? Maybe just a comment with a link is fine?

For the other commits: It would be good for reviewers to explain and motivate them, and explain why they are fine to do and what their tradeoffs are (especially not deleting the gcda files?)

MarcoFalke commented at 1:31 pm on May 1, 2023: member

Also, if the other commits are self-motivated and stand-alone, they can be submitted/reviewed/tested in a separate pull

aureleoules force-pushed on May 1, 2023

aureleoules commented at 1:50 pm on May 1, 2023: member

I think it is good to have coverage reports, but I am not sure if it is good to overlay them in the files tab. It might be too invasive? Maybe just a comment with a link is fine?

You can toggle the annotations in the Files tab client-side using the a key. Or we can fully disable the annotations feature with a custom codecov.yml config file but since they are togglable I think it’s fine.

It would be nice if Drahtbot could link to the Codecov report also.

For the other commits: It would be good for reviewers to explain and motivate them, and explain why they are fine to do and what their tradeoffs are (especially not deleting the gcda files?)

I updated the commits descriptions, thanks.

Also, if the other commits are self-motivated and stand-alone, they can be submitted/reviewed/tested in a separate pull

I think they are small enough to be reviewed in this pull request but let me know if you want me to split them.

in ci/test/06_script_b.sh:111 in 02976e5dec outdated

103@@ -104,3 +104,9 @@ if [ -z "$DANGER_RUN_CI_ON_HOST" ]; then
104   echo "Stop and remove CI container by ID"
105   docker container kill "${CI_CONTAINER_ID}"
106 fi
107+
108+if [ "$RUN_COVERAGE" = "true" ]; then
109+  CI_EXEC make cov
110+  CI_EXEC geninfo . -b src -o coverage.info
111+  CI_EXEC codecov

dergoegge commented at 9:22 am on May 18, 2023:

Maybe add an option to skip this step? then devs could run this job locally, without it failing to upload the reports.

aureleoules commented at 10:47 pm on May 27, 2023:

Done thanks

dergoegge commented at 9:30 am on May 18, 2023: member

Concept ACK

Thank you for working on this, I think this would be valuable to have.

It seems like it would be easy to replace codecov with something we could host on our own infra (should that become necessary). For now codecov seems fine to me though.

Read and write access to … pull requests

Does this mean codecov can push changes to pull request or just that it can comment?

ci: Do not remove gcda coverage files

They are needed in order to create a coverage report than can be
uploaded to Codecov.

2375606a36

ci: Custom functional tests runner args for coverage

This allows us to add specify additional arguments to the test_runner
when running test coverage.

9b8439ba23

aureleoules force-pushed on May 26, 2023

aureleoules force-pushed on May 27, 2023

aureleoules commented at 10:50 pm on May 27, 2023: member

Does this mean codecov can push changes to pull request or just that it can comment?

It seems that the permissions for “Pull request” are the following: https://docs.github.com/en/rest/overview/permissions-required-for-github-apps?apiVersion=2022-11-28#pull-requests

So I don’t think Codecov is able to push to PRs.

DrahtBot removed the label CI failed on May 27, 2023

in .cirrus.yml:371 in 0baadae17f outdated

365@@ -366,3 +366,16 @@ task:
366   << : *MAIN_TEMPLATE
367   env:
368     << : *CIRRUS_EPHEMERAL_WORKER_TEMPLATE_ENV
369+
370+task:
371+  name: 'code coverage [lunar]'

MarcoFalke commented at 6:49 am on May 29, 2023:

any reason to use lunar, when an Ubuntu LTS or debian bookworm can be used?

MarcoFalke commented at 6:50 am on May 29, 2023:

Also, could move up the task to group it with the other native linux tasks, away from the macos and android task down here?

aureleoules commented at 7:47 pm on May 30, 2023:

Done thanks. I rolled back to jammy.

MarcoFalke commented at 6:54 am on May 29, 2023: member

I presume there is a setting to disable the comment and just have DrahtBot add a link in the form of https://app.codecov.io/gh/{owner}/{repo}/pull/{number} to the summary comment?

aureleoules force-pushed on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

aureleoules commented at 7:49 pm on May 30, 2023: member

I presume there is a setting to disable the comment and just have DrahtBot add a link in the form of https://app.codecov.io/gh/{owner}/{repo}/pull/{number} to the summary comment?

Yes there is. Done in 320eb03aeb097f6d14757c66a1b0bf40d0473b88.

ci: Add test coverage job

Uploads a test coverage report to Codecov for each pull request and
master.

a9284f3a07

ci: Add initial codecov config file a583ff9bd1

aureleoules force-pushed on May 30, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label Needs rebase on Jun 14, 2023

DrahtBot commented at 3:04 pm on June 14, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

pablomartin4btc commented at 3:24 pm on June 23, 2023: contributor

Concept ACK.

I really like this feature, I find it very useful. Thanks @aureleoules.

I agree with @dergoegge on that perhaps we could replace it with something hosted on us as well and in the meaintime we can take advantage from Codecov.

As commented by @MarcoFalke on being a bit invasive:

one thing is the comment on the PR itself, where I can see it’s only one line now? I see your example here: https://github.com/aureleoules/bitcoin/pull/5#issuecomment-1528823591.
on the annotations, on the Files section of the PR, I’m looking at your example here: https://github.com/aureleoules/bitcoin/pull/5/files#annotation_10900357719. I was going to ask you again how to disable/ toggle off the actual comments from Codecov and just found out that you were referring in your comment actually to the key A from the keyboard haha, I like this!

in .cirrus.yml:330 in a583ff9bd1

325+      CI_IMAGE_NAME_TAG: ubuntu:jammy
326+      FILE_ENV: "./ci/test/00_setup_env_native_coverage.sh"
327+  env:
328+    << : *CIRRUS_EPHEMERAL_WORKER_TEMPLATE_ENV
329+    MAKEJOBS: "-j4"
330+    CODECOV_TOKEN: ENCRYPTED[!ecde4475f93a3607c87a3abaea30a6692c19c658e1b924130ad15b317e44d5f1abdd596984036eada4eeded24384cbd2!]

MarcoFalke commented at 11:43 am on August 22, 2023:

I wonder if this can be done completely outside of this repo. Otherwise, anyone can print this token and use it?

I think you can set up a mirror, that pulls all open pull requests and runs this task. Then DrahtBot can leave a comment (https://github.com/bitcoin/bitcoin/pull/27547#pullrequestreview-1448964603) to it. Maybe the GitLab mirror can be used for this?

MarcoFalke commented at 11:45 am on August 22, 2023:

Alternatively, it may be possible to use “differential coverage analysis” from https://github.com/linux-test-project/lcov/releases/tag/v2.0 and somehow publish the report via GitHub Actions CI.

MarcoFalke commented at 11:48 am on August 22, 2023:

Alternatively, it may be possible to run this completely outside of CI (locally or on a dedicated machine), which updates the summary comment, once and if the coverage report is ready.

aureleoules commented at 7:22 am on August 29, 2023: member

Thanks for the reviews.

Alternatively, it may be possible to run this completely outside of CI (locally or on a dedicated machine), which updates the summary comment, once and if the coverage report is ready.

I’ve actually been working on a self-hosted coverage tool for Bitcoin Core recently. It would run independently of this repo. So I’ll close this PR. I’ll probably comment on this PR once I have an MVP ready to be presented.

aureleoules closed this on Aug 29, 2023

ci: Add test coverage job #27547

Reviews

Conflicts