Simplify ca reconciler test class

[katheris]

Type of change

Select the type of your PR

• Refactoring

Description

Motivation

In main today the CaReconcilerTest has two distinct ways the tests are run:

1. Tests that call a CaReconcilerTest.reconcileCa method which sets up lots of Mockito mocks and then creates an actual CaReconciler instance and calls reconcile on it
2. Tests that call CaReconcilerTest.initTrustRolloutTestMocks which sets up different Mockito mocks and then create a MockCaReconciler instance to call reconcile on

The reason for the separation is that tests of type 1 are actually only concerned about the behaviour of reconciling the CA secrets (so CaReconciler.reconcileCas method), and do not care about rolling updates or any other changes later in the reconciliation.

Tests of type 2 are focused on checking that Kafka pods are rolled correctly, or other updates occur, such as removing old Ca certificates from the cluster CA.

This means when adding new tests (for example in future when adding tests for Cert Manager integration) it's difficult to understand the best approach without first reviewing every single test and understanding the two distinct ways that the tests work. The class is also very long so it's hard to get an overview of the test cases and find specific test cases.

Changes

To simplify the test classes for developers I have separated the tests into two distinct classes:

• CaReconcilerReconcileCasTest to test the reconcileCas method in CaReconciler in isolation
• CaReconcilerTest to test all steps in CaReconciler, particularly the rolling updates for trust

I have then gone through each class and made some further simplifications, for example asserting that secrets were created correctly in the method used to create them, rather than in each test method. In CaReconcilerReconcileCasTest the tests now also call CaReconciler.reconcileCas directly, rather than the CaReconciler.reconcile method, which allowed me to remove additional Mockito calls that weren't relevant to the tests.

I created the PR with two commits, one to just duplicate CaReconciler, and the second to take out the tests that are in the other test class and simplify the remaining tests. Hopefully this helps with reviews as in the second commit it's easier to see what changed

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• Write tests
• Make sure all tests pass
• Update documentation
• Check RBAC rights for Kubernetes / OpenShift roles
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
• Reference relevant issue(s) and close them after merging
• Update CHANGELOG.md
• Supply screenshots for visual changes, such as Grafana dashboards

[scholzj]

@katheris It might help with the reivew if you try to explain what/how you simplified it? To give better understanding what to look for.

[katheris]

@scholzj you're right, I completely failed to add any context. I've updated the description to be much clearer, sorry about that. Let me know if you have any other questions

[scholzj]

Thanks for the explanation. That seems to make sense. It is not easy to review the tests line by line, but in general it seems to look good.

Maybe you can add some class-level Javadoc comments to describe which tests should be in which class?

[katheris]

@scholzj I've added some javadoc, see what you think. I considered explicitly calling out the other test class but decided not to since it seemed overkill as the classes are next to each other in the file structure, so let me know if you think that would be useful

[see-quick]

LGTM, thanks @katheris. For future improvements: there’s a bit of redundancy in these tests (e.g., centralizing the CA builder, collapsing duplicate “initial secrets” code, de-duplicate cert/truststore helper ... ). We could extract that redundant code and make this class a bit smaller I hope 📦 .

[scholzj]

@see-quick I'm not sure what parts exactly are you talking about. But minimizing the size of a class is not the goal per-se. Readability, good design, maintainability are way more important than number of lines or characters.

---

@katheris TBH, I hoped the Javadocs can capture mainly these two points:

* CaReconcilerReconcileCasTest to test the reconcileCas method in CaReconciler in isolation
* CaReconcilerTest to test all steps in CaReconciler, particularly the rolling updates for trust

As those might be important when adding future tests. Otherwise the distinction might be gone in few more changes.

[katheris]

@see-quick thanks for the review. I did play around with extracting more methods for things like initialising the Secrets when I was doing this refactor, but since quite a few of the tests need to further customise the Secrets I found it made the tests harder to follow.

@scholzj I've pushed another update to the Javadoc

[scholzj]

@katheris LGTM, thanks.