-
Notifications
You must be signed in to change notification settings - Fork 14
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
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 ? 🙏 |
Thanks! |
Hmm, astropy is picking this up but I don't understand why codecov complains about no token. We have one in repo secrets. https://github.com/astropy/astropy/actions/runs/9355922312/job/25755726624 |
I think that's a bug with this. The sunpy builds in particular seem to be attempting to upload the same coverage artifacts more than once: https://github.com/sunpy/sunpy/actions/runs/9352703510/job/25742354670#step:3:6 As you can see in that log it's finding jobs from a different invocation of the workflow (the "test" one) and then downloading them in the "docs" workflow. Do you think you will have time to try and fix it @pllim ? |
Ah maybe relatedly, that "docs" job on sunpy's CI has |
Yes this is breaking a lot of CI, a lot of my glue builds as well as reproject are failing. I think the artifact name needs to be more unique as the issue seems to happen for me if multiple OSes test e.g. |
Not obvious to me how to grab the job ID without doing some Actions YAML output magicks. Is there an easier way? |
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