Standardize CAPI tests

[fabriziopandini]

Originally this issue was about a different topic (see https://github.com/kubernetes-sigs/cluster-api/issues/3309), but the discussion here took a different direction. So I'm updating issue title/description in order to give better visibility to the highly valuable comments about standardizing tests in CAPI

[vincepri]

I'd add we should also consider moving the tests to ginkgo and standardize on those across the codebase, it could be done as part of this effort for KCP.

/milestone v0.3.x /priority important-longterm

[fabriziopandini]

I'm -1 to move to ginkgo. I personally prefer a combination of go-test and gomega so it is possible to have a much better developer experience from different IDE (like running a single test with a click or debugging)

[vincepri]

Taking a close look at controller-runtime, it's nice that all tests look the same and are consistent. We can discuss it in the next milestone, although I'd like to reduce the amount of context switch between tests and make it so if you can read one, you can read all of them.

[benmoss]

Yes I am -1 to ginkgo as well, I think it encourages difficult to understand test code

[vincepri]

@fabriziopandini will have a proposal at some point. The goal is to have consistent, easy-to-read, and useful tests. Ginkgo is just a tool to achieve that, although I think Fabrizio's idea of using simple go tests with envtest should work out.

[detiber]

I like the idea of standardizing tests, but I think we should likely group things into types of tests and standardize based on the test type:

• e2e style tests (components built and running as containers against test-driven inputs) make sense to fully standardize on ginkgo/gomega

• unit style tests (component code being called directly by tests without any dependencies on other controllers running) probably should avoid ginkgo, but should still standardize on gomega (which we already do mostly)

  • We still probably need to decide if it makes sense to standardize on envtest here or possibly still use a fake client where appropriate
  • Using ginkgo for these tests:
  • makes interactive debugging of tests more difficult, since you can't just run the test using the debugger directly
  • makes it more difficult to only run a subset of tests, since you can't just run or debug individual tests using an IDE, you now need to run the tests using make or the command line and override the focus to select individual tests

We might need to also scope out what the middle ground between these two looks like (integration-style tests), and weigh the benefits/drawbacks of potentially standardizing on ginkgo/gomega for them. I don't think I have much of an opinion either way for these at this time.

[wfernandes]

I'm +1 to using standard golang test framework with design patterns along with Gomega matcher library.

I've seen Ginkgo being misused to making tests more complicated. E.g. nested BeforeEach, JustBeforeEach. Also I've noticed writing table-driven tests are kinda un-intuitive with DescribeTable IMO.

But I also think having tests that match the community will make it easier to attract and enable contributors to the CAPI project. I noticed that kubebuilder and controller-runtime both use Ginkgo/Gomega and their tests are structured as fairly flat and follow simple hygiene which I'm all for wherever we end up using Ginkgo.

Regarding debuggers...well I'm so used to fmt.Println that I rarely have to bust out the debugger 😄

[vincepri]

Thanks for the extra details @detiber and @wfernandes. During v1alpha4 we should revisit this topic and provide guidelines and policies across the board.

One thing I'd also like to do is to completely remove any use of the fake controller-runtime client, if possible. It has multiple issues and it doesn't work well with patches. Its use should be greatly reduced if not removed. If we standardize on go test without Ginkgo, and run envtest (we can also find extra optimizations to run it faster, or re-use existing clusters) we should be able to use real clients to perform the majority of tests.

That said, all of this is a lot of busy-work and probably needs a proposal.

[benmoss]

I was thinking about this today and wondering if everyone is just using Ginkgo because k8s does, while meanwhile consensus in sig-testing and beyond seems to be "we wish we could remove it out but it's too much work"

[fabriziopandini]

/assign /lifecycle active

[vincepri]

@fabriziopandini What's the action item out of this issue? Are we going to write up a small proposal? Does this effort include the e2e tests suites as well, or should we avoid changing those given that are used externally?

[fabriziopandini]

I'm starting to work on a prototype/proposal and try to figure out how to address all the comments above. Personally I would prefer to avoid a CAEP and discuss on PRs/changes on the contributor docs, but let's see how this shapes out

[vincepri]

/milestone v0.3.9