`istio-cni` code coverage

bleggett commented 4 months ago

(This is used to request new product features, please visit https://github.com/istio/istio/discussions for questions on using Istio)

Describe the feature request We don't have a good ongoing sense of what chunks have poor unit test coverage, and we don't have a good ongoing sense of where adding more cheap unit tests would bring the most value, and we generally want to encourage more, cheaper, easier to debug/maintain unit tests, and fewer, expensive, harder to debug/maintain integ/e2e tests.

We should generate code coverage stats (per branch/per func) as part of PR CI, and compare it with the current high-watermark in mainline, and warn/fail the CI check if coverage drops below the current high-watermark.

(usual caveat - code coverage is not the only proof of good code coverage or useful tests, but it is the easiest to automate, and hard to casually cheat)

We could do this for other components too , but istio-cni seems like a decent place to start as it is both relatively critical, relatively complex, and relatively self-contained.

Unofficial requirements (subject to change)

Must work locally and in CI without extra configuration
Must be something that can be invoked as a simple makefile target locally and in CI
May not rely on external services being available
Should dump coverage report as a CI artifact

When developing the inpod CNI internally we used overcover which is a very basic/simple thing that just parses go test coverage reports. It worked well enough and doesn't require any magic. Can always dump the coverage report as an artifact if people wanna see it, it doesn't need to be fancy.

overcover --coverprofile cover.out ./... --threshold $(COVERAGE_THRESH_PCT)

tl;dr: I really don't want to use anything that can't work locally AND as a simple makefile target.

EDIT: After 5/15 WG, following are the requirements:

Start with one component at a time (istio-cni in this case)
Simple, language-specific tooling only, must run locally and in CI the same way (makefile target, no out of band setup)
CI should publish coverage report artifacts.
An overall number for all the packages in the component should be generated (go test needs help to do this IIRC or it will only report per-go-pkg coverages, which is not as useful - something like overcover can help with that)
Stretch goal -> figure out how to get integration tests to count towards coverage. Istio currently has an (IMO excessively) topheavy inverted test pyramid, so being able to include integ tests in coverage would be nice too).

JayKaku commented 4 months ago

Hey @bleggett, I work with Istio frequently and was looking to pick this up as a way to start contributing, is there any specific section of cni that should be considered first? I'd like to work on it!

bleggett commented 4 months ago

ztunnel side of the same issue: https://github.com/istio/ztunnel/issues/1049

@JayKaku thanks for volunteering - could you hold off a bit? We are still discussing how we want to approach this.

JayKaku commented 4 months ago

Sure! lemme take a look at ztunnel unit test and do lemme know where can I watch the discussions for this! Thanks!

bleggett commented 4 months ago

5/15 WG consensus was that we do want to add codecoverage, but not CI gates for it.

@JayKaku I updated the issue with what we're looking for - if you want to take a stab a this, feel free.

What we want here is to report overall coverage for all of istio-cni, versus just Go package-level reports.

bleggett commented 4 months ago

FWIW branch code coverage can help us identify tests we don't need as well: https://github.com/istio/istio/pull/51192#pullrequestreview-2077958297

Rana-KV commented 3 months ago

Hi @bleggett , I'm interested to work on this issue. I have background in security and SAST tools.

bleggett commented 3 months ago

@Rana-KV can you coordinate with @JayKaku ?

Rana-KV commented 3 months ago

Sure, I will reach out to @JayKaku first.

spacewander commented 2 months ago

@bleggett Excuse me for an off-topic question: is there any place we can find the code coverage number of Istio nowadays?

bleggett commented 2 months ago

@bleggett Excuse me for an off-topic question: is there any place we can find the code coverage number of Istio nowadays?

No. The goal with this issue here is to begin generating code coverage, at least for informational purposes, starting with the istio-cni component.

Stevenjin8 commented 1 month ago

@Rana-KV and @JayKaku, are either of you still interested/working on this issue?

istio / istio

`istio-cni` code coverage #50991