change to merge handling has broken handling of real merges and PR diff calculation

[njsmith]

One of codecov's key features is that for each PR, it tells you how it changes coverage compared to the current master branch. In order to do this, it needs to have some idea of what the coverage is on the master branch. Sometime in the last week or so, on my project Trio, codecov started miscalculating the master coverage as being ~95%, when my coverage is actually ~99%. Since codecov is still calculating coverage correctly for PRs, this means that it's now reporting that every PR is amazing and causing a big jump in coverage. For example, here's a PR that doesn't touch the code at all, but codecov reports is adding hundreds of lines of new coverage: python-trio/trio#388
Of course this makes the codecov reporting rather useless, since it's impossible to see actual PR-related coverage changes in the noise.

If you look at codecov's commit history for my master branch, then you can see that it's all of the "Merge pull request #XX from XX" commits that have the incorrect coverage.

It looks like the reason this is happening is that this project is tested on Appveyor (Windows), Travis (Linux), and Jenkins (MacOS). The commits with correct coverage show uploads from all three sources (example). The commits with incorrect coverage show uploads only from Appveyor (example), and the 95% is what you get if just running the tests on Windows.

So that all makes sense, but why are my merge commits apparently only getting tested on Windows? Well, they are getting tested on Linux and MacOS... here's a Travis log testing that same 4f65fa74 commit that I linked above. And if you scroll down to the bottom of the Travis log, you can see that coverage is being uploaded to codecov..... but the codecov-bash uploaded says Fixing merge commit SHA and then from the URL at the bottom we can see that even though we were testing 4f65fa74, the coverage has been attributed to commit 87d2c9ca1e3 (which is one of the merge parents).

And then the reason I'm getting different behavior on Windows vs. Linux and MacOS is that on Windows, there is no bash so I'm using codecov-python, while on Linux and MacOS I'm using codecov-bash.

Looking over the recent commit history, the problem seems to be this recent commit: beaae435 (which is also in codecov-python, but I'm guessing that part hasn't been shipped yet). The idea of that commit is to replace a hacky-looking regex check for merges, with a more reliable check based on git parentage.

However, this is based on a fundamental misunderstanding of what that code is doing. It's purpose is not to detect merges in general; its purpose is specifically to detect the temporary refs/pull/XX/merge merge commits that Github generates for testing PRs, which have a special auto-generated commit message. The special handling is only supposed to be triggered on these merges, not for all merges, and in particular not for merge commits that are actually pushed to master, as in my case. So I believe that this change should be reverted from both codecov-bash and codecov-python.

CC: @stevepeak @blueyed

[blueyed]

Thanks for the detailed issue report.

I agree with your conclusion that there is a fundamental misunderstanding.

However, instead of reverting we should rather harden the detection to actually only trigger it for GitHub merge commits, e.g. by looking at the merge commit's author.
What do you think?

[njsmith]

What problem are you trying to solve, and what's your plan for testing a new change to make sure it's at least as good as the old thing?

…

On Dec 28, 2017 5:47 AM, "Daniel Hahler" ***@***.***> wrote: Thanks for the detailed issue report. I agree with your conclusion that there is a fundamental misunderstanding. However, instead of reverting we should rather harden the detection to actually only trigger it for GitHub merge commits, e.g. by looking at the merge commit's author. What do you think? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#106 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlOaFzLGgGQq9sZV1QrJ68hcUBKdBDDks5tE5wBgaJpZM4RN8e9> .

[blueyed]

Initially I've thought that looking at Git metadata is less fragile than parsing the log message.
With only reverting to the original behavior, it would still be broken in case of non-PRs, when using the same commit message.

codecov/codecov-python#128 is meant to restore the original bahvior, with the additional fix on top.

I've only done manual testing for it, and it should be reviewed carefully of course.

[stevepeak]

Thank you @blueyed I'll revert the change and deploy in the next 30 minutes.