add unit test coverage for pkg/util/node and Remove duplicate testcases for func TestGetNodeHostIPs #110825

zhoumingcheng · 2022-06-28T08:11:55Z

Signed-off-by: zhoumingcheng zhoumingcheng@beyondcent.com

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Improve test coverage

Run test coverage command:

go test -cover ./pkg/util/node/

Befor:

ok      k8s.io/kubernetes/pkg/util/node/     0.019s  coverage: 60.3% of statements

After:

ok      k8s.io/kubernetes/pkg/util/node/   0.022s  coverage: 67.2% of statements

Remove duplicate testcases for func TestGetNodeHostIPs

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Thank you！

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Signed-off-by: zhoumingcheng <zhoumingcheng@beyondcent.com>

k8s-ci-robot · 2022-06-28T08:12:03Z

@zhoumingcheng: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2022-06-28T08:12:03Z

Hi @zhoumingcheng. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

pkg/util/node/node_test.go

aojea · 2022-06-28T08:24:34Z

/ok-to-test
/sig-node

Remove duplicate testcases for func TestGetNodeHostIPs Signed-off-by: zhoumingcheng <zhoumingcheng@beyondcent.com>

zhoumingcheng · 2022-07-05T01:40:43Z

/assign @thockin
please take a look, thanks!

thockin · 2022-07-05T21:42:55Z

pkg/util/node/node_test.go

+					t.Error("create node error")
+				}
+			}
+			result := GetNodeIP(kubeClientSet, test.nodeName)


Tell me if I am misunderstanding:

Testing through GetNodeIP() seems to give coverage of the wait.ExponentialBackoff() logic but ultimately calls GetNodeHostIP(). This test doesn't actually exercise any of the retry logic, so you're moving test cases around to score coverage "points" but it's bogus coverage.

I think I'd rather see all the test cases in the GetNodeHostIPs() (plural) test, with a subset of those in a test for GetNodeHostIP() (singular). This function (GetNodeIP()) seems to only be called from 2 places and I think it would be better to move this function to be internal to both. "A little copy is better than a little dep".

Thanks for the suggestion, I did notice that GetNodeIP() ends up calling GetNodeHostIP(). My GetNodeIP() test case and TestGetNodeHostIPs() both cover GetNodeHostIP(). I wonder if it's possible to cover wait.ExponentialBackoff() over GetNodeHostIP() directly via TestGetNodeIP(). So I removed TestGetNodeHostIPs. Not sure if this is possible?

I think this moves in the wrong direction. Tests should focus on the lowest level possible.

I'd set this up as a comprehensive test of GetNodeHostIPs() - since that seems to be where the real work is happening.

Testing GetNodeIP() (if done at all) should focus on the logic that is unique to that function, which is the backoff (which is mostly boilerplate and does not really belong in this utils lib in the first place).

I'm sorry, with all your replies to me, I'm a little bit confused about what you mean here. Do I wish I could move the tests for GetNodeIP() to the called function directory, or should the tests be concentrated at the lowest level possible. If it is the second, I will remove the test cases that are repeated with TestGetNodeHostIPs() in TestGetNodeIP(). Then TestGetNodeIP() will only focus on testing GetNodeIP(). Not sure if I misunderstood what you meant if I changed it like this. Or should I move the test of GetNodeIP() to the corresponding test function of the called function, such as pkg/util/node/node_test.go Test_detectNodeIP(). Sorry to bother you.

thockin · 2022-07-07T02:11:04Z

Don't bother to test GetNodeIP() until the lower level functions have complete, full coverage.

…

On Wed, Jul 6, 2022, 7:03 PM zhoumingcheng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/util/node/node_test.go <#110825 (comment)> : > + {Type: v1.NodeExternalIP, Address: "4.3.2.1"}, + {Type: v1.NodeExternalIP, Address: "4.3.2.2"}, + }}, + }, + expect: netutils.ParseIPSloppy("a:b::c:d"), + }, + } + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + kubeClientSet := kubeclientfake.NewSimpleClientset() + if test.node != nil { + if _, err := kubeClientSet.CoreV1().Nodes().Create(context.Background(), test.node, metav1.CreateOptions{}); err != nil { + t.Error("create node error") + } + } + result := GetNodeIP(kubeClientSet, test.nodeName) I'm sorry, with all your replies to me, I'm a little bit confused about what you mean here. Do I wish I could move the tests for GetNodeIP() to the called function directory, or should the tests be concentrated at the lowest level possible. If it is the second, I will remove the test cases that are repeated with TestGetNodeHostIPs() in TestGetNodeIP(). Then TestGetNodeIP() will only focus on testing GetNodeIP(). Not sure if I misunderstood what you meant if I changed it like this. Or should I move the test of GetNodeIP() to the corresponding test function of the called function, such as pkg/util/node/node_test.go Test_detectNodeIP(). Sorry to bother you. — Reply to this email directly, view it on GitHub <#110825 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVASYAJFSJKLSZJI35LVSY3HFANCNFSM52BHMHRQ> . You are receiving this because you were assigned.Message ID: ***@***.***>

Signed-off-by: zhoumingcheng <zhoumingcheng@beyondcent.com>

zhoumingcheng · 2022-07-07T03:18:59Z

/retest

zhoumingcheng · 2022-07-07T04:23:33Z

Don't bother to test GetNodeIP() until the lower level functions have complete, full coverage.
…
On Wed, Jul 6, 2022, 7:03 PM zhoumingcheng @.> wrote: @.* commented on this pull request. ------------------------------ In pkg/util/node/node_test.go <#110825 (comment)> : > + {Type: v1.NodeExternalIP, Address: "4.3.2.1"}, + {Type: v1.NodeExternalIP, Address: "4.3.2.2"}, + }}, + }, + expect: netutils.ParseIPSloppy("a🅱️:c:d"), + }, + } + for _, test := range testCases { + t.Run(test.name, func(t testing.T) { + kubeClientSet := kubeclientfake.NewSimpleClientset() + if test.node != nil { + if _, err := kubeClientSet.CoreV1().Nodes().Create(context.Background(), test.node, metav1.CreateOptions{}); err != nil { + t.Error("create node error") + } + } + result := GetNodeIP(kubeClientSet, test.nodeName) I'm sorry, with all your replies to me, I'm a little bit confused about what you mean here. Do I wish I could move the tests for GetNodeIP() to the called function directory, or should the tests be concentrated at the lowest level possible. If it is the second, I will remove the test cases that are repeated with TestGetNodeHostIPs() in TestGetNodeIP(). Then TestGetNodeIP() will only focus on testing GetNodeIP(). Not sure if I misunderstood what you meant if I changed it like this. Or should I move the test of GetNodeIP() to the corresponding test function of the called function, such as pkg/util/node/node_test.go Test_detectNodeIP(). Sorry to bother you. — Reply to this email directly, view it on GitHub <#110825 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABKWAVASYAJFSJKLSZJI35LVSY3HFANCNFSM52BHMHRQ . You are receiving this because you were assigned.Message ID: @.**>

Thanks for your suggestion, I have removed TestGetNodeIP() and removed the duplicated test cases in TestGetNodeHostIPs().

thockin · 2022-07-08T23:03:18Z

Thanks!

/lgtm
/approve

k8s-ci-robot · 2022-07-08T23:03:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin, zhoumingcheng

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:

~~pkg/OWNERS~~ [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pacoxu · 2022-07-11T01:13:32Z

/sig node

zhoumingcheng · 2022-07-11T02:59:01Z

/retest

add unit test coverage for pkg/util/node

540ec51

Signed-off-by: zhoumingcheng <zhoumingcheng@beyondcent.com>

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 28, 2022

k8s-ci-robot requested review from dchen1107 and thockin June 28, 2022 08:13

aojea reviewed Jun 28, 2022

View reviewed changes

pkg/util/node/node_test.go Outdated Show resolved Hide resolved

aojea reviewed Jun 28, 2022

View reviewed changes

pkg/util/node/node_test.go Outdated Show resolved Hide resolved

aojea reviewed Jun 28, 2022

View reviewed changes

pkg/util/node/node_test.go Outdated Show resolved Hide resolved

aojea reviewed Jun 28, 2022

View reviewed changes

pkg/util/node/node_test.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 28, 2022

update unit test for pkg/util/node

8dfb7af

Remove duplicate testcases for func TestGetNodeHostIPs Signed-off-by: zhoumingcheng <zhoumingcheng@beyondcent.com>

k8s-ci-robot assigned thockin Jul 5, 2022

thockin reviewed Jul 5, 2022

View reviewed changes

zhoumingcheng force-pushed the master-unit-v4 branch from 77fe3e5 to 8dfb7af Compare July 7, 2022 02:44

update test func for pkg/util/node

673930e

Signed-off-by: zhoumingcheng <zhoumingcheng@beyondcent.com>

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2022

zhoumingcheng changed the title ~~add unit test coverage for pkg/util/node~~ add unit test coverage for pkg/util/node and Remove duplicate testcases for func TestGetNodeHostIPs Jul 7, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2022

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 11, 2022

k8s-ci-robot merged commit a3f7776 into kubernetes:master Jul 11, 2022

k8s-ci-robot added this to the v1.25 milestone Jul 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add unit test coverage for pkg/util/node and Remove duplicate testcases for func TestGetNodeHostIPs #110825

add unit test coverage for pkg/util/node and Remove duplicate testcases for func TestGetNodeHostIPs #110825

zhoumingcheng commented Jun 28, 2022 •

edited

Loading

k8s-ci-robot commented Jun 28, 2022

k8s-ci-robot commented Jun 28, 2022

aojea commented Jun 28, 2022

zhoumingcheng commented Jul 5, 2022

thockin Jul 5, 2022

zhoumingcheng Jul 6, 2022

thockin Jul 6, 2022

zhoumingcheng Jul 7, 2022

thockin commented Jul 7, 2022 via email

zhoumingcheng commented Jul 7, 2022

zhoumingcheng commented Jul 7, 2022

thockin commented Jul 8, 2022

k8s-ci-robot commented Jul 8, 2022

pacoxu commented Jul 11, 2022

zhoumingcheng commented Jul 11, 2022

add unit test coverage for pkg/util/node and Remove duplicate testcases for func TestGetNodeHostIPs #110825

add unit test coverage for pkg/util/node and Remove duplicate testcases for func TestGetNodeHostIPs #110825

Conversation

zhoumingcheng commented Jun 28, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR 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.:

k8s-ci-robot commented Jun 28, 2022

k8s-ci-robot commented Jun 28, 2022

aojea commented Jun 28, 2022

zhoumingcheng commented Jul 5, 2022

thockin Jul 5, 2022

Choose a reason for hiding this comment

zhoumingcheng Jul 6, 2022

Choose a reason for hiding this comment

thockin Jul 6, 2022

Choose a reason for hiding this comment

zhoumingcheng Jul 7, 2022

Choose a reason for hiding this comment

thockin commented Jul 7, 2022 via email

zhoumingcheng commented Jul 7, 2022

zhoumingcheng commented Jul 7, 2022

thockin commented Jul 8, 2022

k8s-ci-robot commented Jul 8, 2022

pacoxu commented Jul 11, 2022

zhoumingcheng commented Jul 11, 2022

zhoumingcheng commented Jun 28, 2022 •

edited

Loading