test(queue): add coverage for hierarchical queue controller propagation logic#5321
Conversation
Signed-off-by: ayuxsh009 <1raj.aayush@gmail.com>
|
@ayuxsh009: The label(s) DetailsIn response to this:
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. |
|
[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 |
|
Welcome @ayuxsh009! It looks like this is your first PR to volcano-sh/volcano 🎉 |
volcano-sh-bot
added
the
size/L
![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 introduces a comprehensive suite of unit tests for hierarchical queue management within the queue controller, covering parent updates, annotation handling, and state synchronization between parent and child queues. The reviewer provided feedback aimed at improving test robustness, specifically recommending that existing annotations be verified for preservation during updates and that test resource creation be made more consistent to avoid fragility against future implementation changes.
| } | ||
| result, err := c.updateQueueAnnotation(tc.queue, tc.annotationKey, tc.annotationValue) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, tc.expectValue, result.Annotations[tc.annotationKey]) |
There was a problem hiding this comment.
In the test case existing annotations get new key appended, the assertion only verifies the presence of the new annotation. It is recommended to also verify that existing annotations (e.g., unrelated-key) are preserved to ensure the update logic correctly merges annotations rather than accidentally overwriting the entire map.
| if tc.childInVCClient { | ||
| _, err := c.vcClient.SchedulingV1beta1().Queues().Create(context.TODO(), tc.child, metav1.CreateOptions{}) | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
The conditional creation of the child queue in the fake client based on tc.childInVCClient makes the test setup fragile. If the implementation of syncHierarchicalQueue is updated to perform any API interaction (such as logging an event or updating a different field) for cases where childInVCClient is currently false, the test will fail with a 'not found' error. It is more robust to always create the queue in the fake client and assert on the specific actions taken or the final state.
References
- Tests should be robust against implementation changes that do not affect the core logic being tested.
| if child.Status.State != schedulingv1beta1.QueueStateClosed && | ||
| child.Status.State != schedulingv1beta1.QueueStateClosing { | ||
| _, err := c.vcClient.SchedulingV1beta1().Queues().Create(context.TODO(), child, metav1.CreateOptions{}) | ||
| assert.NoError(t, err) | ||
| } |
There was a problem hiding this comment.
Similar to TestSyncHierarchicalQueue, this conditional creation of children in the fake client is fragile. It assumes that closeHierarchicalQueue will only attempt to patch queues that are not already in Closed or Closing states. If the logic changes to ensure consistency (e.g., ensuring the annotation is present even if already closed), this test will fail. Creating all children in the fake client provides a more realistic and robust test environment.
References
- Tests should be robust against implementation changes that do not affect the core logic being tested.
There was a problem hiding this comment.
Pull request overview
Adds direct unit test coverage for hierarchical queue controller propagation logic, exercising parent/child state transitions and annotation/parent defaulting behaviors in the queue controller.
Changes:
- Introduces a lightweight helper to inject
Queueobjects into the informer cache for lister-based code paths. - Adds table-driven test suites covering
syncHierarchicalQueue,openHierarchicalQueue,closeHierarchicalQueue,updateQueueParent,updateQueueAnnotation, and theupdateQueueevent handler behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // addQueueToStore adds a Queue to the controller's informer cache so that | ||
| // queueLister.Get / queueLister.List return it without needing a running informer. | ||
| func addQueueToStore(c *queuecontroller, q *schedulingv1beta1.Queue) { | ||
| _ = c.queueInformer.Informer().GetStore().Add(q) |
|
Hi @kingeasternsun , @wangyang0616 have a look on this |
PR Description
What this PR does
Adds unit test coverage for hierarchical queue controller actions in:
pkg/controllers/queue/queue_controller_action.gopkg/controllers/queue/queue_controller_handler.goThese paths previously had effectively no direct test coverage.
Why this PR is needed
The hierarchical queue controller logic is responsible for propagating
parent queue state changes to child queues and will be relied upon by the
upcoming
NamespaceQueuefunctionality.The covered functions include:
syncHierarchicalQueueopenHierarchicalQueuecloseHierarchicalQueueupdateQueueParentupdateQueueAnnotationupdateQueueevent handlerBefore this PR, the only indirect coverage came from
TestSyncQueue,which used a
"root"queue and bypassed most hierarchical logic paths.Changes made
addQueueToStorehelper to populate informer cache state during testswithout requiring a running informer.
TestUpdateQueueParentTestUpdateQueueAnnotationTestUpdateQueueHandlerTestSyncHierarchicalQueueTestOpenHierarchicalQueueTestCloseHierarchicalQueueTest plan
go test ./pkg/controllers/queue/... -vWhat type of PR is this?
/kind test
Which issue(s) this PR fixes
Fixes N/A
Special notes for reviewers
newFakeController()pattern and fake clients.tests lightweight and deterministic.
AI disclosure
Claude Code (claude-sonnet-4-6) was used only for initial assistance in identifying uncovered paths and generating a rough test scaffold.
The actual test scenarios, assertions, informer-store setup, edge-case handling, and final implementation were manually reviewed, expanded, modified, and validated locally. Significant manual work was done to refine the test logic, ensure realistic controller behavior coverage, and verify all cases with local test runs before submission.
Does this PR introduce a user-facing change?