🌱 Added unit tests for state_test.go #9468
Conversation
k8s-ci-robot
added
area/testing
|
Welcome @razashahid107! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @razashahid107. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
abdurrehman107
changed the title
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| func TestMPIsUpgrading(t *testing.T) { | ||
| g := NewWithT(t) | ||
| scheme := runtime.NewScheme() | ||
| g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) |
There was a problem hiding this comment.
You will need to add expv1 to the scheme as well since MachinePools are part of that package instead of clusterv1.
g.Expect(expv1.AddToScheme(scheme)).To(Succeed())
| tests := []struct { | ||
| name string | ||
| mp *clusterv1.MachinePool | ||
| machines []*clusterv1.Machine |
There was a problem hiding this comment.
We will check the noderef's kubelet version instead of the machine's version. Thus, we'll need to add a list of *corev1.Node instead of *clusterv1.Machine. The logic for this is here: https://github.com/kubernetes-sigs/cluster-api/blob/main/internal/controllers/topology/cluster/scope/state.go#L174
| func TestMPUpgrading(t *testing.T) { | ||
| g := NewWithT(t) | ||
| scheme := runtime.NewScheme() | ||
| g.Expect(clusterv1.AddToScheme(scheme)).To(Succeed()) |
There was a problem hiding this comment.
You will need to add expv1 to the scheme here as well.
g.Expect(expv1.AddToScheme(scheme)).To(Succeed())
|
@razashahid107: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
|
@willie-yao Do you have some time to / do you want to take this one over? |
Yes I can take this over. I do want to check with @razashahid107 if he still wanted to work on this or needed help |
|
Hey @willie-yao and @sbueringer. I'm sorry I have not been able to get done with this. I understand this probably is a relatively easy task, but I keep getting lost with the preset functions that I have to work on before I get on with the actual tests. @sbueringer @willie-yao I was working on another PR for this test which did not get completed and I just ended up getting lost with it. I'll raise that PR tomorrow and I'll request some help from you. Do I proceed with this try or are we short on time? If thats the case @willie-yao can take over. If thats not the case I'd love to allow myself another attempt at this. |
|
No problem @razashahid107. You can give it another go if you'd like and please don't hesitate to reach out if you have any questions! I will take a look at your new PR whenever it is posted |
What this PR does / why we need it:
This PR adds tests for the new ClusterClass MachinePool implementation for the internal/controllers/topology/cluster/scope/state_test.go
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes part of #5991
/area testing