BeforeSuite and AfterSuite modules not running in the ginkgo v2

[dashanji]

What broke? What's expected?

What broke?
The BeforeSuite and AfterSuite modules not running in the ginkgo v2. In other words, the current make test do nothing in all project with the ginkgo v2.
Refer to the ginkgo v2.0 note, the BeforeSuite and AfterSuite will not run if all tests in a suite are skipped.

What's expected?
In the project with the ginkgo v2, the make test command can run successfully as expected and do the operations such as installing CRD actually.

Reproducing this issue

1. Move into a test-project with ginkgo v1.

$ cd project

Add a wrong assertion as follows.

var _ = BeforeSuite(func(done Done) {
    Expect(1).To(Equal(2))
}, 60)

var _ = AfterSuite(func() {
    Expect(1).To(Equal(2))
})

Run make test, and you will get some errors because the BeforeSuite module executed actually.

2. Move into a test-project with ginkgo v1.

$ cd project/testdata/project-v4

Add a wrong assertion as follows.

var _ = BeforeSuite(func() {
    Expect(1).To(Equal(2))
})

Run make test, and you will run successfully because the BeforeSuite module not executed actually.

KubeBuilder (CLI) Version

3.12.0

PROJECT version

3

Plugin versions

# Code generated by tool. DO NOT EDIT.
# This file is used to track the info used to scaffold your project
# and allow the plugins properly work.
# More info: https://book.kubebuilder.io/reference/project-config.html
domain: testproject.org
layout:
- go.kubebuilder.io/v4
projectName: project-v4
repo: sigs.k8s.io/kubebuilder/testdata/project-v4
resources:
- api:
    crdVersion: v1
    namespaced: true
  controller: true
  domain: testproject.org
  group: crew
  kind: Captain
  path: sigs.k8s.io/kubebuilder/testdata/project-v4/api/v1
  version: v1
  webhooks:
    defaulting: true
    validation: true
    webhookVersion: v1
- api:
    crdVersion: v1
    namespaced: true
  controller: true
  domain: testproject.org
  group: crew
  kind: FirstMate
  path: sigs.k8s.io/kubebuilder/testdata/project-v4/api/v1
  version: v1
  webhooks:
    conversion: true
    webhookVersion: v1
- api:
    crdVersion: v1
  controller: true
  domain: testproject.org
  group: crew
  kind: Admiral
  path: sigs.k8s.io/kubebuilder/testdata/project-v4/api/v1
  plural: admirales
  version: v1
  webhooks:
    defaulting: true
    webhookVersion: v1
- controller: true
  domain: testproject.org
  group: crew
  kind: Laker
  version: v1
version: "3"

Other versions

No response

Extra Labels

No response

[camilamacedo86]

Hi @dashanji,

Thank you for highlighting this issue.

It's crucial to clarify that within suite_test, we shouldn't house the tests directly. This file is designated for configuration, as well as pre and post-test run code.

By convention, tests should be implemented under the specific controller type. Here's an illustrative example:

With this structure, it's understandable that ginkgo might ignore the suite_test.go when no tests are present. However, introducing an empty test to suite_test is not the solution as it might inadvertently prompt contributors to write tests there. This action would contravene good practices and the conventions of Golang, as reflected in PR: #3631.

Here's a potential path forward:

1. Distinct Test Files: Scaffold a unique test file for every controller and webhook.

2. Guidance for Test Implementation: Either provide a universally valid basic test case or, minimally, furnish a TODO(user) comment guiding users on subsequent steps. For insights, the source code's reconcile method and associated TODO comments could be referenced.

3. Modifications to Deploy Image Plugin: There might be a need to introduce minor tweaks to the deploy image plugin. This plugin, by default, scaffolds tests for each controller. Check out this specific link. Likely, appending m.IfExistsAction = OverwriteFile to this template will suffice. This will ensure an overwrite of the controller test if one is already scaffolded.

Furthermore, regarding the new templates for tests:

• For adding new templates in this directory, create a new file, e.g., controller_test_template.go, and add your scaffolded tests there.

• Similarly, for the API directory, you can add another file, e.g., api_test_template.go.

Make sure to differentiate between APIs which have webhooks ONLY and the ones for each controller. Use suite_test.go as a base, but customize it according to the requirements of each scenario.

Looking forward to a collaborative resolution.

[Sajiyah-Salat]

Hey, How it is a good-first-issue? REading the full issue overwhelmed me. I dont think this is a good-first-issue.

[dashanji]

/assign

Hey, How it is a good-first-issue? REading the full issue overwhelmed me. I dont think this is a good-first-issue.

Hi @Sajiyah-Salat. I think it's a good first issue for people like me who have experience using kubebuilder but no actual development experience. Also, the code modification is small and we can refer to the existing template to get some insights.

Distinct Test Files: Scaffold a unique test file for every controller and webhook.

Hi @camilamacedo86, it looks like we need to add a new command like kubebuilder create unittest for the previous controllers/webhooks without unittest and add a new step INFO Create UnitTest [y/n] for the newly added controllers/webhooks, WDYT?

[Sajiyah-Salat]

Okay, would you like to take this up. It would be great as you have gone through the issue. If not let me know the detailed step I will make it happen.

[dashanji]

Okay, would you like to take this up. It would be great as you have gone through the issue. If not let me know the detailed step I will make it happen.

Yeah, I would take it done.

[camilamacedo86]

Hi @camilamacedo86, it looks like we need to add a new command like kubebuilder create unittest for the previous controllers/webhooks without unittest and add a new step INFO Create UnitTest [y/n] for the newly added controllers/webhooks, WDYT?

No. We just need to create the templates and call them in the subcommands to create controllers and webhooks
See for example we call the templates when we create api and controllers: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/api.go#L94-L112

So, we need to add the templates and call them out in the commands.
therefore, that means when we call kb create API and we create a controller we will also scaffold the controller_test.go for each controller. Same for webhooks.

[camilamacedo86]

HI @dashanji @Sajiyah-Salat

Following the some clarification about what was requested in this task and how to achieve the goal.
I hope that helps out.

Goal

Ensure that every scaffolded controller and webhook have a corresponding test file. The test file should contain a significant test to ensure that the suite isn't merely ignored. For instance, a test assertion like 1 == 2 should result in a failure when make test is executed.

Example

var _ = BeforeSuite(func(done Done) {
Expect(1).To(Equal(2))
}, 60)

var _ = AfterSuite(func() {
Expect(1).To(Equal(2))
})

Implementation Steps

1. Design a template for scaffolding tests specific to each controller.
   • Reference: controller_suitetest.go
2. Integrate this template into the scaffolding process. This will ensure that when kubebuilder create api is run with --controller=true, the corresponding test for that controller will be scaffolded.
   • Reference: api.go
3. Extend the same logic and templating process for webhook scaffolds.
4. Investigate and ensure that the solution applied for webhooks can work well with all types of webhooks. That means we might need to have a template for each kind such as:

func (f *WebhookSuite) SetTemplateDefaults() error {
    // ... existing logic ...

    switch f.WebhookType {
    case "defaulting":
        f.TemplateBody = defaultingWebhookTestSuiteTemplate
    case "validating":
        f.TemplateBody = validatingWebhookTestSuiteTemplate
    case "conversion":
        f.TemplateBody = conversionWebhookTestSuiteTemplate
    default:
        return fmt.Errorf("unknown webhook type: %s", f.WebhookType)
    }
    // ... remaining logic ...
}

Scaffold examples

For controllers

package controller

import (
	"context"
	"time"

	//nolint:golint
	. "github.com/onsi/ginkgo/v2"
	. "github.com/onsi/gomega"
	"k8s.io/apimachinery/pkg/api/errors"
	"k8s.io/apimachinery/pkg/types"
	"sigs.k8s.io/controller-runtime/pkg/reconcile"

	 // HERE will be the API see how your template will be implemented for it works: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/v4/scaffolds/internal/templates/controllers/controller.go#L77-L79
	apiversion "path/to/your/api/version"
)

var _ = Describe("Controller<Kind>Reconciler", func() {
	Context("When reconciling a resource", func() {

		const resourceName = "test-resource"

		ctx := context.Background()

		typeResourceName := types.NamespacedName{
			Name:      resourceName,
			Namespace: "default",  // TODO(user):Modify as needed
		}
		resource := &yourapiversion.YourResourceKind{} //HERE SHOULD BE THE KIND

		BeforeEach(func() {
			testResource := &yourapiversion.YourResourceKind{
				ObjectMeta: metav1.ObjectMeta{
					Name:      resourceName,
					Namespace: "default",
				},
				// TODO (user): Specify other spec details if needed.
			}
			Expect(k8sClient.Create(ctx, testResource)).To(Succeed())
		})

		AfterEach(func() {
			// TODO(user): Cleanup logic after each test, like removing the resource instance.

			By("Cleanup the specific resource instance <KIND>")
			Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
		})

		It("should successfully reconcile the resource", func() {
			By("Reconciling the created resource")
			controllerReconciler := &YourControllerReconciler{
				Client: k8sClient,
				Scheme: k8sClient.Scheme(),
			}

			_, err := controllerReconciler.Reconcile(ctx, reconcile.Request{
				NamespacedName: typeResourceName,
			})
			Expect(err).To(Not(HaveOccurred()))

			// TODO(user): Add more specific assertions depending on your controller's reconciliation logic.
			// Example: If you expect a certain status condition after reconciliation, verify it here.
		})
	})
})

For Webhooks

var _ = Describe("MyResource Webhook", func() {
    Context("When creating MyResource", func() {
        It("Should deny if a required field is empty", func() {
            // Assuming MyResource has a field "Name" that shouldn't be empty.
            myResource := &MyResource{
                Spec: MyResourceSpec{
                    Name: "",
                    // other fields...
                },
            }

            err := k8sClient.Create(context.Background(), myResource)
            Expect(err).To(HaveOccurred())  // Expect an error because the Name field is empty.
            // Optionally, you can check for specific error messages or types.
        })

        It("Should admit if all required fields are provided", func() {
            myResource := &MyResource{
                Spec: MyResourceSpec{
                    Name: "ValidName",
                    // other fields...
                },
            }

            err := k8sClient.Create(context.Background(), myResource)
            Expect(err).NotTo(HaveOccurred()) // Expect no error as it's a valid resource.
        })
    })
})

Acceptance Criteria

• Running kubebuilder create api --controller=true should scaffold both the controller and its respective test file.
• The test file should contain at least one significant test example.
• Similar functionality should be extended when creating a webhook.
• Running make generate to update the samples. The tests generated for the sample under testdata/project-v4-with-deploy-image should still be there.See; https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4-with-deploy-image/internal/controller/busybox_controller_test.go
• The scaffold should follow the scaffolds done by the project. That means the tests should have something like an TODO(user): here you should implement your test for x.
• The tests should be generic and be valid for ANY controller or webhook
• Ensure that the test scaffolding for webhooks is tailored to their specific type. As part of this task, evaluate whether it would be beneficial to have distinct templates for defaulting, validating, or conversion webhooks instead of a one-size-fits-all approach. If differentiated templates would be more effective, they should be implemented.

[camilamacedo86]

I removed /good-first-issue
since it is not a small change

[Sajiyah-Salat]

Will work on that soon! Thank you for detailed steps
Hello @dashanji are you willing to work on this? if not I would like to take this up.

[dashanji]

Will work on that soon! Thank you for detailed steps Hello @dashanji are you willing to work on this? if not I would like to take this up.

Hi @Sajiyah-Salat, I just updated the PR to address this issue. If you have any suggestions, please let me know. Your assistance has been invaluable. Thank you.