feat: add unit test for createLBStatus #2408

sakshi-1505 · 2023-10-09T08:11:52Z

What this PR does / why we need it:

Which issue this PR fixes(if applicable):
refers #2400

Special notes for reviewers:

Release note:

NONE

k8s-ci-robot · 2023-10-09T08:12:01Z

Hi @sakshi-1505. 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.

dulek · 2023-10-09T17:14:19Z

/ok-to-test

dulek · 2023-10-09T17:18:06Z

Looks good I think, just needs a rebase.

sakshi-1505 · 2023-10-10T13:00:03Z

/retest

sakshi-1505 · 2023-10-10T13:01:34Z

@dulek please take a look now

sakshi-1505 · 2023-10-11T13:35:09Z

bump @dulek @pierreprinetti

dulek · 2023-10-12T16:16:00Z

Please squash the commits before this can be merged.

dulek · 2023-10-13T15:25:04Z

/approve

I'm leaving approval, will lgtm once it's squashed.

sakshi-1505 · 2023-10-13T15:26:49Z

Sorry for the delay here. Got busy in my studies, I will get this done asap

…

On Fri, 13 Oct 2023 at 20:55, Kubernetes Prow Robot < ***@***.***> wrote: [APPROVALNOTIFIER] This PR is *APPROVED* This pull-request has been approved by: *dulek <#2408 (comment)>* The full list of commands accepted by this bot can be found here <https://go.k8s.io/bot-commands?repo=kubernetes%2Fcloud-provider-openstack> . The pull request process is described here <https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process> Needs approval from an approver in each of these files: - pkg/openstack/OWNERS <https://github.com/kubernetes/cloud-provider-openstack/blob/master/pkg/openstack/OWNERS> [dulek] Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment — Reply to this email directly, view it on GitHub <#2408 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BDCZVU44OAU4X3BC5ACUOJDX7FMOJAVCNFSM6AAAAAA5YMYVCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRRG4YDCOBSGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

k8s-ci-robot · 2023-10-15T15:46:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

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/openstack/OWNERS~~ [dulek]

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

sakshi-1505 · 2023-10-15T15:47:34Z

@pierreprinetti @dulek Can you please approve this now? I did the rebase, there is addition of another test in this PR because the authors are adding test in b/w of the file rather than appending things at the end which is the major cause of all this conflicts & errors.

pierreprinetti

Your test is correct, as in: it validates the logic that is currently in place.

However, I think you could walk an extra mile and make it more robust.

Essentially, your test validates two fields of Status: Hostname and IP. Now, imagine that tomorrow we need to add another field: for example (and this is totally made up), maximumConcurrentConnections, that defaults to 1000.

As soon as we add that new field, your test fails, because you are comparing for equality an entire corev1.LoadBalancerStatus object.

One way to make your test future-proof is to only check the properties you are actually testing for. Instead of comparing the entire object, you could check that the single Hostname and IP properties match what you need them to. Do you want to try that?

pierreprinetti · 2023-10-16T09:55:31Z

pkg/openstack/loadbalancer_test.go

@@ -824,7 +937,6 @@ func Test_buildPoolCreateOpt(t *testing.T) {
 		})
 	}
 }
-


Woops! Please leave this blank line in :)

pierreprinetti · 2023-10-16T10:32:10Z

pkg/openstack/loadbalancer_test.go

+			},
+		},
+		{
+			name: "it should return fakehostname if proxyProtocal & IngressHostName is enabled without svc annotation",


little typo:

Suggested change

name: "it should return fakehostname if proxyProtocal & IngressHostName is enabled without svc annotation",

name: "it should return fakehostname if proxyProtocol & IngressHostName is enabled without svc annotation",

pierreprinetti · 2023-10-16T10:38:54Z

pkg/openstack/loadbalancer_test.go

+			fields: fields{
+				LoadBalancer: LoadBalancer{
+					opts: LoadBalancerOpts{
+						EnableIngressHostname: false,
+						IngressHostnameSuffix: "test",
+					},
+				},
+			},


Is this setup required for the test case? Why did you choose to include it?

Actually, this is included to make sure that if at some later time we change the way of getHostFromSVC annotation the function doesn't go into this code block https://github.com/kubernetes/cloud-provider-openstack/blob/60ad70c0b53d545f323df71b4603bfc8a98dd2df/pkg/openstack/loadbalancer.go#L1881C1-L1882C1. Moreover this is to ensure the declarative behaviour of the unit test as they should be.

pierreprinetti · 2023-10-16T15:47:36Z

pkg/openstack/loadbalancer_test.go

+				LoadBalancer: tt.fields.LoadBalancer,
+			}
+			if tt.want.HostName != "" {
+				assert.Equal(t, tt.want.HostName, lbaas.createLoadBalancerStatus(tt.args.service, tt.args.svcConf, tt.args.addr).Ingress[0].Hostname)


I'd call the function under test just once, save the result in a variable, and assert the variable. I know it's not a big deal in this case, but calling it once per assertion can be a problematic pattern when the function under test is more expensive.

pierreprinetti · 2023-10-16T15:48:03Z

pkg/openstack/loadbalancer_test.go

+			lbaas := &LbaasV2{
+				LoadBalancer: tt.fields.LoadBalancer,
+			}
+			if tt.want.HostName != "" {


What happens if you remove this conditional?

I don't know why I believed that the want.result would be a pointer where it's a string type itself. I removed both conditional

pierreprinetti · 2023-10-16T15:48:15Z

pkg/openstack/loadbalancer_test.go

+			if tt.want.HostName != "" {
+				assert.Equal(t, tt.want.HostName, lbaas.createLoadBalancerStatus(tt.args.service, tt.args.svcConf, tt.args.addr).Ingress[0].Hostname)
+			}
+			if tt.want.IPAddress != "" {


What happens if you remove this conditional?

#2408 (comment)

sakshi-1505 · 2023-10-16T17:58:31Z

Please take a look again @pierreprinetti

pierreprinetti

Looks good to me! Please apply the little fix in the inline comments, then squash, and we're ready to merge. Thank you!

pkg/openstack/loadbalancer_test.go

sakshi-1505 · 2023-10-17T14:41:02Z

PLease review again @pierreprinetti , I have made all requested changes

pierreprinetti · 2023-10-17T14:46:08Z

/lgtm
Thank you!

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 9, 2023

k8s-ci-robot requested review from anguslees and FengyunPan2 October 9, 2023 08:11

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 9, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 9, 2023

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 Oct 9, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2023

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2023

sakshi-1505 force-pushed the createLBStatus branch from c66e151 to 2da2817 Compare October 15, 2023 15:41

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2023

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2023

pierreprinetti suggested changes Oct 16, 2023

View reviewed changes

sakshi-1505 force-pushed the createLBStatus branch from 3deab8f to b823e48 Compare October 16, 2023 14:51

pierreprinetti reviewed Oct 16, 2023

View reviewed changes

sakshi-1505 requested a review from pierreprinetti October 16, 2023 17:49

pierreprinetti reviewed Oct 17, 2023

View reviewed changes

pkg/openstack/loadbalancer_test.go Outdated Show resolved Hide resolved

feat: add unit test for createLBstatus

5f7262f

sakshi-1505 force-pushed the createLBStatus branch from 6cefdb3 to 5f7262f Compare October 17, 2023 14:40

k8s-ci-robot assigned pierreprinetti Oct 17, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2023

k8s-ci-robot merged commit 8f896e3 into kubernetes:master Oct 17, 2023
6 checks passed

mandre pushed a commit to shiftstack/cloud-provider-openstack that referenced this pull request Feb 6, 2024

feat: add unit test for createLBstatus (kubernetes#2408)

9cd245b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add unit test for createLBStatus #2408

feat: add unit test for createLBStatus #2408

sakshi-1505 commented Oct 9, 2023

k8s-ci-robot commented Oct 9, 2023

dulek commented Oct 9, 2023

dulek commented Oct 9, 2023

sakshi-1505 commented Oct 10, 2023

sakshi-1505 commented Oct 10, 2023

sakshi-1505 commented Oct 11, 2023

dulek commented Oct 12, 2023

dulek commented Oct 13, 2023

sakshi-1505 commented Oct 13, 2023 via email

k8s-ci-robot commented Oct 15, 2023

sakshi-1505 commented Oct 15, 2023

pierreprinetti left a comment

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

pierreprinetti Oct 16, 2023

sakshi-1505 Oct 16, 2023

sakshi-1505 commented Oct 16, 2023

pierreprinetti left a comment

sakshi-1505 commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023

@@ @@ -824,7 +937,6 @@ func Test_buildPoolCreateOpt(t *testing.T) { @@
               		})
               	}
               }

	name: "it should return fakehostname if proxyProtocal & IngressHostName is enabled without svc annotation",
	name: "it should return fakehostname if proxyProtocol & IngressHostName is enabled without svc annotation",

feat: add unit test for createLBStatus #2408

feat: add unit test for createLBStatus #2408

Conversation

sakshi-1505 commented Oct 9, 2023

k8s-ci-robot commented Oct 9, 2023

dulek commented Oct 9, 2023

dulek commented Oct 9, 2023

sakshi-1505 commented Oct 10, 2023

sakshi-1505 commented Oct 10, 2023

sakshi-1505 commented Oct 11, 2023

dulek commented Oct 12, 2023

dulek commented Oct 13, 2023

sakshi-1505 commented Oct 13, 2023 via email

k8s-ci-robot commented Oct 15, 2023

sakshi-1505 commented Oct 15, 2023

pierreprinetti 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

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

sakshi-1505 commented Oct 16, 2023

pierreprinetti left a comment

Choose a reason for hiding this comment

sakshi-1505 commented Oct 17, 2023

pierreprinetti commented Oct 17, 2023