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.

Sign up for GitHub

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

Jump to bottom

test: add pod controller unit test #490

Merged
merged 8 commits into from
Apr 25, 2024
Merged

test: add pod controller unit test #490

merged 8 commits into from
Apr 25, 2024

Conversation

googs1025
Copy link
Member

Added unit test for podcontroller
issue from #478

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 30, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @googs1025. 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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 30, 2024
@netlify Netlify
Copy link

netlify bot commented Mar 30, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-jobset canceled.

Name Link
🔨 Latest commit 9e7e98b
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-jobset/deploys/6629e21e51411500088a591a

@kannon92
Copy link
Contributor

/ok-to-test

@k8s-ci-robot 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 Mar 30, 2024
@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch 2 times, most recently from f7ff96a to 0760e9e Compare March 31, 2024 00:54
@googs1025
Copy link
Member Author

@danielvegamyhre /PTAL thank a lot!

@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch from 0760e9e to 2188080 Compare April 1, 2024 02:39
@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch 2 times, most recently from 6590dd1 to 8c470ad Compare April 3, 2024 01:08
@googs1025
Copy link
Member Author

@danielvegamyhre /PTAL I have made the necessary modifications. Please take the time to review them. thanks a lot.

@danielvegamyhre
Copy link
Contributor

Been busy lately but will review this as soon as I can

@googs1025 googs1025 mentioned this pull request Apr 5, 2024
Closed
@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch from 8c470ad to 7fb078f Compare April 6, 2024 00:18
danielvegamyhre
danielvegamyhre reviewed Apr 6, 2024
View reviewed changes
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/util/testing/wrappers.go Show resolved Hide resolved
@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch 2 times, most recently from 043d929 to 2598008 Compare April 23, 2024 01:36
@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch from 2598008 to cc22f12 Compare April 23, 2024 01:37
danielvegamyhre
danielvegamyhre reviewed Apr 23, 2024
View reviewed changes
pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
pkg/controllers/pod_controller_test.go Outdated
followerPodWrapper.Obj(),
},
wantPodsDeleted: []corev1.Pod{
wantPodWrapper.ResourceVersion("999").SetConditions([]corev1.PodCondition{{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remind me why we need to explicitly set the resourceVersion here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/fake/client.go#L285
I guess the reason for using a magic number here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, honestly I think we should just count the number of pod deletion calls made via the Delete() interceptor func. Needing to dive into the weeds of another code base to understand the unit tests isn't worth it for the marginal improvement of the precision of our tests.

Sorry this came up rather late in the review process, I hope you don't mind making the change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, it's also a learning experience for me, haha. I'll make the change later.

Copy link
Member Author

@googs1025 googs1025 Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already fixed @danielvegamyhre

pkg/controllers/pod_controller_test.go Outdated Show resolved Hide resolved
@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch 2 times, most recently from 839ec0c to 717d86d Compare April 24, 2024 04:09
@googs1025 googs1025 force-pushed the add_pod_controller_unit_test branch from 717d86d to 56ae233 Compare April 24, 2024 04:11
@danielvegamyhre
Copy link
Contributor

/lgtm
/approve

Thanks @googs1025 for improving our test coverage! This was a much needed improvement.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielvegamyhre, googs1025

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2024
@k8s-ci-robot k8s-ci-robot merged commit c986e5f into kubernetes-sigs:main Apr 25, 2024
12 checks passed
@googs1025
Copy link
Member Author

Finally merged! Haha, thank you for the review. It did take some time, but I also learned a lot from it. @danielvegamyhre

@googs1025 googs1025 mentioned this pull request Apr 28, 2024
Closed
11 tasks
@danielvegamyhre danielvegamyhre mentioned this pull request Aug 19, 2024
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dejanzele dejanzele dejanzele left review comments

@kannon92 kannon92 Awaiting requested review from kannon92

@danielvegamyhre danielvegamyhre Awaiting requested review from danielvegamyhre

Assignees

@danielvegamyhre danielvegamyhre

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@googs1025 @k8s-ci-robot @kannon92 @danielvegamyhre @dejanzele