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

Add coverage to CI #2072

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Add coverage to CI #2072

merged 1 commit into from
Jun 7, 2022

Conversation

franmomu
Copy link
Contributor

@franmomu franmomu commented Jun 3, 2022

I guess once merged it will have something to compare 🤔

@ruflin
Copy link
Owner

ruflin commented Jun 7, 2022

+1 on adding this. The concern I have is on what diff it will show for each PR? Will it just compare the lines changed or will now each PR have a very lengthy list like this PR does? We do not have 100% coverage and I don't think it is the goal, it is more about good coverage (whatever that means).

@franmomu
Copy link
Contributor Author

franmomu commented Jun 7, 2022

+1 on adding this. The concern I have is on what diff it will show for each PR? Will it just compare the lines changed or will now each PR have a very lengthy list like this PR does? We do not have 100% coverage and I don't think it is the goal, it is more about good coverage (whatever that means).

It will only compare the lines changed like in doctrine/orm#9377 for example. Yeah it will also show if a change modifies the coverage significantly.

@ruflin
Copy link
Owner

ruflin commented Jun 7, 2022

That is what I'm hoping for. The odd part in this PR is that I see all these:

Screenshot 2022-06-07 at 15 18 08

I'm now wondering if this is a beta feature I have enabled or everyone is seeing this under "Files changed"?

@franmomu
Copy link
Contributor Author

franmomu commented Jun 7, 2022

Yeah I see that too, I guess is what codecov is returning in https://github.com/ruflin/Elastica/pull/2072/checks?check_run_id=6726796537

In theory, next PRs won't show those annotations.

@ruflin ruflin merged commit fc267fc into ruflin:master Jun 7, 2022
@ruflin
Copy link
Owner

ruflin commented Jun 7, 2022

Lets try it out, we can always disable it if needed. We had coverage reporting in the past, not sure when I got lost. There is even still the badge in the README.md file linked to https://codecov.io/github/ruflin/Elastica?branch=master

@franmomu franmomu deleted the add_coverage branch June 7, 2022 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants