[Scheduler] refactor TestPriorityQueue_Update #122524
Conversation
k8s-ci-robot
added
release-note-none
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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
added
needs-triage
| // each test will call Update() with oldPod and newPod returned from it. | ||
| prepareFunc func(q *PriorityQueue) (oldPod, newPod *v1.Pod) | ||
| // validateFunc is the function called after the test calls Update(), to validate the pod is updated as expected. | ||
| validateFunc func(pInfo *framework.QueuedPodInfo) bool |
There was a problem hiding this comment.
Is it necessary to be a function? I mean, can we change it to wantUpdatedPod: *framework.QueuePodInfo or something?
There was a problem hiding this comment.
I think we can, I added wantUpdatedPod to the return value of prepareFunc, as we'll use the updatedPod for validate.
AxeZhan
force-pushed
the
testPQupdate
branch
2 times, most recently
from
14cb465
to
52f94ea
Compare
There was a problem hiding this comment.
One last from me, otherwise LGTM. Leave /approve to @alculquicondor.
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 1c1d4df3996c340f3eaa79489bb44834dc99d03e
|
| if diff := cmp.Diff(wantUpdatedPod, pInfo, cmpopts.IgnoreFields(framework.QueuedPodInfo{}, "Timestamp", "InitialAttemptTimestamp")); diff != "" { | ||
| t.Errorf("Unexpected updated pod diff (-want, +got): %s", diff) | ||
| } |
There was a problem hiding this comment.
Is this comparison that useful? What in the QueuedPodInfo is important to check?
Can we skip the fields that are not interesting so that we can build the wantQueuedPodInfo without the need of a function?
I don't particularly like the idea of prepare as it makes the test quite hard to read.
There was a problem hiding this comment.
Ideally, this comparison is to check if update action takes effect.
In this test, all the update action we do is for pod's annotations
updatedPod := medPriorityPodInfo.Pod.DeepCopy()
updatedPod.Annotations["foo"] = "test"
So I think maybe we can just check the newpod with the pod we got from queue?
if diff := cmp.Diff(newPod, pInfo.PodInfo.Pod); diff != ""
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, AxeZhan 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 |
k8s-ci-robot
added
the
approved
|
/retest |
|
ping @sanposhiho @alculquicondor |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7a10a9e19ec11aadcfbd6d5150d685e5a218b2cf
|
|
/retest |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This this separated from #122234 , for a better review.
#122234 (comment)
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: