Skip to content

Use codecov/codecov-action #674

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 2, 2023
Merged

Use codecov/codecov-action #674

merged 6 commits into from
Apr 2, 2023

Conversation

skirpichev
Copy link
Collaborator

@skirpichev skirpichev commented Mar 29, 2023 •
edited
Loading

@skirpichev skirpichev force-pushed the coverage-action branch 7 times, most recently from 29b96aa to 947856f Compare March 31, 2023 05:03
cbm755
cbm755 reviewed Apr 1, 2023
View reviewed changes
cbm755
cbm755 reviewed Apr 1, 2023
View reviewed changes
Copy link
Contributor

@cbm755 cbm755 left a comment

Choose a reason for hiding this comment

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

I think this looks good, although I have no experience with coverage on GitHub (on GitLab its integrated into the web UI, here it looks like an external service is being used).

Perhaps before it was computing coverage but not uploading the artifacts elsewhere and this change makes it so the artifacts are uploaded to the third-party for viewing.

I cannot speak to whether that is wanted or not but this looks like it does accomplish that.

@skirpichev skirpichev requested a review from cbm755 April 2, 2023 01:39
@skirpichev skirpichev force-pushed the coverage-action branch 2 times, most recently from 7d3b103 to 54479d4 Compare April 2, 2023 02:59
@skirpichev
Copy link
Collaborator Author

FYI: coverage uploaded to https://app.codecov.io/gh/mpmath/mpmath, only status checks aren't enabled (I can't do this without Fredrik). But sometimes it's useful to have html/xml coverage data (e.g. codecov.io has bugs).

Now pypi things go to a separate commit and no more exclusions for doctests.

@cbm755
Copy link
Contributor

cbm755 commented Apr 2, 2023

Thanks for explanation. I have now learned what app.codecov.io is. I don't understand how it was uploading there before this change, but it appears it somehow was.

Thus this is not a change from the project to adopt a new service but rather one it was already using.

Is that correct? If so, I have no objections.

(OTOH, if the previous CI was not uploading artifacts to codecov.io, then I think this is something that Fredrik would have to approve).

@cbm755
Copy link
Contributor

cbm755 commented Apr 2, 2023

that Fredrik would have to approve).

I should say "someone other than me would have to approve".

@skirpichev
Copy link
Collaborator Author

Thus this is not a change from the project to adopt a new service

Nop. Just little info for debugging code coverage. (And BTW the coverage html output is more informative than the codecov.io)

cbm755
cbm755 approved these changes Apr 2, 2023
View reviewed changes
Copy link
Contributor

@cbm755 cbm755 left a comment

Choose a reason for hiding this comment

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

My concerns addressed, +1 to merge.

@skirpichev skirpichev merged commit 98c48fa into mpmath:master Apr 2, 2023
@skirpichev skirpichev deleted the coverage-action branch April 2, 2023 22:24
@skirpichev skirpichev added this to the 1.4 milestone May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@cbm755 cbm755 cbm755 approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.
2 participants
@skirpichev @cbm755