Add some tests for pkg/scheduler
#5274
Conversation
karmada-bot
added
the
kind/flake
|
@varshith257: The label(s) In 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-sigs/prow repository. |
|
Welcome @varshith257! It looks like this is your first PR to karmada-io/karmada 🎉 |
karmada-bot
added
the
size/L
varshith257
changed the title
pkg/scheduler
|
@XiShanYongYe-Chang, I have added some tests to familiarize myself with the codebase and the coding style. Will incrementally add tests to make coverage of |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5274 +/- ##
==========================================
+ Coverage 29.83% 29.94% +0.11%
==========================================
Files 632 632
Lines 43936 43936
==========================================
+ Hits 13107 13156 +49
+ Misses 29871 29823 -48
+ Partials 958 957 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Thanks @varshith257 |
|
@XiShanYongYe-Chang Can you review this PR and get merged? It will also helps to me link it in cover letter and submit application :) I will push follow up prs gradually increase test coverage of pkg/scheduler as mentioned |
|
@XiShanYongYe-Chang PTAL |
pkg/scheduler/scheduler_test.go
Outdated
| disableSchedulerEstimatorInPullMode bool | ||
| expectedError bool | ||
| expectEstablishConnection bool | ||
| }{ |
There was a problem hiding this comment.
It seems that all cases have an error when getting the cluster object.
There was a problem hiding this comment.
Hi @anujagrawal699 would you like to give some advice on this case?
There was a problem hiding this comment.
Hello @XiShanYongYe-Chang I think the issue is that test is creating a cluster using karmadaClient, but the reconcileEstimatorConnection method is using a clusterLister to retrieve it. This can be a reason of an error when getting the cluster object. More cases like the cluster in push mode might help us determine it. There can be also other reasons of this errors. @varshith257
There was a problem hiding this comment.
I have tried though and debuged it. Maybe I can add this test in separate PR with fresh perspective of solving it. I will remove adding this test in this PR.
There was a problem hiding this comment.
I think the issue is that test is creating a cluster using karmadaClient, but the reconcileEstimatorConnection method is using a clusterLister to retrieve it.
@anujagrawal699 I see you wrote a test with similar logic and it passed, so wanted to see what you thought.
I have tried though and debuged it. Maybe I can add this test in separate PR with fresh perspective of solving it. I will remove adding this test in this PR.
Hi @varshith257 I agree with you.
There was a problem hiding this comment.
@XiShanYongYe-Chang In desheduler_test.go, the tests starts and waits for the informer cache to sync and also it allows the test operations to complete with timeout. Also, i don't think the test is successfully retrieving the cluster. _, err = karmadaClient.ClusterV1alpha1().Clusters().Create(context.TODO(), cluster, metav1.CreateOptions{}) is creating a cluster. I think retrieving it using the same clusterLister method could work, it goes like
retrievedCluster, err := scheduler.clusterLister.Get(clusterName)
if err != nil {
t.Fatalf("Failed to retrieve cluster: %v", err)
}
|
@XiShanYongYe-Chang Any guidance on it will be greatly appreciated. I have tested with cluster creation mock and expected to call the |
|
@XiShanYongYe-Chang Any review or guidance on this |
|
Lines in red are not covered by the test. You can find other tests to see if you can successfully get the resource from the lister. |
|
Hi @varshith257 how is it going now? By the way, would you like to review the ut tests submitted by others? It's not mandatory, you can do it as you wish, and it's an important contribution to the community. Thank you very much. (I highly encourage you to review each other. My perspective may be subjective, but you can discuss it fully and provide more valuable review comments.) |
|
Sure! |
|
/retest |
|
@varshith257: Cannot trigger testing until a trusted user reviews the PR and leaves an In 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-sigs/prow repository. |
|
@XiShanYongYe-Chang Can do /ok-to-test? I can't isolate why failing in e2e whiles changes done for UT? |
|
Hi @varshith257, can you help pull the latest code from master and rebase it? |
|
Sure! |
|
@XiShanYongYe-Chang Done! |
|
Hi @varshith257, can you help squash the commits into one? |
karmada-bot
added
the
do-not-merge/contains-merge-commits
karmada-bot
removed
the
do-not-merge/contains-merge-commits
|
@XiShanYongYe-Chang Done! |
|
Hi @varshith257 . Thanks for the PR. Try running |
Signed-off-by: Vamshi Maskuri <[email protected]> add review comments Signed-off-by: Vamshi Maskuri <[email protected]> fix lint
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: XiShanYongYe-Chang 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 |
karmada-bot
added
the
approved
What type of PR is this?
/kind test
/kind flake
What this PR does / why we need it:
Which issue(s) this PR fixes:
Part of #5235
Special notes for your reviewer:
No
Does this PR introduce a user-facing change?: