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

CI: Ensure code coverage is always uploaded #28164

Merged
merged 1 commit into from May 9, 2024

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented May 3, 2024

PR summary

If code coverage is not uploaded on failure, then overall coverage can suffer mysteriously (e.g., if a mac system failed, then it would appear as if backend_macosx.py was not tested). Codecov would then show several "indirect changes" that are spurious.

On GitHub Actions, if you don't have any status check function in the if entry, then it is treated as success() && (whatever else you had), so put in an explicit check.

Azure also had no condition, so defaulted to only-on-success.

Also, fix the image-cleanup script, which would only run on success instead of failure, where it would be useful.

PR checklist

@QuLogic QuLogic added this to the v3.9.0 milestone May 3, 2024
@github-actions github-actions bot added the CI: Run cygwin Run cygwin tests on a PR label May 3, 2024
@QuLogic QuLogic added CI: Run cygwin Run cygwin tests on a PR and removed CI: Run cygwin Run cygwin tests on a PR labels May 3, 2024
@QuLogic
Copy link
Member Author

QuLogic commented May 3, 2024

OK, this looks good; coverage is being uploaded even on failure on all the runs. I'm not sure if we should upload Cygwin coverage, since it's not done on all PRs too. Maybe there's a way to keep that separate.

Also, the script to remove passing images is now working, though I see it doesn't really clean up as much as it can.

One other thing is that it appears that Azure only uploads on Linux; it is using the Bash uploader, which is supposed to be broken, but still works there. However, it stopped working on other platforms. I think we would need to set up a token in order to get the new uploader working.

@ksunden
Copy link
Member

ksunden commented May 7, 2024

I think preventing cygwin from uploading is probably fine. If having it would cause main to have more coverage than PRs, that would make almost all runs fail coverage check I think.

Azure may be worth either fixing (could be here or in a followup) or removing coverage entirely (if its not getting us additional coverage/hasn't been working anyway).

I would leave any adjustments to the upload cleaner for a separate PR.

If code coverage is not uploaded on failure, then overall coverage can
suffer mysteriously (e.g., if a mac system failed, then it would appear
as if `backend_macosx.py` was not tested). Codecov would then show
several "indirect changes" that are spurious.

On GitHub Actions, if you don't have any status check function in the
`if` entry, then it is treated as `success() && (whatever else you
had)`, so put in an explicit check.

Azure also had no condition, so defaulted to only-on-success.

Cygwin is only run on `main` and sometimes on PRs, so disable code
coverage reporting from it. That would cause random "reductions" in
coverage for all PRs that don't run it (which is most of them.)

Also, fix the image-cleanup script, which would only run on success
instead of failure, where it would be useful.
@QuLogic QuLogic force-pushed the codecov-upload branch 2 times, most recently from 45d5dc0 to 25847dd Compare May 8, 2024 20:22
@QuLogic
Copy link
Member Author

QuLogic commented May 8, 2024

I removed Cygwin, as it would just cause extra flapping, since only main runs it and PRs only run it conditionally. I did leave the coverage reporting to the terminal just in case someone wanted to browse it.

@tacaswell
Copy link
Member

Agree about both not uploading from cygwin and defering fixing uploading on non-linux azure to a follow on PR.

@ksunden ksunden merged commit 3ee509d into matplotlib:main May 9, 2024
43 of 45 checks passed
Copy link

lumberbot-app bot commented May 9, 2024

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout v3.9.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 3ee509d14f5a624f1d99c3a1727810e9c4989258
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #28164: CI: Ensure code coverage is always uploaded'
  1. Push to a named branch:
git push YOURFORK v3.9.x:auto-backport-of-pr-28164-on-v3.9.x
  1. Create a PR against branch v3.9.x, I would have named this PR:

"Backport PR #28164 on branch v3.9.x (CI: Ensure code coverage is always uploaded)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

@QuLogic QuLogic deleted the codecov-upload branch May 9, 2024 18:32
QuLogic pushed a commit to QuLogic/matplotlib that referenced this pull request May 9, 2024
ksunden added a commit that referenced this pull request May 10, 2024
…3.9.x

Backport PR #28164 on branch v3.9.x (CI: Ensure code coverage is always uploaded)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Run cygwin Run cygwin tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants