Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
4 tasks
jvanz opened this issue Mar 1, 2024 · 3 comments
Open
4 tasks

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

jvanz opened this issue Mar 1, 2024 · 3 comments

Comments

@jvanz
Copy link
Member

jvanz commented Mar 1, 2024

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 #648

Acceptance criteria

@flavio
Copy link
Member

flavio commented Mar 1, 2024

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
Copy link
Member Author

jvanz commented Mar 4, 2024

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
Copy link
Contributor

Looks good to me, I would leave the OTLP test as a nice to have.
I've added a question to #664

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

3 participants