search

kubewarden / kubewarden-controller

Manage admission policies in your Kubernetes cluster with ease
https://kubewarden.io
Apache License 2.0
189 stars 33 forks source link

Reach close to 70% test coverage for the `internal` package #665

Open jvanz opened 7 months ago

jvanz commented 7 months ago

We have a goal to reach 70% of code coverage in the kubewarden controller repository. For that, we can improve the coverage of the internal package. The first package that must need to be improved is the internal/pkg/admission. This is a pretty important package and it coverage is only 37.61%. To improve the test coverage, unit tests should be enough. The code in this package need only a Kubernetes client. Witch can be easily mocked.

The package internal/pkg/policyserver is pretty small and it's missing test only for a if statement. Therefore, I think we can make a final effort and cover it as well using unit tests.

As mentioned in the #661, many packages inside internal/pkg are missing in the report. Even if they are not so important as the admission one, I think we can, while or after the fix for this issue, improve the naming and metrics coverage adding very simple unit tests. The effort does not look so big and we improve our test coverage in the end. For the metrics one we need to find a way to mock the OTEL calls.

Another package missing in the report is the /internal/pkg/admissionregistration. Witch has a misleading name. This is the package used by the controller to generate the CA and certificates. I think we should refactor this package renaming it and maybe bring the changes from the feat-ca-changes branch into main. The code in the branch already have tests and name make more sense. Furthermore, we advance a little bit in the path of removing cert-manager (but this is not the main reason for this change in this card). As this is considerable change, I've open another issue to cover this.

This is a spin off of https://github.com/kubewarden/kubewarden-controller/issues/648

Acceptance criteria

flavio commented 7 months ago

I'm fine with that, let's keep the changes to /internal/pkg/admissionregistration for later, since they require more work (at least that's my impression)

jvanz commented 7 months ago

I'm fine with that, let's keep the changes to /internal/pkg/admissionregistration for later, since they require more work (at least that's my impression)

Just to make things more clear. When I was talking about bringing the code from the branch. I was not talking about creating the root CA certificate. In other words, keeps the current logic in the reconciliation. Just using a different package to do the job.

fabriziosestito commented 7 months ago

Looks good to me, I would leave the OTLP test as a nice to have. I've added a question to https://github.com/kubewarden/kubewarden-controller/issues/664