New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add test for GetAPIServerVirtualIP #111596
Conversation
@muyangren2: 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. |
Hi @muyangren2. Thanks for your PR. I'm waiting for a kubernetes 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. |
b0e2ce0
to
d135793
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
d135793
to
a400b40
Compare
/assign @neolit123 |
if err == nil { | ||
t.Errorf("failed APIServerVirtualIP:\n\texpected error, but got no error") | ||
} | ||
} else if !rt.expectedErr && err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these branches can be
if (err != nil) != rt.expectedError {
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have modified as you suggested, thank you for your review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
code freeze for 1.25 is soon so not sure if this PR will make it.
a400b40
to
6c8482d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold
i think you can simplify the branches more.
t.Errorf("failed APIServerVirtualIP:\n\texpected error, but got no error") | ||
} | ||
} else if (err != nil) != rt.expectedErr { | ||
t.Errorf("failed APIServerVirtualIP:\n\texpected no error, but got: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just have one branch in case err does not match
"expected error x, got y"
if rt.expectedErr { | ||
if err == nil { | ||
t.Errorf("failed APIServerVirtualIP:\n\texpected error, but got no error") | ||
} | ||
} else if (err != nil) != rt.expectedErr { | ||
t.Errorf("failed APIServerVirtualIP:\n\texpected no error, but got: %v", err) | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if rt.expectedErr { | |
if err == nil { | |
t.Errorf("failed APIServerVirtualIP:\n\texpected error, but got no error") | |
} | |
} else if (err != nil) != rt.expectedErr { | |
t.Errorf("failed APIServerVirtualIP:\n\texpected no error, but got: %v", err) | |
} else { | |
if (err != nil) != rt.expectedErr { | |
t.Errorf("failed APIServerVirtualIP:\n\texpectedErr: %v, got: %v", rt.expectedErr, err) | |
} else { |
ee94516
to
7bd3c55
Compare
|
||
func TestGetAPIServerVirtualIP(t *testing.T) { | ||
var tests = []struct { | ||
name, svcSubnet, expectedIP string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm curious, what happens if you pass a subnet like?
"10.96.0.12/24", it should give 10.96.0.1, or it fails the subnet parsing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will get 10.96.0.1 ,i will add this case
modify modify modify modify modify
7bd3c55
to
4af5cc3
Compare
lgtm |
@neolit123 we have test freeze until Aug 10, this can still make it, is just adding tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/hold cancel
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: muyangren2, neolit123 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 |
/assign @dims @BenTheElder |
/milestone v1.25 @aojea i think if you want milestone privs a number of people will +1 that 👍 |
didn't know about it, where should I request those privs, is a PR somewhere? |
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Add UT
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.: