-
Notifications
You must be signed in to change notification settings - Fork 1k
Add InterpretReplica test case for CloneSet #7019
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Abhay349, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on bolstering the testing framework for Kruise CloneSet by introducing a dedicated test case. The primary goal is to validate the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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.
Code Review
This pull request adds a test case for the InterpretReplica operation on CloneSet resources, which is a good addition for test coverage. The change correctly asserts the replica count. My feedback focuses on making this new test case more comprehensive by also validating the resource requirements, which would lead to a more robust test.
| output: | ||
| replica: 4 |
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.
While this test correctly validates the replica count, it could be more comprehensive. The GetReplicas Lua function in customizations.yaml for CloneSet returns both replica and requirement. This test only validates the replica.
To improve test coverage, please consider also validating the requirement output. This would ensure that kube.accuratePodRequirements(obj.spec.template) is correctly interpreting fields like affinity and tolerations from the CloneSet's pod template.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7019 +/- ##
==========================================
+ Coverage 46.60% 46.62% +0.02%
==========================================
Files 699 699
Lines 48182 48182
==========================================
+ Hits 22453 22463 +10
+ Misses 24032 24024 -8
+ Partials 1697 1695 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8dac3c1 to
8542cc6
Compare
|
The failing This PR only adds a test case for All relevant unit tests and validation checks are passing locally and in CI. |
|
Hi @Abhay349, One of the testing objectives we aim to achieve is the ability to design multiple test cases that cover the logic under test, similar to writing unit tests. This can be accomplished using orthogonal testing methods. You can refer to #6991 and #7012 (comment) |
|
Hi @XiShanYongYe-Chang, thanks a lot for the clarification and for sharing the references to #6991 and #7012. That really helped me understand the expected direction better. I now understand that the aim of this umbrella issue is to design orthogonal test cases, where each interpreter operation is tested independently, similar to how we write unit tests. In this PR, I have focused only on InterpretReplica for CloneSet as an initial incremental step. I added explicit output verification in interpretreplica-test.yaml to validate the replica interpretation logic in isolation and align it with the expected testing structure. Based on your guidance, I’m planning to continue adding output verification for the remaining CloneSet operations as follow-up work. Please let me know if this incremental approach is okay, and I’ll proceed accordingly, either by extending this PR or by raising separate follow-up PRs for the remaining test cases, as you prefer. Thanks again for the guidance 🙏 |
|
Could you directly modify this PR by completing all the clonset tests? |
Signed-off-by: Abhay349 <[email protected]>
8542cc6 to
a9ac1a4
Compare
Signed-off-by: Abhay349 <[email protected]>
a9ac1a4 to
40ca714
Compare
|
I’ve updated this PR to complete all CloneSet interpreter tests with output verification:
Please let me know if any further changes are needed. |
|
Thanks, the number of test files is as expected. For each test file, or rather each operation, it is still not enough. We need to add sufficient test cases to cover the logic being tested. Therefore, a test file should ideally contain multiple test cases. |
|
Thanks for the clarification, that makes sense. I’ll proceed accordingly once confirmed. |
|
Lines 10 to 17 in 9ba7dc0
The number of test cases is not a fixed requirement; it only needs to cover the logic being tested. For example, in the case of Clonset's replicaResource, since there is no if branch, just one test case is sufficient. |
|
I understand that the goal is not to hit a specific number of test cases, but to ensure the logic is fully covered. For operations like replicaResource, where the logic is straight-through with no conditional branches, a single test case is sufficient. For operations that do contain branching logic (for example InterpretHealth and AggregateStatus), I’ll add additional test cases to cover the different paths in the logic, rather than adding redundant cases elsewhere. I’ll update this PR accordingly to improve branch-level coverage where it’s meaningful. |
What this PR does
Adds a test case for
InterpretReplicaofCloneSetunder third-party resource customizations.Why we need it
Notes
InterpretReplicafor CloneSet returns a scalar replica count (int64),consistent with existing interpreter behavior.