-
Notifications
You must be signed in to change notification settings - Fork 178
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 causing merge of passing commits to fail #1153
Comments
The issue here seems to be that coveralls measures a coverage decrease, and thus fails. There should be a threshold, as we can all agree reasonably that a -0.003% decrease should not cause a CI failure. I'll set a failure threshold up now if I have access to the settings, and from there we can monitor its future behaviour. |
UPDATE: I do not have access to the Coveralls settings for this repository. Can you either add me or set a failure threshold, @suchow? |
@Nytelife26 I've changed it to 1.0%. It looks like this variable can be set in a However, I don't see how this solves the issue. The problem is that whatever coverage Coveralls is computing on the last commit of a PR differs from what it computes on the commit that merges that PR, which doesn't make any sense to me. The branch |
Unless it was computing the same minor decrease in coverage for both, but fails the CI run only when it's on branch master? |
It's the discrepancy that is the bigger issue than the failure. Pushing us to add more tests is good, but being inconsistent and failing the CI on master with no fair warning is bad. |
I believe this is a problem with how we're using Coveralls. It only builds on the master branch - PRs only upload their coverage, but the action doesn't run on them it seems. I'll investigate further into using |
See in particular merge commit 9a94cbe, which fails the Coveralls check despite the fact that the final commit (0745774) of the PR that it is merging (#1152) passes. I'm not sure whether the desired behavior is both failing or both passing, but they should rise and fall together.
The text was updated successfully, but these errors were encountered: