Improve Ginkgo/Gomega Test Practices in Scaffolded and e2e Tests

[camilamacedo86]

What do you want to happen?

Kubebuilder creates/scaffolds a lot of test code. This test code uses Ginkgo and Gomega. Some of the tests as written don't take full advantage of Gomega's testing framework. Since Kubebuilder is likely to introduce many developers to Gomega, it is important that the scaffolded code provides the best possible examples.

We should see that the tests scaffolded for end-users have this issue fixed already: testdata/project-v4/test.

What we need to do here is ensure that ALL our e2e tests are also following the same patterns as the end-user scaffolds. Therefore, the necessary changes must be implemented in: test/e2e.

Context

The context for this issue and similar discussions can be found here: kubernetes-sigs/kubebuilder#4135.

Tasks

1. Review and compare the test scaffolds in testdata/project-v4/test to ensure best practices in Ginkgo/Gomega are being followed.
2. Update all e2e tests in test/e2e to align with these patterns and provide the best examples for developers.
3. Validate that the tests maintain readability while leveraging Gomega's framework effectively.

Extra Labels

/kind cleanup

[Sijoma]

/assign

[mogsie]

In #4135, I originally made a suggestion of replacing the following type of tests:

result, err := someFunc(param, param)
Expect(err).NotTo(HaveOccurred())
Expect(result).To(ContainSubstring("blah"))

with the more concise (but functionally equivalent)

Expect(someFunc(param, param)).To(ContainSubstring("blah"))

The one line above

• checks that err is nil
• and that the resullt of the function call matches the matcher.

Should we reconsider this? The one line is (to me) more maintainable, although it relies Gomega implicitly testing that err == nil of the last return value.

From #4135:

... this has been decided against, as it hides the number of return values, which makes it harder to learn the API being tested. The desire is therefore to keep the three-line version, instead of in-lining it.

Maybe we should provide a document that describes the desired test idioms for the scaffolded tests and for the internal tests? Because, as mentioned, kubebuilder will often be introducing both Ginkgo and Gomega to its users; we should probably spend time explaining some of those test idioms in the scaffolded tests.

e.g. adding a comment as denoted by /**/ below:

	BeforeAll(func() {
		By("creating manager namespace")
		cmd := exec.Command("kubectl", "create", "ns", namespace)
		_, err := utils.Run(cmd)
		Expect(err).NotTo(HaveOccurred(), "Failed to create namespace")

		By("labeling the namespace to enforce the restricted security policy")
		cmd = exec.Command("kubectl", "label", "--overwrite", "ns", namespace,
			"pod-security.kubernetes.io/enforce=restricted")
/**/		// Expect(...).Error() allows us to make assertions about any errors
/**/		Expect(utils.Run(cmd)).Error().NotTo(HaveOccurred(), "Failed to label namespace with restricted policy")

This way we would be "teaching" the API (in this case the utils.Run(), and then how to use Ginkgo to make assertions against that.

[camilamacedo86]

Hi @mogsie,

We really appreciate the amazing contributions you’ve been making to the tests—it’s fantastic work!
However, regarding the use of utils.Run(cmd), we need to keep it consistent with examples like the one below:

kubebuilder/testdata/project-v4/test/e2e/e2e_test.go

Lines 70 to 71 in e25aec4

	_, err = utils.Run(cmd)
	Expect(err).NotTo(HaveOccurred(), "Failed to deploy the controller-manager")

I believe we discussed this previously and even reverted a similar change in the past—right?

Why this is important:

It makes clear what is returned from utils.Run(cmd). Otherwise, requiring explicit checks for the function reduces its maintainability. If we need to add comments to explain its behavior, that indicates the code itself isn’t as clear as it could be. Don't you agree?

Most importantly, this approach allows us to check for errors encountered during the tests. Remember that we checked that?

[camilamacedo86]

So, to close this one, IHMO it is only missing:

Replace the ExpectWithOffset. See: https://github.com/search?q=repo%3Akubernetes-sigs%2Fkubebuilder%20ExpectWithOffset(1&type=code

Then, we can close this one.

[mogsie]

I believe we discussed this previously and even reverted a similar change in the past—right?

Yes, but I think it happened on slack, so it is hard to find. This is why I think maybe these decisions should live in a document in the repo, for contributors to get guidance.

Why this is important:

It makes clear what is returned from utils.Run(cmd).

utils.Run(cmd) is invoked many times elsewhere in the scaffolded code, capturing its results into variables, when they are necessary for next steps. This should be enough. Here is an example.

kubebuilder/testdata/project-v4/test/e2e/e2e_test.go

Lines 154 to 159 in e25aec4

	podOutput, err := utils.Run(cmd)
	g.Expect(err).NotTo(HaveOccurred(), "Failed to retrieve controller-manager pod information")
	podNames := utils.GetNonEmptyLines(podOutput)
	g.Expect(podNames).To(HaveLen(1), "expected 1 controller pod running")
	controllerPodName = podNames[0]
	g.Expect(controllerPodName).To(ContainSubstring("controller-manager"))

If we need to add comments to explain its behavior, that indicates the code itself isn’t as clear as it could be. Don't you agree?

The comment is mainly to explain why two styles of assertions are used in the same function body. I would personally prefer to only do the second style, removing the need for a comment.

The implicit checking of errors of Gomega is indeed "hidden", but not embracing it results in a lot of extra noise and typing. Just in that e2e_test.go file, there are ~20 such "expect err not to have occurred" lines, and additional noise introduced by temporary variables that are often only used once By not showing this way of writing tests, the scaffolded code encourages IMHO bad habits.

Most importantly, this approach allows us to check for errors encountered during the tests. Remember that we checked that?

I'm not quite sure I understand this comment. The errors are automatically checked for nil-ness, unless you expect an error. If you're talking about making assertions on the error itself, then yes, you can still do this:

kubebuilder/docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/cronjob_webhook_test.go

Lines 116 to 118 in e25aec4

	Expect(validator.ValidateCreate(ctx, obj)).Error().To(
	MatchError(ContainSubstring("must be no more than 52 characters")),
	"Expected name validation to fail for a too-long name")