Clearly mark test constructors in CA classes or move them out of the production code

[scholzj]

All the Ca classes (Ca, ClientsCa, ClusterCa) and to some extend also the new CaConfig class have various constructors. Some of them are for tests only. But it is not clearly denominated as we probably need to keep them as public given the variety of places they are used from. I think that could get some improving …

• Maybe something simple such as improving the Javadoc comments to clearly state they are used for tests only
• But maybe we could also consider having some static creator methods in some test utils class to abstract the test defaults and keep these constructors away from the production code.

(Based on observation during #12680 review, briefly discussed in https://cloud-native.slack.com/archives/C018247K8T0/p1777983551564739)

[see-quick]

I think I saw in multiple huge projects annotation @VisibleForTesting (which could live in operator-common), I think that would be just simple annotation for marking such constructors). I know that our current convention is just using /* test */ in those parts right but maybe having such annotation would better?

But maybe we could also consider having some static creator methods in some test utils class to abstract the test defaults and keep these constructors away from the production code.

+1

[scholzj]

TBH, I think such an annotation is useless. Especially given it leaves unnecessary methods in the production code-space.

From my perspective the proper solution is to:

• Have any constructors using dummy values in the test code itself as that is super dangerous. That should be pretty simple as they often just pass dummy values instead of real values. So you can easily have some factory method in tests to do that.
• Regular constructors needed for tests only should:
  • ideally no need to be public and use the /* test */ marker (that is a common pattern in Strimzi)
  • If they need to be public because of the package structure, we can mark it in the Javadoc, but we should make sure they are real constructors and for example take just different format of the parameters (e.g. 5 separate parameters instead of some API class) for easier testing. But they should not set dummy values inside.

I do not think a custom annotation would help if it doesn't do anything real (such as throw checkstyle error when used outside of test classes etc).

[scholzj]

Triaged and approved asynchronously (👍)