add some unit tests #8960

zhouhao3 · 2020-04-23T06:39:29Z

Signed-off-by: Zhou Hao [email protected]

k8s-ci-robot · 2020-04-23T06:39:37Z

Hi @q384566678. 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.

zhouhao3 · 2020-04-23T06:43:32Z

/assign @granular-ryanbonham

hakman · 2020-04-23T06:53:39Z

/ok-to-test

zhouhao3 · 2020-05-12T05:53:47Z

@olemarkus PTAL

olemarkus · 2020-05-12T07:30:11Z

upup/pkg/fi/utils/sanitize_test.go

+		},
+		{
+			s:        "~/test",
+			expected: homedir.HomeDir() + "/test",


This test is pretty much the implementation of ExpandPath. I would change this to a hardcoded string.

Because the HomeDir obtained in different test environments is different, I think it is impossible to write a definite string.

The only relevant environment is linux. you can skip if the environment is something else. If not this test is rather meaningless.

Getting the value of homedir.HomeDir () in a linux environment is also different, so I will delete this test. Thanks!

geojaz

@q384566678 Thanks for putting this together! I'm +1 on the effort to improve our unit test coverage.

In general, I think we should make the code work harder and really exercise it. :) If you have questions, happy to clarify. thanks

geojaz · 2020-05-12T07:28:26Z

pkg/model/defaults/volumes_test.go

+	"k8s.io/kops/pkg/apis/kops"
+)
+
+func TestDefaultInstanceGroupVolumeSize(t *testing.T) {


This test feels like it creates maintenance work rather than unit testing "kops logic". That is to say, DefaultInstanceGroupVolumeSize basically just returns different constants with a switch.

IMO, changes that would break this test are likely to be intentional changes, so my vote would be to remove volumes_test.go.

geojaz · 2020-05-12T07:30:27Z

upup/pkg/fi/utils/equals_test.go

+	"testing"
+)
+
+func TestStringSlicesEqual(t *testing.T) {


FYI: I think this is a much more useful unit test than the volumes_test.

geojaz · 2020-05-12T07:30:43Z

upup/pkg/fi/utils/hash_test.go

@@ -0,0 +1,40 @@
+/*
+Copyright 2019 The Kubernetes Authors.


I think these should be bumped to 2020 now?

geojaz · 2020-05-12T07:33:32Z

upup/pkg/fi/utils/hash_test.go

+		expectedStr string
+	}{
+		{
+			s:           "test",


I think this is also a fairly useful test... could you add some more spicy or exciting edge cases? For these relatively simple functions, I would prefer to exercise the hard cases and assume the libraries generally do what they're supposed to.

geojaz · 2020-05-12T07:34:40Z

upup/pkg/fi/utils/sanitize_test.go

+		expected string
+	}{
+		{
+			s:        "test",


Again- are there any cases that better exercise the edges than these? These ones seem very easy :)

Thanks for your suggestions, I have updated the relevant tests. PTAL.

Signed-off-by: Zhou Hao <[email protected]>

olemarkus · 2020-05-12T07:57:28Z

pkg/model/defaults/volumes_test.go

+		},
+		{
+			role:     "Node2",
+			expected: -1,


@geojaz this particular one could perhaps have some value. I suppose we want to preserve -1 being the return value for unknown roles. The above could perhaps just test for positive integers instead.

If we were to add another role, it would at least be nice to see this test failing.

Great point +1 @olemarkus

Signed-off-by: Zhou Hao <[email protected]>

geojaz

/approve
/lgtm
thanks @q384566678

k8s-ci-robot · 2020-05-16T21:57:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geojaz, q384566678

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:

~~OWNERS~~ [geojaz]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 23, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 23, 2020

k8s-ci-robot requested review from joshbranham and robinpercy April 23, 2020 06:39

k8s-ci-robot assigned granular-ryanbonham Apr 23, 2020

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 Apr 23, 2020

zhouhao3 force-pushed the add-ut branch 2 times, most recently from d9baa8f to 0e9af4a Compare April 23, 2020 07:04

olemarkus reviewed May 12, 2020

View reviewed changes

geojaz reviewed May 12, 2020

View reviewed changes

Zhou Hao added 2 commits May 12, 2020 15:55

equals: add UT for StringSlicesEqual

2d6e814

Signed-off-by: Zhou Hao <[email protected]>

equals: add UT for StringSlicesEqualIgnoreOrder

e608e4d

Signed-off-by: Zhou Hao <[email protected]>

olemarkus reviewed May 12, 2020

View reviewed changes

sanitize: add UT for SanitizeString

0b79c41

Signed-off-by: Zhou Hao <[email protected]>

zhouhao3 force-pushed the add-ut branch 2 times, most recently from 51f8d6c to cef2975 Compare May 12, 2020 09:07

Zhou Hao added 2 commits May 12, 2020 17:26

hash: add UT for HashString

599ed6d

Signed-off-by: Zhou Hao <[email protected]>

model: add UT for DefaultInstanceGroupVolumeSize

1a1be5f

Signed-off-by: Zhou Hao <[email protected]>

zhouhao3 force-pushed the add-ut branch from cef2975 to 1a1be5f Compare May 12, 2020 09:26

geojaz approved these changes May 16, 2020

View reviewed changes

k8s-ci-robot assigned geojaz May 16, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2020

k8s-ci-robot merged commit 66b5322 into kubernetes:master May 16, 2020

k8s-ci-robot added this to the v1.18 milestone May 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add some unit tests #8960

add some unit tests #8960

zhouhao3 commented Apr 23, 2020

k8s-ci-robot commented Apr 23, 2020

zhouhao3 commented Apr 23, 2020

hakman commented Apr 23, 2020

zhouhao3 commented May 12, 2020

olemarkus May 12, 2020

zhouhao3 May 12, 2020

olemarkus May 12, 2020

zhouhao3 May 12, 2020

geojaz left a comment

geojaz May 12, 2020

zhouhao3 May 12, 2020

geojaz May 12, 2020

geojaz May 12, 2020

geojaz May 12, 2020

geojaz May 12, 2020

zhouhao3 May 12, 2020 •

edited

Loading

olemarkus May 12, 2020

olemarkus May 12, 2020

geojaz May 12, 2020

geojaz left a comment

k8s-ci-robot commented May 16, 2020

add some unit tests #8960

add some unit tests #8960

Conversation

zhouhao3 commented Apr 23, 2020

k8s-ci-robot commented Apr 23, 2020

zhouhao3 commented Apr 23, 2020

hakman commented Apr 23, 2020

zhouhao3 commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geojaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhouhao3 May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geojaz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 16, 2020

zhouhao3 May 12, 2020 •

edited

Loading