Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scaffold webhook test examples do not use suite_test structure #4442

Open
mateusoliveira43 opened this issue Dec 26, 2024 · 2 comments
Open
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature.

Comments

@mateusoliveira43
Copy link
Contributor

What do you want to happen?

For example, Defaulter webhook was already setup here

err = SetupAdmiralWebhookWithManager(mgr)

And manager started here

But example tells user to manually call Defaulter webhook

// TODO (user): Add logic for defaulting webhooks
// Example:
// It("Should apply defaults when a required field is empty", func() {
// By("simulating a scenario where defaults should be applied")
// obj.SomeFieldWithDefault = ""
// By("calling the Default method to apply defaults")
// defaulter.Default(ctx, obj)
// By("checking that the default values are set")
// Expect(obj.SomeFieldWithDefault).To(Equal("default_value"))
// })

It could be simplified to

It("Should apply defaults when a required field is empty", func() { 
    By("creating a object where defaults should be applied") 
    obj.SomeFieldWithDefault = "" 
    Expect(k8sClient.Create(ctx, obj)).NotTo(HaveOccurred())

    By("checking that the default values are set") 
    Expect(obj.SomeFieldWithDefault).To(Equal("default_value")) 
}) 

Note: scaffold internal/controller/suite_test.go do not start a manager. Instead of changing users examples, another approach would be removing manager start up in webhook suite_test.go, which would be my suggestion.

Extra Labels

/kind cleanup

@mateusoliveira43 mateusoliveira43 added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 26, 2024
@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 26, 2024
@camilamacedo86
Copy link
Member

camilamacedo86 commented Dec 28, 2024

Hi @mateusoliveira43,

To ensure that I understand, you are saying that instead of calling the method that set the default values

By("calling the Default method to apply defaults")
// defaulter.Default(ctx, obj)

We should create the resource Expect(k8sClient.Create(ctx, obj)).NotTo(HaveOccurred()) so the default values should be set.

I think you are right about this one.
Good catcher 👍

@mogsie
Copy link
Contributor

mogsie commented Dec 28, 2024

Small nit, but to make assertions on the error of a function call, you should chain the Expect with Error():

    Expect(k8sClient.Create(ctx, obj)).Error().NotTo(HaveOccurred())

Gomega also provides a shorthand for this specifically for the single-error func: Use the Succeed() matcher

    Expect(k8sClient.Create(ctx, obj)).To(Succeed())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

4 participants