Add support for measuring unit test coverage difference in presubmits

[wojtek-t]

It would be great if could have a presubmit that will ensure that test coverage of packages/files touched by a given PR is not decreasing (or is at least X%).

A bit of context:

We already have postsubmits that show us the test coverage, e.g.:
https://testgrid.k8s.io/sig-testing-canaries#ci-kubernetes-coverage-unit
https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-testing/coverage.yaml#L127-L158

We're using the gopherage tooling to do that:
https://github.com/kubernetes/test-infra/tree/master/gopherage

It already allows to fail if coverage of a given file is lower than X.

It would be useful if we could add a functionality that would allow us to filter the report only to files/packages that are touches in a given PR.

Given that Profiles that are produced contain the filename:
https://github.com/kubernetes/test-infra/blob/master/gopherage/cmd/junit/junit.go#L66

it effectively boils down to filtering them to use only ones touched in the PR.

What I'm thinking about is:

• adding a flag to gopherage/junit that will take list of files/packages to which the report should be filters (default empty means no filtering)
• adding some functionality that will allow us to get list of files/packages changed in a given PR so that we can effectively use it to appropriate set the flag above

I'm not sure what's the best way to do the second though - so would appreciate some thoughts

@kubernetes/sig-testing @chaodaiG @BenTheElder - for thoughts

[BenTheElder]

Given that we always merge into the target branch locally before testing, and we have continuous unit testing ... strawman:

• persist the most recent coverage results for [merged code in] a given branch to a known location (GCS?)
• download current results in presubmit after unit testing
• compare across all files (no need to filter by file, since we're testing the "if we merged this PR" git state anyhow)

Something similar was implemented once for a Google internal project on a Prow.

[aojea]

+1
/cc

[mrbobbytables]

/cc

[chaodaiG]

this is a great idea! I'm in supporting.

• adding some functionality that will allow us to get list of files/packages changed in a given PR so that we can effectively use it to appropriate set the flag above

According to https://github.com/kubernetes/test-infra/blob/master/prow/jobs.md#job-environment-variables, PULL_BASE_SHA is the SHA of base branch, so in prow job you can use git diff $PULL_BASE_SHA --name-only to get the changed list of files.

I have a couple of questions:

• do we have active owners of gopherage who can do code review, bug fixes etc.?
• inspecting https://testgrid.k8s.io/sig-testing-canaries#ci-kubernetes-coverage-unit, the coverage of each file varies from 0 to >80 percent, do we want to make sure that each file has some coverage before enabling this?

[BenTheElder]

I don't think we need a list of changed files because either way we need a baseline to compare to and changing one file can change the coverage in an untouched file.

Consider the trivial case of modifying a unit test file only.

[stevekuznetsov]

Would e.g. removing a file or splitting it into separate files be something we expect the automation to pick up on and handle gracefully or would it be hard enough to automate / rare enough to require /override?

[chizhg]

Is there any specific reason for not using codecov? It's free for public GitHub repos and is very easy to setup. The Knative projects have been using it for a while and it worked pretty well. See the below links as an example:

• Dashboard: https://app.codecov.io/gh/knative/serving
• Coverage change on PRs: All Hops Encrypted: TLS between activator and queue-Proxy knative/serving#12815 (comment)

[wojtek-t]

Given that we always merge into the target branch locally before testing, and we have continuous unit testing ... strawman:

#25966 (comment)

I like that - it seems simpler to do - so hopefully also faster to act on.

To do that, IIUC we only need to:

• persist the info to GCS from the postsubmit jobs [sure there are races possible, but it's a good enough start]
• add a small comparison tool

Something similar was implemented once for a Google internal project on a Prow.

@BenTheElder - Do you have any pointers maybe? Would it be possible to upstream this potentially?

Would e.g. removing a file or splitting it into separate files be something we expect the automation to pick up on and handle gracefully or would it be hard enough to automate / rare enough to require /override?

I would prefer start simple and expand later. Even if we will be missing those cases initially, the other cases will be a huge gain already.

Is there any specific reason for not using codecov?

We do use golang test coverage underneath:

https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-testing/coverage.yaml#L150
It's all about interpreting the results.

Will anyone be able to help with this maybe?

/help wanted

[chizhg]

Is there any specific reason for not using codecov?

We do use golang test coverage underneath:

https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-testing/coverage.yaml#L150
It's all about interpreting the results.

I think my question/concern is whether it's worth the effort to reinvent the wheel, since codecov should have supported all the features you need.

To add a bit more context, Knative previously had an in-house coverage tool that was able to post unit test coverage diff on PRs, but we recently deprecated it in favor of codecov - knative/test-infra#3155

[chizhg]

And it looks some Kubernetes projects are already using it - https://app.codecov.io/gh/kubernetes

[wojtek-t]

I'm not owning our test-infra so I will let others to chime in, but while I generally agree with non reinventing the wheel, there seem to be advantages of having unifying the tooling (and we're using prow-based tooling for everything).

[BenTheElder]

@BenTheElder - Do you have any pointers maybe? Would it be possible to upstream this potentially?

I actually don't have it handy, I could dig it up but it's just some very naive bash capturing the overall coverage value and comparing IIRC, not really worth re-using, especially with so many lines of code.

The concept of using continuously uploaded post-merge results to compare is good though (and presumably how e.g. codecov works?).

I think my question/concern is whether it's worth the effort to reinvent the wheel, since codecov should have supported all the features you need.

I haven't looked at codecov in some time, our own existing coverage tooling is moreso about filtering out e.g. vendor, generated code, but it produces normal looking go coverage results so if codecov can accept uploading filtered results from a prowjob that could work.

We also probably want to continue making only prowjobs required in k/k, for reasons like /retest support.

It's also important that we control how go test is executed, as Kubernetes is particular about the flags, build tags, etc.

[BenTheElder]

It looks like it is possible to upload coverage files with codecov and that's the expected implementation https://about.codecov.io/blog/getting-started-with-code-coverage-for-golang/

So probably someone could update the unit test prowjobs to upload the final maniulated coverage files.

.. ButiIt doesn't appear that the uploader will set an exit code based on coverage dropping, so while it might gain us a visualization of modified files, it won't create the prow pass / fail we probably want ... (which comes with guarantees about being up to date to the latest code changes, something that is difficult to guarantee for externally posted statuses).

We already have a built in treemap => source file coverage visualization of the current coverage in prow, FWIW.

At minimum, I think someone will need to build a tool that produces a pass/fail exit code on coverage changes.

I don't think that requires a huge effort, we already have a command for diffing files https://github.com/kubernetes/test-infra/blob/master/gopherage/cmd/diff/diff.go

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[wojtek-t]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[wojtek-t]

/remove-lifecycle stale