lemurheavy / coveralls-public

The public issue tracker for coveralls.io
http://coveralls.io
124 stars 7 forks source link

PR statuses stay "pending" if a new commit is added #1733

Closed dgw closed 8 months ago

dgw commented 8 months ago

We are getting a bit frustrated by what seems to be a relatively recent change in Coveralls' behavior regarding PR check statuses. To make code review easier, the best approach to amending a PR is often adding a new commit on top; this facilitates use of GitHub's "show changes since last review" feature.

True, these changes must sometimes be integrated back into the PR history via a rebase-and-squash operation. But sometimes the new changes are a logical follow-up that doesn't need to be squashed, making a rebase unnecessary—except that Coveralls no longer evaluates the coverage for a PR branch in this situation:

image

Coveralls demands that the branch be "rebased" to resolve these pending checks, even if there is no other reason to rewrite history:

'Target branch is ahead of PR base. Rebase the PR to fix.'

This leaves "pending" commits in the commit list and also does not let the PR "turn green" when CI tests pass, which makes it harder to evaluate whether the changes are "good other than coverage metrics" at-a-glance.

Is this intended behavior? Is it perhaps something we can change our Coveralls configuration to disable?

afinetooth commented 8 months ago

Hi @dgw just a very brief response here right now to say that we are deploying a reversion to these recent changes today. It's the type of deployment that requires close attention after, so we may roll back or make additional changes throughout the day.

I will circle back when we are fully deployed and considered stable.

In the meantime, to the extent it helps explain the recent changes, I'll post a link to a more detailed comment about them here.

The important thing to know is that this feature was "over-implemented"---by which, I mean, the behavior of its terminal state was implemented, instead of its first transitional state, which represented the first step toward a 1-year transition, which only started introducing messaging and recommendations. That is what today's deployment is reverting.

Thanks for your patience.

dgw commented 8 months ago

Thanks for the update, @afinetooth! I'll look over the whole explanation of what's coming when I have a longer moment to sit and read it, but what I've skimmed so far makes sense. We'll stay tuned, grab a round of $preferred_beverages, and get ready to act on the new state once y'all finish deploying. :)

afinetooth commented 8 months ago

@dgw The above-mentioned rollback of our feature-in-transition to its initial state was deployed yesterday.

As a result, pull_request builds which are out-of-sync with their target branches will continue being processed, but will receive messaging, in-page and in PR Comments and Status Updates, indicating there's a chance your coverage report for the current state of the PR may be inaccurate for that reason and providing instructions on how to resolve the issue temporarily and permanently.

To clarify, though, PR Comments and Status Updates will be sent for those builds and not help, so, as in your case, you should no longer have to wait for any Status Updates in Pending state.

Here's an example of the in-page message for "out-of-sync" PR builds:

Screenshot 2023-11-03 at 11 43 32 AM

(*Please note that the documentation under that link has not been deployed yet. It should be up by Mon.)

Let me know if you have any questions.

dgw commented 8 months ago

@afinetooth Wonderful news! I'll check back sometime next week, after the linked documentation is rolled out, to start evaluating how we can handle this in our GitHub Actions CI workflow.

We should probably see about upgrading to the Coveralls action from Marketplace at the same time, since at present we're just installing the coveralls package from PyPI and running coveralls --service=github as the last step of the CI build workflow (followed by coveralls --finish in a final job that runs after the parallelized tests are done).

afinetooth commented 8 months ago

Hi @dgw. Wanted to let you know that that linked documentation has deployed---though, the link itself has changed. It is now: https://docs.coveralls.io/build-types#recommended-ci-configurations

We should probably see about upgrading to the Coveralls action from Marketplace at the same time, since at present we're just installing the coveralls package from PyPI and running coveralls --service=github as the last step of the CI build workflow (followed by coveralls --finish in a final job that runs after the parallelized tests are done).

I think that makes sense, just for the fact that with our official integrations we can support your entire toolchain.

You won't lose much by testing it in a PR, starting with the default setup. Here's the Parallel Build usage example from the README.

Note the two distinct steps there:

The GitHub Action (really, the Universal Coverage Reporter under-the-hood) should be able to find your coverage report, determine its format and process it automatically. If you encounter an issue, then you'll want to start laying in some specific input options to be more declarative about where your report exists in CI, what its format is, etc. The most commonly required options:

dgw commented 8 months ago

Ah, it's probably time to close this out. The immediate problem seems to be resolved, and we'll keep an eye on our PR builds (plus your wonderful summary of how to switch to the Action, @afinetooth) to guide us as we make adjustments, probably after our own next release. (It's tantalizingly close, almost 2½ years since we started on this next major version!)

Thanks for all the detailed responses, and for keeping Coveralls available for us FOSS people to use!

afinetooth commented 8 months ago

Our pleasure, @dgw. Good luck with your upcoming release! 🎉