ci: Add test coverage job #27547
ci: Add test coverage job #27547
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
aureleoules
force-pushed
the
2023-04-ci-coverage
branch
2 times, most recently
from
2671acc
to
76cb418
Compare
|
A backwards compatibility test seems to fail because of BDB, I will fix it if this PR gets Concept ACKed. |
|
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?) |
|
Also, if the other commits are self-motivated and stand-alone, they can be submitted/reviewed/tested in a separate pull |
aureleoules
force-pushed
the
2023-04-ci-coverage
branch
from
76cb418
to
02976e5
Compare
You can toggle the annotations in the Files tab client-side using the It would be nice if Drahtbot could link to the Codecov report also.
I updated the commits descriptions, thanks.
I think they are small enough to be reviewed in this pull request but let me know if you want me to split them. |
There was a problem hiding this comment.
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?
They are needed in order to create a coverage report than can be uploaded to Codecov.
This allows us to add specify additional arguments to the test_runner when running test coverage.
aureleoules
force-pushed
the
2023-04-ci-coverage
branch
5 times, most recently
from
8dc19fc
to
0baadae
Compare
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. |
aureleoules
force-pushed
the
2023-04-ci-coverage
branch
from
0baadae
to
9196b48
Compare
Yes there is. Done in 320eb03. |
Uploads a test coverage report to Codecov for each pull request and master.
aureleoules
force-pushed
the
2023-04-ci-coverage
branch
from
320eb03
to
a583ff9
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
There was a problem hiding this comment.
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: testing aureleoules/bitcoin#5 (comment).
- 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
Afrom the keyboard haha, I like this!
| env: | ||
| << : *CIRRUS_EPHEMERAL_WORKER_TEMPLATE_ENV | ||
| MAKEJOBS: "-j4" | ||
| CODECOV_TOKEN: ENCRYPTED[!ecde4475f93a3607c87a3abaea30a6692c19c658e1b924130ad15b317e44d5f1abdd596984036eada4eeded24384cbd2!] |
There was a problem hiding this comment.
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 (#27547 (review)) to it. Maybe the GitLab mirror can be used for this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for the reviews.
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. |
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.
Example
It can also generate a GitHub comment with a brief summary of the coverage: aureleoules#5 (comment).
Though, that may be undesirable considering Drahtbot already comments on pulls. It can be turned off.
Example
These are the permissions Codecov needs to work:
19212b4 also fixes #26736.