🌱improve Ginkgo/Gomega test style #4426

Sijoma · 2024-12-14T11:59:48Z

Updates a couple of e2e tests to use Gomega style mentioned in #4135 .

I'm not sure if I 100% understood the issue so before I try to rewrite all of them I thought it would be better to create a smaller PR to get feedback earlier.

relates #4424

k8s-ci-robot · 2024-12-14T11:59:57Z

Hi @Sijoma. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

camilamacedo86 · 2024-12-14T19:47:06Z

test/e2e/v4/plugin_cluster_test.go

+			g.ExpectWithOffset(2, err).NotTo(HaveOccurred())
 			// check that ca should be long enough, because there may be a place holder "\n"
-			ExpectWithOffset(2, len(mwhOutput)).To(BeNumerically(">", 10))
+			g.ExpectWithOffset(2, len(mwhOutput)).To(BeNumerically(">", 10))


The idea is to follow the same approach as the test scaffolds: https://github.com/kubernetes-sigs/kubebuilder/tree/master/testdata/project-v4/test (as closer as possible is the ideal, same standards of checks)
If you look there, you will see that it is quite similar to what we do here. You can indeed compare the same checks for metrics etc.

For example, we should no longer use ExpectWithOffset; instead, we should use the functions and simplify it.

Look at #4135

And check the PRs done by @mogsie related to the tests: https://github.com/kubernetes-sigs/kubebuilder/pulls?q=is%3Apr+author%3Amogsie+is%3Aclosed

I think that will help out understand better what we are trying to do,
BTW, thank you a lot for checking this one

Sijoma · 2024-12-22T13:18:25Z

@camilamacedo86 does this go in the right direction?

Some test looked like they could be refactored to a table driven test, the ones with the Run function. I've kept it like this for now.

It's a bit confusing since #4135 still contains examples that contain EventuallyWithOffset. I've looked at https://github.com/kubernetes-sigs/kubebuilder/pull/4137/files and it looks a bit similar. Although Eventually still uses the global and not g.Eventually (Maybe this is actually incorrect from my side).

Happy to get an example if some of these changes are not correct how to fix it.

Edit: This doesn't even work as the Context nodes cannot have a function where Gomega is passed.

camilamacedo86 · 2024-12-23T11:47:58Z

test/e2e/v4/plugin_cluster_test.go

 	var controllerPodName string
 	var err error

 	By("creating manager namespace")


Hi @Sijoma

Please, ensure that you have your changes released with master
Also, please squash the commits; We should not have more than 1 commit to be able to get it merged

Thank you 🥇

Squashed & Rebased it now.

Thanks!

mateusoliveira43 · 2024-12-23T12:51:10Z

test/e2e/v4/plugin_cluster_test.go


 	By("validating that the Prometheus manager has provisioned the Service")
-	EventuallyWithOffset(1, func() error {
+	Eventually(func(g Gomega) {


I think there is no need to change function inside Eventually. The .Should(Succeed()) will do the assertion for us

I found it a bit simpler to not have return values inside these functions. It's a bit simpler to reason as there is nothing returned.

That way we can already immediately see that the error should never occur. Whereas when we return it it could be still checked for NotFound etc.

WDYT?

Using a func(Gomega) in an Eventually is more "scalable" in that if you want to do more things inside the function, you can keep to the Gomega semantics with assertions.

The error messages tell you exactly what's wrong.

The line number of the error is correct (the error points you to the line inside the func() not just the line of the Eventually() call).

No need to switch between two different ways of checking for errors in tests.

So my vote is to keep this change.

camilamacedo86 · 2024-12-25T17:30:44Z

/ok-to-test

Sijoma · 2024-12-27T08:24:47Z

/retest

Sijoma · 2024-12-27T08:25:46Z

Not really sure what went wrong here:

I see the following logs:

✗ Waiting ≤ 1m0s for control-plane = Ready ⏳
• WARNING: Timed out waiting for Ready ⚠️

Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal Scheduled 5s default-scheduler Successfully assigned e2e-vlfy-system/e2e-vlfy-controller-manager-6c94678688-6k4dd to kind-control-plane
Warning FailedMount 4s kubelet MountVolume.SetUp failed for volume "kube-api-access-fkgst" : failed to sync configmap cache: timed out waiting for the condition

Summarizing 6 Failures:
[FAIL] kubebuilder plugin go/v4 [It] should generate a runnable project
/home/prow/go/src/sigs.k8s.io/kubebuilder/test/e2e/v4/plugin_cluster_test.go:336

Sijoma · 2024-12-27T08:37:28Z

Seeing it now:

[FAILED] Unexpected non-nil/non-zero argument at index 1:
<*utils.TestContext>: &utils.TestContext{CmdContext:(*utils.CmdContext)(0xc0004767c0), TestSuffix:"tdqg", Domain:"example.comtdqg", Group:"bartdqg", Version:"v1alpha1", Kind:"Footdqg", Resources:"footdqgs", ImageName:"e2e-test/controller-manager:tdqg", BinaryName:"kubebuilder", Kubectl:(*utils.Kubectl)(0xc00014cc90), K8sVersion:(*utils.KubernetesVersion)(0xc000136600), IsRestricted:false}
In [It] at: /home/prow/go/src/sigs.k8s.io/kubebuilder/test/e2e/v4/plugin_cluster_test.go:336 @ 12/25/24 17:53:14.473

relates kubernetes-sigs#4424

Sijoma · 2024-12-27T09:46:32Z

Also realised now that make test isn't even running these tests 😄. Using make test-e2e-local now.

mogsie · 2024-12-27T12:17:26Z

test/e2e/v4/plugin_cluster_test.go

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


I also like this simpler in-line Expect(...).Error().NotTo(HaveOccurred()) — any reason why you made this change (only) here, and not the other places?

I started to work through it a bit - and initially changed a couple of more things (also incorrect ones). So I didn't want to move everything in one go. I then focused really on all the "...WithOffset" calls and the functions passed to Eventually(...)

Happy to add also change it in more places. I agree that it's better. Maybe just as a follow up pr? Or is there a preference to have it more in one PR?

It's probably ok to do it in several PRs. I'd keep it focused and to-the-point, so that PRs don't need to live too long and need constant rebasing etc. The PR you have already fixes a lot of actual bugs (i.e. the Eventually() calls that use the global assertions).

I remember a few months back, when I fixed assertion code like this, I discovered that the test itself was testing the completely wrong thing, and the incorrect assertions just happened to allow the test to pass.

So, when you do fix things like this, you might end up with tests that don't pass, and it might just be that the test itself was not testing what the original authors thought it was testing.

camilamacedo86

/lgtm
/ok-to-test
/approve

Well DONE 🥇

k8s-ci-robot · 2024-12-27T17:54:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mogsie, Sijoma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from rashmigottipati and varshaprasad96 December 14, 2024 11:59

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 14, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 14, 2024

camilamacedo86 reviewed Dec 14, 2024

View reviewed changes

camilamacedo86 self-requested a review December 14, 2024 19:50

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 22, 2024

camilamacedo86 reviewed Dec 23, 2024

View reviewed changes

mateusoliveira43 reviewed Dec 23, 2024

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 23, 2024

Sijoma force-pushed the master branch from dda1738 to 16e9a2a Compare December 24, 2024 11:29

Sijoma requested review from camilamacedo86 and mateusoliveira43 December 24, 2024 11:31

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 25, 2024

Sijoma force-pushed the master branch from 16e9a2a to a9ae54d Compare December 27, 2024 08:52

🌱improve Ginkgo/Gomega test style

ca5f69b

relates kubernetes-sigs#4424

Sijoma force-pushed the master branch from a9ae54d to ca5f69b Compare December 27, 2024 09:34

mogsie reviewed Dec 27, 2024

View reviewed changes

mogsie approved these changes Dec 27, 2024

View reviewed changes

camilamacedo86 approved these changes Dec 27, 2024

View reviewed changes

k8s-ci-robot assigned camilamacedo86 Dec 27, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 27, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 27, 2024

k8s-ci-robot merged commit d16e6c3 into kubernetes-sigs:master Dec 27, 2024
17 checks passed

camilamacedo86 mentioned this pull request Dec 28, 2024

Next Kubebuilder Release v4.4.0: Planning and Alignment #4427

Closed

Copilot AI mentioned this pull request Oct 30, 2025

📖 (multiversion-tutorial): Show v2 webhook implementation alongside v1 #5159

Merged

🌱improve Ginkgo/Gomega test style #4426

🌱improve Ginkgo/Gomega test style #4426

Conversation

Sijoma commented Dec 14, 2024

Uh oh!

k8s-ci-robot commented Dec 14, 2024

Uh oh!

camilamacedo86 Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sijoma commented Dec 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camilamacedo86 Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

Sijoma Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

mateusoliveira43 Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

Sijoma Dec 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mogsie Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 commented Dec 25, 2024

Uh oh!

Sijoma commented Dec 27, 2024

Uh oh!

Sijoma commented Dec 27, 2024

Uh oh!

Sijoma commented Dec 27, 2024

Uh oh!

Sijoma commented Dec 27, 2024

Uh oh!

mogsie Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

Sijoma Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

mogsie Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Dec 27, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

camilamacedo86 Dec 14, 2024 •

edited

Loading

Sijoma commented Dec 22, 2024 •

edited

Loading

Sijoma Dec 24, 2024 •

edited

Loading