-
Notifications
You must be signed in to change notification settings - Fork 13
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
Coverage: Upload as separate step #199
base: main
Are you sure you want to change the base?
Conversation
The test-conda OSX failure is unrelated (#197) |
I guess it is quite impossible to test But with this patch, you will notice new jobs called |
This comment was marked as outdated.
This comment was marked as outdated.
Okay... autofix didn't do anything and I failed to see how my diff would trigger any such failures.
|
I see the advantage of this, but it does cause you to loose some fidelity in the codecov reports, as you only get one CI job and you loose all the information about what OS / job it was (I think?). Can you elaborate on the motivation for this a bit more, is it just codecov being flakey? |
Can you please elaborate on this? Each report would be uniquely named when uploaded as artifacts. They will all be downloaded by artifacts, and then re-uploaded to codecov all together. I don't see it being different from what workflow is already doing except the upload to codecov is a separate step.
@bsipocz asked for a solution to astropy/astropy#16379 |
Also it is painful to rerun test suite when only the upload fails. |
I think the difference (and it's not really a deal breaker) is that codecov looses the GHA metadata about which report was from which run? Anyway, I am happy to merge it, but from the astropy test PR I am unconvinced it actually works? |
Alternate solution is welcome. We have been using this over at https://github.com/spacetelescope/jdaviz/blob/main/.github/workflows/ci_workflows.yml and https://github.com/astropy/specutils/blob/main/.github/workflows/ci_workflows.yml but both workflows only have one coverage job each. |
@Cadair , what is this pre-commit silliness? |
TMP: Remove extra jobs
so it can be restarted without re-running the whole test suite because sometimes Codecov rate limits.
7aac2c2
to
4d7ffd8
Compare
TMP: Remove extra jobs.
I think it works as intended now. Please re-review, @Cadair . Thanks! |
What was causing the trouble? |
Initially, I didn't realize both artifact names actually contains the same |
@Cadair , are you convinced yet? Anything else I need to do here? Thanks! |
@Cadair ? @astrofrog ? @ConorMacBride ? 🙏 |
Upload to Codecov as separate step so it can be restarted without re-running the whole test suite because sometimes Codecov rate limits.
We need this to fix astropy/astropy#16379
This PR is tested downstream at astropy/astropy#16419
Also see spacetelescope/jdaviz#2865
You will see deprecation warning from actions/download-artifact#283 but it won't fail the upload.
cc @ConorMacBride @bsipocz