8000 TST, CI: turn on codecov patch diffs by tylerjereddy · Pull Request #12253 · scipy/scipy · GitHub
[go: up one dir, main page]

Skip to content

TST, CI: turn on codecov patch diffs #12253

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

Merged
merged 2 commits into from
Nov 11, 2020

Conversation

tylerjereddy
Copy link
Contributor

< 8000 /div>
@tylerjereddy tylerjereddy added the CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure label May 28, 2020
@tylerjereddy tylerjereddy added this to the 1.6.0 milestone May 31, 2020
* now that we have an open channel of communication
with the codecov team and GitHub app has been activated
(scipygh-12240), try turning on pull request patch diffs again,
similar to NumPy PR 16409
Co-authored-by: Thomas Hu <thomasrockhu@users.noreply.github.com>
@thomasrockhu
Copy link
Contributor

One note here, we've been seeing some issues with users using actions/checkout@v2 via actions/checkout#237.

@tylerjereddy
Copy link
Contributor Author

Ah, very cool, before applying your fix I did:

curl -X POST --data-binary @codecov.yml https://codecov.io/validate

and I can indeed reproduce locally:

could not determine a constructor for the tag '!appveyor'
  in "<byte string>", line 4, column 7:
        - !appveyor

tylerjereddy added a commit to tylerjereddy/numpy that referenced this pull request Jun 4, 2020
* use the codecov `yaml` validation server in our CI
to check for issues in our codecov config; based on discussion
in scipy/scipy#12253

* I think the exit code is still `0` when invalid, but at least
the output would provide some traction if coverage seems to be
misbehaving

* we could also try to parse the output and set exit code based
on validity status; maybe overkill for now though?
@tylerjereddy
Copy link
Contributor Author

The patch entry is showing up now. The new problem is that the patch diff is clearly being calculated on changes for files that are not changed in this PR, hmm.

@tylerjereddy
Copy link
Contributor Author

Those files were changed recently in other PRs though, that's probably a clue. I think I also saw that @peterbell10 may change how our Travis build caching works--not sure if that's related or not.

@peterbell10
Copy link
Member

It's showing the diff since 3393164. I'm guessing this is because coverage results aren't uploaded for master so it doesn't have results for the true base commit.

@tylerjereddy
Copy link
Contributor Author

Hmm, so I'd have to convince the team to re-enable running the full test suite for one job in a CI matrix somewhere for merge commits basically.

@thomasrockhu
Copy link
Contributor
thomasrockhu commented Jun 4, 2020

@tylerjereddy would carryforward flags be helpful here (aka use flags so you don't have to upload all coverage reports on every commit)?

@tylerjereddy
Copy link
Contributor Author

@thomasrockhu @larsoner

It looks like we do still use GitHub Actions to run CI after a push to master: https://github.com/scipy/scipy/blob/master/.github/workflows/testing.yml#L6

If we were to turn on codecov reports for the MacOS jobs on GitHub Actions, would that provide the necessary "base" for producing useful patch diffs?

Are patch diffs really only useful for the same job "type?" So, if we only push codecov reports for MacOS CI, would the patch diff contribution for Windows 32 bit on Azure CI play nicely?

@larsoner
Copy link
Member

Are patch diffs really only useful for the same job "type?" So, if we only push codecov reports for MacOS CI, would the patch diff contribution for Windows 32 bit on Azure CI play nicely?

I don't know. I say let's just try it and see if it yields something useful, and disable it if it doesn't. Another option would be to add one more entry to the GitHub actions that does the most complete testing possible (TESTMODE=full) with the fastest build (probably Linux?). That is assuming that the GitHub actions is usually not (and wouldn't become) our bottleneck, which I think is probably true.

@andyfaff
Copy link
Contributor

w.r.t github actions, there can be more than one workflow YAML file in .github/workflows. scipy already has two, one for macOS (testing.yml) and one for Linux + Python 3.9 (bleedingedge.yml).
I think that the testing.yml file could be renamed to macOS.yml, bleedingedge.yml to nightly.yml.
The testing you're describing here might be best done in a linux.yml file. That could simply be a copy of the bleedingedge.yml file with minor modifications.

@larsoner larsoner closed this Nov 9, 2020
@larsoner larsoner reopened this Nov 9, 2020
Copy link
Member
@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM. I say we merge and try it. It's easy enough to add patch: false or target: 1 in the future if this turns out not to be useful. But I've used a similar config in other repos and so far it has been helpful.

@tylerjereddy
Copy link
Contributor Author

LGTM. I say we merge and try it.

Alright, I'll put it in then. If patch coverage diffs do start working properly it can indeed be useful for the reviewer of PRs.

@tylerjereddy tylerjereddy merged commit 832e91d into scipy:master Nov 11, 2020
@tylerjereddy tylerjereddy deleted the codecov_patch_diff branch November 11, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0