Skip to content
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 ut for pkg/util/annotation #20

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jxs1211
Copy link

@jxs1211 jxs1211 commented Sep 15, 2022

Signed-off-by: xian-jie.shen [email protected]

Pre-Checklist

Description

add ut

New Behavior (screenshots if needed)

[root@dev testing]# go test annotation_test.go annotation.go -v -cover 
=== RUN   TestGetRegion
=== RUN   TestGetRegion/base
=== RUN   TestGetRegion/LabelZoneRegion
=== RUN   TestGetRegion/not_found
--- PASS: TestGetRegion (0.00s)
    --- PASS: TestGetRegion/base (0.00s)
    --- PASS: TestGetRegion/LabelZoneRegion (0.00s)
    --- PASS: TestGetRegion/not_found (0.00s)
=== RUN   TestGetZone
=== RUN   TestGetZone/base
=== RUN   TestGetZone/LabelZoneFailureDomain
=== RUN   TestGetZone/not_found
--- PASS: TestGetZone (0.00s)
    --- PASS: TestGetZone/base (0.00s)
    --- PASS: TestGetZone/LabelZoneFailureDomain (0.00s)
    --- PASS: TestGetZone/not_found (0.00s)
=== RUN   TestGetInstanceType
=== RUN   TestGetInstanceType/base
=== RUN   TestGetInstanceType/LabelInstanceTypeStable
=== RUN   TestGetInstanceType/not_found
--- PASS: TestGetInstanceType (0.00s)
    --- PASS: TestGetInstanceType/base (0.00s)
    --- PASS: TestGetInstanceType/LabelInstanceTypeStable (0.00s)
    --- PASS: TestGetInstanceType/not_found (0.00s)
=== RUN   TestGetOperatingSystem
=== RUN   TestGetOperatingSystem/base
=== RUN   TestGetOperatingSystem/betaLabel
=== RUN   TestGetOperatingSystem/not_found
--- PASS: TestGetOperatingSystem (0.00s)
    --- PASS: TestGetOperatingSystem/base (0.00s)
    --- PASS: TestGetOperatingSystem/betaLabel (0.00s)
    --- PASS: TestGetOperatingSystem/not_found (0.00s)
PASS
coverage: 100.0% of statements
ok      command-line-arguments  0.010s  coverage: 100.0% of statements

@@ -5,41 +5,50 @@ import (
)

func GetRegion(labels map[string]string) (string, bool) {
if _, ok := labels[v1.LabelTopologyRegion]; ok { // Label as of 1.17
_, ok := labels[v1.LabelTopologyRegion]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need to make such change as the original code line is a better approach to me, it create a variable in smaller scope and the memory would be release soon. and it's a common practice in golang.
if _, ok := labels[v1.LabelTopologyRegion]; ok {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need to make such change as the original code line is a better approach to me, it create a variable in smaller scope and the memory would be release soon. and it's a common practice in golang. if _, ok := labels[v1.LabelTopologyRegion]; ok {

The original code has a little bit much if-else, which seems is not necessary, so the pr is try to reduce the unnecessary code for making the logic clearn.
Speaking of golang's idioms, on one hand I suppose happy path is the commonly used way in golang for reducing indent and embeded codes, on the other hand the memory will be eventually released after the function returned, just sooner or later, not a big deal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed with @mfanjie

Copy link
Author

@jxs1211 jxs1211 Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed with @mfanjie

@qmhu @mfanjie
updated.

@jxs1211 jxs1211 force-pushed the test-pkg-util-annotation branch 2 times, most recently from 5686fd6 to fdc209f Compare September 23, 2022 00:47
Signed-off-by: xian-jie.shen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants