-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
✨ (go/v4): Add tests for the webhooks and controllers which are created so that suite_test are no longer ignored #3631
Conversation
Welcome @dashanji! |
Hi @dashanji. 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 Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests should not be automatically added to the suite_test by default. Remember, the scaffold primarily sets up the suite_test, but it doesn't generate tests for individual API/controllers or webhooks.
Given this, I'm curious: under what circumstances would you add assert checks to the Before/AfterSuite without first implementing comprehensive tests for each controller and/or webhook, especially in light of the issue scenario you've described? Could you please provide more details on this?
Moreover, if we do opt to include placeholder tests to address this concern, we should:
- Scaffold a distinct test file for each controller and webhook.
- Consider including a basic test case that would universally be valid, or at the very least, include a TODO(user): note guiding users on the next steps. For reference, you might want to check out the source code—particularly the reconcile method and the accompanying TODO comments.
- Lastly, we might need to perform small changes to the implementation of the deploy image plugin since this one scaffold tests for each controller by default. See: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/deploy-image/v1alpha1/scaffolds/internal/templates/controllers/controller-test.go (**Probably just add
m.IfExistsAction = OverwriteFile
into this template is enough so that we will re-write the controller test if any be scaffolded already)
So, what do you think?
Could you change it accordingly?
Hi, @camilamacedo86, thanks for the details. Actually, in ginkgo v1, the BeforeSuite and AfterSuite can run successfully, so users can test the default case(CRDs, Controllers, and Webhooks can be installed successfully, just as what BeforeSuite does) without any changes just running |
Can you please describe what problem and when are you facing it? All works fine. So, can you pelase raise an issue with:
|
Thanks for the advice, I just opened an issue that can reproduce the bug. |
6d4ffcc
to
8ac2e81
Compare
Hi, @camilamacedo86 Could you please take another review? Thanks. |
9493187
to
f04e5a8
Compare
@dashanji one thing that I noticed is common that there is a |
docs/book/src/cronjob-tutorial/testdata/project/api/v1/cronjob_webhook_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 🥇
Just a few nits:
- We should not scaffold a code that is not valid. So, for the webhooks under the test add a TODO(user): Add your logic here as we do for the reconcile and it is fine
- We should not change the go/v3 since it is deprecated. Can you please revert the changes and re-run make generate?
- The title of the PR we use to generate the changelog, I will update it accordingly to try to help out but you can check more about it in the CONTRIBUTION.md guide
After the changes please ensure that we have only 1 commit as of now so that we can get this one merged. You can squash the commits.
/ok-to-test |
Hi, @Sajiyah-Salat. A possible way is to debugging the The webhook test is generated here and its logic code is here. The controller test is generated here and its logic code is here. FYI. |
Make sense to me. |
54748ad
to
e0507cb
Compare
…test are no longer ignored. Signed-off-by: dashanji <dashanjic@gmail.com>
e0507cb
to
fc064f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shows great for me
I am approving
But would be nice see if we get a second reviewer as well
Let's try, if nobody be able to do this review I can either lgtm after a while
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@varshaprasad96 @everettraven @Kavinjsir
It seems OK for me.
Since it is open for a while I will /lgtm
otherwise the author might need to rebase this one
However, if you have suggestions/objections and etc we can always address in a follow up.
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, dashanji, Kavinjsir 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:
Approvers can indicate their approval by writing |
How to reproduce?
Add any valid ginkgo assert such as
Expect(1).To(Equal(2))
to the BeforeSuite and AfterSuite. As a result, the test should fail, but it succeeds actually.What do these changes do?
Add the unit test template for webhook and controllers so that suite_test are no longer ignored.
Fixes #3646