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

#11744 Generate coverage report using only GitHub Actions #11745

Open
wants to merge 17 commits into
base: trunk
Choose a base branch
from

Conversation

adiroiban
Copy link
Member

@adiroiban adiroiban commented Nov 5, 2022

Scope and purpose

Fixes #11744

This tries to replace the functionality of codecov.io

For now, we have both custom coverage report and codecov.io in parallel

I would like to have this used in parallel for some time.

Once we are happy with this code, we can remove codecov.io

This will only fail the PR check on missing diff coverage.

There is no coverage badge with this code... we can look into the badge thing later, but for me the badge is not that important.

The coverate report should be visible inside github step summary

@adiroiban
Copy link
Member Author

Here is the coverage report

image

There is a comment with the details.

If PR/diff coverage is below 100% the check will fail.

This is done via https://github.com/twisted/twisted/actions/runs/3402219054/jobs/5657937027#step:8:1

diff-cover --fail-under=100 --compare-branch origin/trunk coverage.xml

needs-review

@chevah-robot chevah-robot requested a review from a team November 6, 2022 00:59
@glyph
Copy link
Member

glyph commented Nov 6, 2022

Hm. I want to get rid of codecov as soon as possible, but this is too significant a downgrade to start using as-is, I think. Mainly I want an easy link to the diff-cover HTML report so that I can quickly see what new uncovered lines we are dealing with. The inline line annotations in the github diff view are also super useful, although if we have to give those up, as long as I can quickly navigate through uncovered lines even if they're in a different window I think I'll be happy

@adiroiban
Copy link
Member Author

Thanks @glyph for the review and feedback.

The guys from codecov.io have put many years of work behind the service.
Is not easy to replace codecov.io during a weekend hack :p

But I think things are not that bad.

We can remove the coverage report output and only leave the diff-cover output.

Most PR should not contain too many changes, and at the same time should not contain too many uncovered lines.

So the output will include 2 or 3 snippets.

You can check here for an example of how missing coverage is reported as a comment

twisted/towncrier#451 (comment)


Mainly I want an easy link to the diff-cover HTML report so that I can quickly see what new uncovered lines we are dealing with

We have the diff-cover markdown output as a comment.

In theory, the HTML output and markdown output from diff-cover should provide the same level of information.

I think that what is missing, is to improve the markdown output to contain the same level of info as the HTML output.


My suggestion is to have the 2 systems in parallel for a while, and only use the diff-cover markdown ouput as a comment.

Contribute to the diff-cover project to improve the markdown report.

After a few PRs we can see codecov.io is our only option.


Storing the HTML file for easy view with a browser will complicate our CI system.
At the same time, I would like to keep our CI system simple and as independent as possible from the GitHub API or other external services.

I like inline annotations and I start working on it... but in practice, I think that diff-cover output should be enough.

The current coverage check/report is just a safety measure.
I don't think it should be used as a development tool.

Instead, we should improve the local development tools to make it easier to generate a coverage report.

The author of the PR needs to run the test locally anyway and can generate a local diff report.

This is what I use for towncrier and my projects at work.
I get a coverage error as part of the PR tests.
Then I run the checks locally. write more tests, re-run the tests and check the coverage locally... write more tests...etc.

@@ -0,0 +1,80 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Is there no similarly-functional thing here that is at least in Python? It's annoying enough to have to maintain our own build scripts in-repo rather than importing them from elsewhere, but to have to do it in a foreign programming language (with, ironically, no test coverage of its own) feels like it's likely to become a huge maintenance headache.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be implemented in Python. No problem.

I went with GitHub API helper since it's easy to experiment.

I have some local JS testing harness that helped me write this... but I am not proud of that code...so I have not added it here.

I didn't want to write a lot of code and tests for something that we decided that we don't want.

Testing will most probably involve a lot of injections since this makes heavy use of GitHub API.

If we decide that using a single comment is how we want to receive the coverage report, I can look into rewriting this in Python.

@exarkun
Copy link
Member

exarkun commented Nov 7, 2022

The author of the PR needs to run the test locally anyway and can generate a local diff report.

This is impossible for many contributions. I cannot run the tests locally on macOS and Windows. I rely on CI for coverage reporting for code specific to these platforms.

@adiroiban
Copy link
Member Author

I rely on CI for coverage reporting for code specific to these platforms.

I think that one of the main issue with codecov, is that it randomly fails to upload the coverage reports which results in failed jobs.

I guess that we can still have codecov.io reports available, but as optional.
That is, don't block the merge if codecov.io fails.

The merge is blocked only by the diff-cover report.

In this way, we can still check the codecov.io reports


diff-cover also generates coverage reports, but they can't compare to all the different coverage report types that you can get with codecov.io

@adiroiban adiroiban marked this pull request as draft September 13, 2023 23:33
@adiroiban
Copy link
Member Author

I am moving it as draft.
I think that lately codecov.io was stable enough, not to require a separate tool for coverage reporting.

@twisted twisted deleted a comment from github-actions bot Oct 12, 2023
@adiroiban adiroiban marked this pull request as ready for review October 12, 2023 21:53
@chevah-robot chevah-robot requested a review from a team October 12, 2023 21:53
@adiroiban
Copy link
Member Author

adiroiban commented Oct 12, 2023

I think this is ready for another review.

for now codecov.io is still kept.

The coverage step is here https://github.com/twisted/twisted/actions/runs/6501904701/job/17660361713?pr=11745

The status is here
image

Report is here https://github.com/twisted/twisted/actions/runs/6501904701?pr=11745#summary-17660361713

This delays the CI report with about 2 minutes, as it waits for coverage to be reported.

Also, combining all the covergage result for each job from parallel runs takes between 30-50 seconds for each job... but this is done in parallel.

needs-review

Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

OK, let's give it a shot.

name: Coverage report
runs-on: ubuntu-latest
# For code coverate only need to wait for test jobs.
# We want to alwasy run the coverage, even on
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We want to alwasy run the coverage, even on
# We want to always run the coverage, even on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate coverage reports using only GitHub Actions
4 participants