-
Notifications
You must be signed in to change notification settings - Fork 8
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
Coveralls seems to invalidate other GitHub actions statuses #1739
Comments
Hi, @AndrewLister-STFC. Sorry for the delay. This is very interesting, and some what alarming. I have not seen a case like this before and I am not aware of how one status update could affect others, unless they represent dependencies defined elsewhere, such as in CI. Can you help me understand if there is any such dependency between your Coveralls upload step and the completion of those other steps? It feels like received, completed checks would not change state unless they were...sent again? Do you have any working theories? I have asked around here, but no one is sure just yet. Perhaps we could run another test? I could try re-running the build for you to see if that has any effect on the pending statuses? |
No worries @afinetooth, thanks for the help! The job in the PR that submits the result is below. Nothing uses the You're welcome to re-run the tests or push some blank commits to trigger them (I can add someone as a dev temporarily) if it would help. The only thing I can think of trying is some combination of
|
@AndrewLister-STFC I found a clue (or two). I hope it helps: Clue 1: Parallel build: Coverage Reports uploaded: If, indeed, your build is parallel, then you would not only want to indicate that in GitHub Actions Workflow like the parallel usage example here:
You would also want to close the parallel build with the final
Clue 2: According to your coverage reports, it's because your script did that twice, in two attempts: This is partly on us because, while we don't currently, we should probably delete any identical reports when the only difference is a higher But, do you have any idea why your workflow would send two version of the same report, if the first one succeeded? |
Thanks for digging into this, that would make sense for why it would invalidate the other results. I think this might be a red herring though. The dates at the bottom are almost 2 weeks apart, it could be from re-running the tests to see if the behaviour happened again. As soon as the coveralls results are in the other statuses are set to pending so I don't know how this would affect it. |
Watching this happen more closely, the results from coveralls come in exactly when all of the other tests complete (and the results from the other ones disappear). Could it be somehow defining a new set of tests rather than reporting on the running one? |
Sorry for the multiple messages. Looking at the statuses with the github REST API, there are statuses on both the commit which triggered the workflow and the temporary merge commit created to run the tests on. On the commit there are also status updates from rtd, where the merge commit only has a coveralls status. Is coveralls meant to put statuses on both of these? Adding the commit ref explicitly seems to have fixed it:
|
I'm trying to update our use of coveralls to use the new Universal Reporter via the official GitHub Action (our coveralls has been failing to report the status for some time and I was hoping this would fix it).
Coveralls now successfully reports coverage but when the status comes through it marks all other checks as "expected" even though they've already run and had the correct status before the coveralls result changed.
Here's a screenshot with the status on the commit expanded to show all of the tests have run and the summary box showing only coveralls results
and a link to the PR: fitbenchmarking/fitbenchmarking#1206
I'm not sure if this is an issue with how I'm using coveralls, GitHub, or the use of the GitHub API within coveralls. Hopefully I'm just missing something and it's a straight forward fix.
Has anyone seen this before? Is there a known workaround?
The text was updated successfully, but these errors were encountered: