Fix #722: aggregate_coverage.py does not aggregate

[cyrilleverrier]

Fix #722: aggregate_coverage.py does not aggregate the coverage reports of the same tracked lines

Accumulate the counters of the same tracked blocks of code from multiple coverage files

[elasticsearch-release]

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

[ruflin]

@cyrilleverrier Would you mind signing the CLA? https://www.elastic.co/contributor-agreement/

Looks like this PR makes @stevepeak life easier :-)

[ruflin]

@cyrilleverrier @stevepeak Could this break the partial coverage as the key is only based on the position? https://codecov.io/github/elastic/beats/filebeat/crawler/prospector.go?ref=be576ad137217d37798da766e8570b858c6d4a57#l-51 Not sure, just came to my mind.

[cyrilleverrier]

@ruflin : my company signed a "Corporate CLA" and added my name on it. It has been done today, just before I submitted the patches.

[cyrilleverrier]

@ruflin I don't know the internal of https://codecov.io but I guess that their server knows how to aggregate identical blocks of code in the same coverage file.

[andrewkroh]

I'm glad to see a fix for this. I had looked through various go tools to see if there was an existing solution to accumulate the counters but never found one. I wish that go tool cover just did this for us, but it doesn't.

And I also think that codecov.io is already handling this since their code coverage numbers appear correct. I had been relying on codecov instead of go tool cover reports due to this issue.

LGTM

[ruflin]

Yes, codecov is handling this. I know from @stevepeak that they did some magic here to also handle partial lines. My only worry about the script is if this would break it.

[andrewkroh]

There are some small differences between the reports on codecov report for master and 723 but it doesn't look broken. The differences could just be due to master having some changes that this PR branch does not have (I did not check the commits).

[ruflin]

Strangely the coverage files do not load for me for in #722 and get a 404. I assume it is an issue on the codecov side? I will restart the travis build.

[ruflin]

@cyrilleverrier Thanks for the CLA. I probably have to check it manually :-)

[ruflin]

@cyrilleverrier Thanks. CLA looks good.

[stevepeak]

Hey @ruflin is it to a late to jump in here? Go partial line coverage is fully implemented in our next major release. We are adding a "layers" to our coverage overlay, basically it will have the full line coverage and the partials together (as well as all the builds) in one UI.

[ruflin]

@stevepeak It's never too late :-) I'm curious if the following line would actually work "agains" the future provided by codecov, as it does already the summing up (per full line): https://github.com/elastic/beats/pull/723/files#diff-b4e51af4fe9718b862bd68526748ba44R42

[stevepeak]

@ruflin In terms of aggregating lines we do merge the lines and append the number of hits. The line mentioned may not be necessary.

Is this file used exclusively for Codecov? I'm not sure if its necessary because we handle all this merging server side.

Thanks!

[ruflin]

@stevepeak It is also use locally. My main concern was that because codecov already gets the aggregated lines, that we perhaps loose some data.

[stevepeak]

I see. Have you seen issues with data lose?

PS we archive all reports, the raw uploads. In our next UI you can download them any time (aka non-vendor lock-in)

[ruflin]

@stevepeak Didn't check yet. The only loss I could see is some partial line coverage details. So nothing big to worry about :-) I dragged you initially into the conversation as I thought perhaps you could spot a potential issue.