search

istio / old_pilot_repo

Deprecated home of Istio's Pilot, now in istio/istio's pilot dir
Apache License 2.0
137 stars 91 forks source link

Improve code coverage to ~90% #1233

Closed rshriram closed 6 years ago

rshriram commented 7 years ago

Our code coverage is at 70%. Need unit tests to increase code coverage to 95%. Lets track all PRs related to this issue here.

rshriram commented 7 years ago

PTAL at this for details: https://codecov.io/gh/istio/pilot (ignore the test folder coverage)

kyessenov commented 7 years ago

Pilot functionality is difficult to test without real envoy and real kubernetes unfortunately. Let's first devise a plan to meaningfully increase coverage before writing a mock galore. I'd start by collecting coverage metrics from e2e tests.

gyliu513 commented 7 years ago

One question for the code coverage, every time I run make coverage, I can get a report as following:

root@pc002:~/go/src/istio.io/pilot# make coverage
ok      istio.io/pilot/adapter/config/aggregate 0.209s  coverage: 92.6% of statements
ok      istio.io/pilot/adapter/config/crd   12.612s coverage: 65.6% of statements
ok      istio.io/pilot/adapter/config/ingress   0.520s  coverage: 71.8% of statements
ok      istio.io/pilot/adapter/config/memory    0.209s  coverage: 93.6% of statements
ok      istio.io/pilot/adapter/serviceregistry/aggregate    0.067s  coverage: 58.5% of statements
...

It will also generate a coverage.txt as following:

root@pc002:~/go/src/istio.io/pilot# cat coverage.txt

mode: set
istio.io/pilot/adapter/config/aggregate/config.go:31.66,34.31 3 1
istio.io/pilot/adapter/config/aggregate/config.go:40.2,40.41 1 1
istio.io/pilot/adapter/config/aggregate/config.go:43.2,46.8 1 1
istio.io/pilot/adapter/config/aggregate/config.go:34.31,35.55 1 1
istio.io/pilot/adapter/config/aggregate/config.go:35.55,38.4 2 1
istio.io/pilot/adapter/config/aggregate/config.go:40.41,42.3 1 1
istio.io/pilot/adapter/config/aggregate/config.go:51.81,53.31 2 1
istio.io/pilot/adapter/config/aggregate/config.go:56.2,57.16 2 1
istio.io/pilot/adapter/config/aggregate/config.go:60.2,63.8 1 1
istio.io/pilot/adapter/config/aggregate/config.go:53.31,55.3 1 1
istio.io/pilot/adapter/config/aggregate/config.go:57.16,59.3 1 0
...

But it does not like what we run for kubernetes, with kubernetes, I can get a HTML file which will report all of the code coverage, and I can also see the line of code which is covered by the test, but in pilot here, I do not know how to check which line of code is not covered? Any comments for how to analysis the coverage report? Thanks.

kyessenov commented 7 years ago

If you have a Go IDE, you should be able to see coverage directly in the editor. Atom can do it, or vim-go.

gyliu513 commented 7 years ago

I was using eclipse as the Go IDE, can you please show me a screenshot here?

Do you mean that after I run make coverage, once the coverage.txt is generated, I will be able to check the code coverage from Go IDE?

It would be great if you can show me some concrete steps for how to check the coverage with some Go IDE. Thanks @kyessenov

kyessenov commented 7 years ago

Sure, here how it looks after I run go coverage in vim-go: Screen Shot 2017-09-11 at 7.33.42 PM.PDF

kyessenov commented 7 years ago

95% is going to be tough to achieve...

@ldemailly : please add tests for https://github.com/istio/pilot/blob/master/platform/kube/register.go

@GregHanson : please add tests for https://github.com/istio/pilot/tree/master/platform/consul

I'll try to improve configuration backends coverage.

ldemailly commented 7 years ago

@kyessenov register has unit tests for all the functions that have no side effect/don't require kube but not for the main one. it would a lot easier to test the functionality through istioctl cmd line, I asked during the pr where/how can I put istioctl tests and if I recall the answer was "there aren't any" (in pilot)

do you have a pointer to a similar test (I guess kube-inject tests ?)

kyessenov commented 7 years ago

@ldemailly you should test the libraries, not CLI tools, if possible.

Example using real k8s in a test: https://github.com/istio/pilot/blob/e6217330e54402305b60d93577796fdd76baa42d/adapter/config/crd/client_test.go#L65 Example using fake k8s API: https://github.com/istio/pilot/blob/e6217330e54402305b60d93577796fdd76baa42d/platform/kube/admit/admit_test.go#L419

ldemailly commented 7 years ago

Thanks for the pointer !

In general, my 2 cents by the way would be to focus on stability (we're in code freeze), finding and fixing actual bugs, starting with the known ones, and increase real coverage rather than making big refactoring just for the sake of increasing a number - that number can be increased during the dev cycle next time (vs post code freeze).

(not to say that istioctl register shouldn't have a test, it should)

kyessenov commented 7 years ago

Once we raise it to ~90%, we're not going to chase the number as it provides marginal value. It's important to get basic coverage since it prevents accidental regressions. We never pay enough attention to this during our regular dev cycle, so now is a good time to add coverage test suites.

kyessenov commented 6 years ago

Issue moved to istio/istio #1455 via ZenHub