Add complete interpreter test coverage for Policy #7146
Conversation
Signed-off-by: Abhay349 <[email protected]>
karmada-bot
added
the
kind/cleanup
|
[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 |
karmada-bot
added
the
size/L
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 significantly enhances the test coverage for the Kyverno Policy resource interpreter. By introducing a comprehensive suite of unit tests for status aggregation, interpretation, and health checks, it aims to improve the reliability and robustness of Kyverno's policy status handling across different clusters. These additions ensure that future changes or updates to the interpreter logic maintain expected behavior and prevent regressions. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive test coverage for the Kyverno Policy resource interpreter. It introduces new test files and cases for AggregateStatus, InterpretHealth, and InterpretStatus operations, ensuring the Lua-based customization logic is well-tested against various scenarios, including edge cases like nil statuses and different condition reasons. The changes are clear, correct, and significantly improve the robustness of the interpreter. Excellent work!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7146 +/- ##
==========================================
- Coverage 46.56% 46.53% -0.03%
==========================================
Files 700 700
Lines 48103 48136 +33
==========================================
+ Hits 22398 22399 +1
- Misses 24025 24050 +25
- Partials 1680 1687 +7
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:
|
|
/assign |
Hi @Abhay349, this info can be updated to: As it is currently written, when a PR is merged, the associated issue will also be closed by GitHub. This is appropriate for individual issues, but for umbrella issues, a single PR often resolves only a subset of the problems in most cases. |
|
ya we still get one pr to track |
@XiShanYongYe-Chang Thanks, I will this in mind from next times. |
| # test case for aggregating status of Policy | ||
| # case1. Policy with two status items | ||
| # case2. Policy with different Ready reasons | ||
| # case3. Policy with nil statusItems |
There was a problem hiding this comment.
conseder add a mixed test that a certain statusItem has no status or its status is empty.
| verifyimages: 0 | ||
| --- | ||
| name: "Policy with nil status" | ||
| description: "InterprettStatus should return empty status when status is nil" |
| # test case for interpreting status of Policy | ||
| # case1. Policy: interpret status test | ||
| # case2. Policy with nil status | ||
| # case3. Policy with only ready field |
There was a problem hiding this comment.
Besides Policy with only ready field we can also add only conditions and only autogen case
| # case1. Policy with status.ready = true | ||
| # case2. Policy with status.ready false | ||
| # case3. Policy with Ready=True and Succeeded condition | ||
| # case4. Policy with Ready=True but non-succeeded reason |
There was a problem hiding this comment.
nit: The test case here covers "there is a Ready condition but the reason is incorrect"; consider adding a test to cover "there is no Ready condition at all" (status.conditions is not empty, but there is no type=Ready).
5 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR adds comprehensive test coverage for the Policy resource interpreter customization.
Specifically, it:
• Adds unit tests for AggregateStatus, InterpretStatus, and InterpretHealth
• Aligns test expectations with the actual aggregation behavior
These tests improve confidence in Kyverno Policy status handling across clusters and prevent future regressions.
Which issue(s) this PR fixes:
Fixes part of #6952
Special notes for your reviewer:
• Tests are written to strictly follow the existing Lua aggregation logic
• All cases are verified using:
go test ./pkg/resourceinterpreter/default/thirdpartyDoes this PR introduce a user-facing change?:
None