Some Gomega assertions are long-winded

[mogsie]

trafficstars

What broke? What's expected?

Kubebuilder creates/scaffolds a lot of test code. This test code uses Ginkgo and Gomega. Some of the tests as written don't take advantage of Gomega's testing framework. Since kubebuilder is likely to be one of those projects that introduce developers to Gomega, I think it is useful if the scaffolded code provides the best possible code samples.

~Avoiding Expect(err)~

For example, from test/e2e/v4/plugin_cluster_test.go

		outputGet, err := kbc.Kubectl.Get(
			false,
			"pods",
			"-n", "kube-system",
			"-l", "k8s-app=calico-node",
			"-o", "jsonpath={.items[*].status.phase}",
		)
		Expect(err).NotTo(HaveOccurred(), "Failed to get Calico pods")
		Expect(outputGet).To(ContainSubstring("Running"), "All Calico pods should be in Running state")

adds two variables to the scope, and then checks both responses. the two assertions could be replaced with an inline call:

		Expect(kbc.Kubectl.Get(
			false,
			"pods",
			"-n", "kube-system",
			"-l", "k8s-app=calico-node",
			"-o", "jsonpath={.items[*].status.phase}",
		).To(ContainSubstring("Running"), "All Calico pods should be in Running state")

This ensures that the error is nil, in addition to checking the output.

Edit 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.

Errors

Similarly, when the output is not checked, the code generally has Expect(err). This can also be in-lined:

			_, err = kbc.Kubectl.Command("label", "namespaces", kbc.Kubectl.Namespace,
				"metrics=enabled")
			ExpectWithOffset(2, err).NotTo(HaveOccurred())

can be shortened to

			Expect(kbc.Kubectl.Command("label", "namespaces", kbc.Kubectl.Namespace,
				"metrics=enabled").Error().NotTo(HaveOccurred())

The Error() call ensures that the last return value (the error) is nil, and ignores the output.

Eventually

The use of Eventually() often uses a mix of assertion styles because the function passed to Eventually is not passed a Gomega instance:

		EventuallyWithOffset(1, func() error {
			_, err := kbc.Kubectl.Get(
				true,
				"secrets", "webhook-server-cert")
			return err
		}, time.Minute, time.Second).Should(Succeed())

could be simplified to

		EventuallyWithOffset(1, func(g Gomega) {
			g.Expect(kbc.Kubectl.Get(
				true,
				"secrets", "webhook-server-cert")).Error().ToNot(HaveOccurred())
		}, time.Minute, time.Second).Should(Succeed())

Reproducing this issue

No response

KubeBuilder (CLI) Version

master

PROJECT version

No response

Plugin versions

No response

Other versions

No response

Extra Labels

/kind cleanup

[mogsie]

Some other weird stuff I found while fixing things:

This code never worked (as intended), but was hiding a configuration error:

By("validating that pod(s) status.phase=Running")
getMemcachedPodStatus := func() error {
	status, err := kbc.Kubectl.Get(true, "pods", "-l",
		fmt.Sprintf("app.kubernetes.io/name=%s", kbc.Kind),
		"-o", "jsonpath={.items[*].status}",
	)
	ExpectWithOffset(2, err).NotTo(HaveOccurred())
	if !strings.Contains(status, "\"phase\":\"Running\"") {
		return err
	}
	return nil
}
EventuallyWithOffset(1, getMemcachedPodStatus, time.Minute, time.Second).Should(Succeed())

The "if not strings contains" returns err which is nil. It exposes that it's checking the wrong pod: by sprintf'ing app.kubernetes.io/name=%s", kbc.Kind — resulting in -l app.kubernetes.io/name=Foorelf

When I fixed the code to be "correct" the test failed because it never could find this pod :smile:

edit: ~The fix is just wrapping kbc.Kind with strings.ToLower.~ edit: The fix is replacing kbc.Kind the "e2e" prefix.

[mogsie]

The following test fails for me because the resulting pod tries to run as root and for some reason my kind cluster disallows that.

func creatingAPI(kbc *utils.TestContext) {
	By("creating API definition with deploy-image/v1-alpha plugin")
	err := kbc.CreateAPI(
		"--group", kbc.Group,
		"--version", kbc.Version,
		"--kind", kbc.Kind,
		"--plugins", "deploy-image/v1-alpha",
		"--image", "busybox:1.36.1",
		"--make=false",
		"--manifests=false",
	)
	ExpectWithOffset(1, err).NotTo(HaveOccurred())
}

Is it OK for me to add a --run-as-user 1001 option here? Either that, or I could change the verification to verify that the podspec doesn't have runAsUser? But I'm not 100% sure of the main reason behind this test.

[mogsie]

@camilamacedo86 I hope that it's OK for me to make small, focused PRs as I make may way around the codebase. I prefer many smaller, short-lived PRs like this, to avoid merge conflicts that changes like this could cause.

[camilamacedo86]

Hi @mogsie

One problem that I notice with the changes is that we are no longer able to check the results when them fails. See: from https://github.com/kubernetes-sigs/kubebuilder/actions/runs/10807585375/job/29978696165?pr=4154

  Expected
      <[]uint8 | len:151, cap:1024>: [78, 65, 77, 69, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 69, 78, 68, 80, 79, 73, 78, 84, 83, 32, 32, 32, 65, 71, 69, 10, 112, 114, 111, 106, 101, 99, 116, 45, 118, 52, 45, 109, 117, 108, 116, 105, 103, 114, 111, 117, 112, 45, 99, 111, 110, 116, 114, 111, 108, 108, 101, 114, 45, 109, 97, 110, 97, 103, 101, 114, 45, 109, 101, 116, 114, 105, 99, 115, 45, 115, 101, 114, 118, 105, 99, 101, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 32, 50, 109, 49, 115, 10]
  to contain substring
      <string>: 8443

So, we need to change all in its regards. I mean, we need to ensure that we are able to check the output message as before in the logs. Because as a developer I need to know why my tests is not passing and what is the output. That is much more important than make the code less verbose.

Do you think that you could solve this one? Maybe it is only missing covert the bytes to string

You can use the PR: https://github.com/kubernetes-sigs/kubebuilder/pull/4154 to validate this one.

[mogsie]

Sure, I can look at this.

[camilamacedo86]

Hi @mogsie

I found the fix for it and keep all amazing changes that you have been doing see:

https://github.com/kubernetes-sigs/kubebuilder/pull/4158

[mogsie]

Hi @camilamacedo86

I hope you found the change from

func Run(cmd *exec.Cmd) ([]byte, error) {

to

func Run(cmd *exec.Cmd) (string, error) {

a satisfactory fix to the problem?

[camilamacedo86]

HI @mogsie

Please feel free to continue improve tests as you see fit. Also, to close this issue when you see that all that you want to do regards this specific topic is done.

[camilamacedo86]

Hi @mogsie

Since now we need to ensure it in our e2e tests, I created an issue with the clarifications to see if someone can help us with it. It seems a good first issue.

See: https://github.com/kubernetes-sigs/kubebuilder/issues/4424

So, I am closing this one.