elastic / apm-agent-go

https://www.elastic.co/guide/en/apm/agent/go/current/index.html
Apache License 2.0
421 stars 194 forks source link

CI: Jenkins isn't uploading coverage for merge commits to codecov #384

Closed axw closed 5 years ago

axw commented 5 years ago

The builds keep showing misleading coverage changes. Looking at https://codecov.io/gh/elastic/apm-agent-go/branch/master/commits, the merge commits have lower coverage than the non-merge commits. The most obvious difference between them is that there are no coverage reports from Jenkins on the merge commits.

kuisathaverat commented 5 years ago

I am not sure why, we report the same info on PRs and on merges, but the measure seems about 3% different, I will keep an eye on it.

axw commented 5 years ago

The most obvious difference between them is that there are no coverage reports from Jenkins on the merge commits.

e.g. see the difference between these:

kuisathaverat commented 5 years ago

Jenkins PR build https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-go%2Fapm-agent-go-mbp/detail/PR-387/2/pipeline

Jenkins merge Master build https://apm-ci.elastic.co/blue/organizations/jenkins/apm-agent-go%2Fapm-agent-go-mbp/detail/master/25/pipeline/73

Jenkins report https://codecov.io/gh/elastic/apm-agent-go/commit/f61a7f13aa02b1cba4e823807d9d53cbb3a764c7/build

Travis Report https://codecov.io/gh/elastic/apm-agent-go/commit/c4a71d917a09679badfa025e9bb26d212f120d39/build

I saw on the Jenkins logs that Jenkins report to the same commit for the PR and the merge on master (https://codecov.io/gh/elastic/apm-agent-go/commit/f61a7f13aa02b1cba4e823807d9d53cbb3a764c7/build) I dunno why Travis change the report commit, the commit SHA1 is f61a7f13aa02b1cba4e823807d9d53cbb3a764c7 Tavis points to c4a71d917a09679badfa025e9bb26d212f120d39

https://github.com/elastic/apm-agent-go/commit/f61a7f13aa02b1cba4e823807d9d53cbb3a764c7

kuisathaverat commented 5 years ago

Mystery resolved, you make "Merge Pull request" that merge commits on the PR and create an additional merge commit this merge commit is ignored by Jenkins because does not have changes. "Merge and squash" do not generate this additional commit.

screenshot 2018-12-11 at 11 40 38
axw commented 5 years ago

In some cases I definitely want a merge commit, as I will sometimes have multiple logical changes within a PR. Most of the time it's just one commit and I could squash&merge, but not all the time.

kuisathaverat commented 5 years ago

it does not matter how you make the merge you will always have the report, the confusion that we have now it is because we have 2 systems reporting the same info in different ways. Now we know it, at some point I understand that only one will report that data, it would remove the confused data.

axw commented 5 years ago

Now we know it, at some point I understand that only one will report that data, it would remove the confused data.

Good point! I'll remove the Travis one later, after we figure out why there's a 3% difference.

kuisathaverat commented 5 years ago

The difference on the example I posted before is about 0.4% avr (83.93% vs 84.29%), I tracked down the differences to two files

Here the difference is some lines in the Travis report masked as yellow, I dunno why, I have checked we use the same commands for both

Jenkins 81.74% https://codecov.io/gh/elastic/apm-agent-go/src/f61a7f13aa02b1cba4e823807d9d53cbb3a764c7/model/marshal.go

Travis 86.57% https://codecov.io/gh/elastic/apm-agent-go/src/c4a71d917a09679badfa025e9bb26d212f120d39/model/marshal.go

In this last one, the difference on coverture are some "{" 😲

Jenkins 93.54% https://codecov.io/gh/elastic/apm-agent-go/src/f61a7f13aa02b1cba4e823807d9d53cbb3a764c7/module/apmhttp/client.go

Travis 94.11% https://codecov.io/gh/elastic/apm-agent-go/src/c4a71d917a09679badfa025e9bb26d212f120d39/module/apmhttp/client.go

kuisathaverat commented 5 years ago

I found why the comments are computed on the coverage report and how to ignore them https://docs.codecov.io/docs/fixing-reports

axw commented 5 years ago

The flip-flopping of coverage is driving me crazy, so I've looked into it a bit more. My current hypothesis is that it has to do with the Cobertura reports, which are being reported to codecov for Jenkins builds:

==> Reading reports
    + ./build/coverage/coverage-apm-agent-go-docker-report.xml bytes=11308417
    + ./build/coverage/coverage.cov.raw bytes=2768781
    + ./build/coverage/coverage.cov bytes=2768183

Whereas Travis-CI reports only coverage.txt:

==> Reading reports
    + ./coverage.txt bytes=2768725
kuisathaverat commented 5 years ago

It could be part of the issue, but also you have to keep in mind that we make a merge with the master before to build the PR, so if the master changes the coverture could change too. Another issue is codecove make some king of coverture report about the PR that could fail if does not have reference to compare, we saw it on agent .NET and finally, we disabled it https://github.com/elastic/apm-agent-dotnet/blob/master/codecov.yml#L18, also we forced to use the parent commit as reference always https://github.com/elastic/apm-agent-dotnet/blob/master/codecov.yml#L16

axw commented 5 years ago

It could be part of the issue, but also you have to keep in mind that we make a merge with the master before to build the PR, so if the master changes the coverture could change too.

99% of the time the PR is directly based on master anyway, so that's OK. I can deal with (real) differences like those, it's the spurious ones that were making the coverage reports basically meaningless.

Another issue is codecove make some king of coverture report about the PR that could fail if does not have reference to compare, we saw it on agent .NET and finally, we disabled it https://github.com/elastic/apm-agent-dotnet/blob/master/codecov.yml#L18, also we forced to use the parent commit as reference always https://github.com/elastic/apm-agent-dotnet/blob/master/codecov.yml#L16

Yeah I've seen that before too I think. I'll keep an eye out for it, may do the same for Go. I do look at the patch coverage, though.

axw commented 5 years ago

This is sorted now: https://github.com/elastic/apm-agent-go/pull/421