Codecov isn't finding the correct base with "squash and merge"

[agoose77]

Version of Awkward Array

main

Description and code to reproduce

I've been noticing recently that Codecov estimates in changed coverage do not correspond to what's actually happening in the PR; namely, PRs that add no tests (e.g. those which add documentation or rename variables) still increase test coverage.

It turns out, Codecov uses the commit history to find the base with uploaded coverage metadata. Because we use "squash and merge", and don't run any Codecov jobs in main, Codecov always finds an old commit (e.g. e692946) to use as the base. Our build-test workflow has a schedule, so Codecov is only ever at-most 1 month out of date. In the below figure, you can see that each new branch never finds a codecov commit until the first commit which has the codecov tag

gitGraph
       commit tag: "codecov"
       commit
       branch feat-add-x
       checkout feat-add-x
       commit
       commit
       commit tag: "codecov"
       checkout main
       merge feat-add-x  tag: "no-codecov"
       branch feat-add-y
       checkout feat-add-y
       commit
       commit
       commit tag: "codecov"
       checkout main
       merge feat-add-y  tag: "no-codecov"

Loading

The simplest solution here is to run a special CI job upon the closure of a PR that uploads the codecov metadata: https://community.codecov.com/t/unable-to-determine-a-parent-commit-to-compare-against-in-base-branch-after-squash-and-merge/2480/20
However, my initial concern is that unless we also require PRs to be up-to-date before merging, the merge would happen after the CI had run Codecov, which could lead to out-of-sync coverage estimates.

A better solution might just be to run the build-test workflow for commits to main.

[jpivarski]

To be honest, I couldn't figure out what Codecov was doing, though I figured we'd only pay a lot of attention to it when we're working on maximizing coverage. I didn't think we needed it to run in every PR, I would have been happy to run it on demand.

Even without changing anything, Codecov is correctly describing the current absolute level of coverage, right? It's true deltas that are wrong, I expect. That's what we'd be most interested in when working on maximizing it.

We'd have to really know what we're doing if we get into a mode where we want every PR to have a non-negative coverage change. That's what this system seems to be designed for, and I can see that being a good idea in a very stable codebase—you don't want contributors adding features without testing them. For us, now, it can go up and down, as long as it is generally increasing.

If making the tests run against a putative merge to main would also fix the Codedov deltas, then that improves two things. We want our tests to tell us what will happen after merging, after all. That can make a CI test result be different from an offline one, and maybe for a reason not related to the PR itself, but this is information we want to know. Contra what I said above, we do know what we're doing enough to require that main should always be passing tests, no matter what. We're at least at that level of stability. So if switching the tests to test merge commits also makes the Codecov have the right base, then let's do it!

[agoose77]

Yes, right now it's only our deltas that are wrong. If we don't disable squash and merge (which, for all intents and purposes, I prefer as a default), then we need to patch the codecov metadata, or run coverage again. I think we can probably get away with modifying the metadata, provided that we enforce the following rule:

If we do that, then we'll know that the result of the squash-and-merge will by identical to the result of the merge commit (at least, I believe we have that certainty), and we can consider the coverage identical.

[jpivarski]

I've seen that option. It would be super-safe to require branches to be up-to-date before merging, but it would mean a concurrency traffic jam for small-fix PRs, and we tend to have a lot of those. If we have two PRs with auto-merge turned on, when one of them gets merged, the other will now need to update to satisfy the rule. If it doesn't auto-update (which I expect to be the case), then it needs to wait for a human to notice it and press the update button. If there's no notification for that, it might be waiting for a long time. Regardless, it might have been 90% done with its tests and now has to start over from scratch. I expect that a lot of the testing time would be wasted that way.¹

What I was saying above is that the tests would run on a putative merge-commit, rather than the actual last commit of the branch. (Azure had an option for that, and we used it.) If main updates after that test, then the test would have been run on an out-of-date merge-commit, and so merging it would introduce the possibility that main will be made to fail, but the same is true of testing entirely within-branch, so it's not worse than the alternative. Requiring that tests always run against an absolutely up-to-date merge-commit (exactly the one that will be the result of the merge) would be safer, but it essentially serializes testing. Worse, it serializes the tests that would be accepted, while parallel tests still happen, uselessly.

So I was willing to accept that a CI test is testing something different from what will happen when the PR gets merged (as we have now); I was just saying that it's a little better to test out-of-date merge-commits than non-merge-commits. And if that allows Codecov to be correct, then even better; we can adopt that model.

Footnotes

1. Incidentally, the all-contributors bot commits have to be absolutely serialized this way. One has to be fully merged before another can be generated; otherwise, git diff interleaves lines of JSON in a terrible way. The diff algorithm could stand to be made aware of the syntax of some languages, at least declarative ones like JSON and YAML. ↩