Implement internal/info/info test #862

kevindiu · 2020-11-27T06:33:59Z

Description:

This PR implements internal/info/info test.
It refactors info.go to allow mocking the runtime.Caller and runtime.FuncForPC function to complete the test case.

grammar check passed

Because this PR includes refactoring, author review is required.

Related Issue:

How Has This Been Tested?:

Environment:

Go Version: 1.15.2
Docker Version: 19.03.8
Kubernetes Version: 1.18.2
NGT Version: 1.12.1

Types of changes:

Bug fix [type/bug]
New feature [type/feature]
Add tests [type/test]
Security related changes [type/security]
Add documents [type/documentation]
Refactoring [type/refactoring]
Update dependencies [type/dependency]
Update benchmarks and performances [type/bench]
Update CI [type/ci]

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes locally?

Checklist:

I have read the CONTRIBUTING document.
I have checked open Pull Requests for the similar feature or fixes?
I have added tests and benchmarks to cover my changes.
I have ensured all new and existing tests passed.
I have commented my code, particularly in hard-to-understand areas
I have updated the documentation accordingly.

vdaas-ci · 2020-11-27T06:34:21Z

[CHATOPS:HELP] ChatOps commands.

🙆‍♀️ /approve - approve
💌 /changelog - add changelog comment
🍱 /format - format codes and add licenses
✅ /gen-test - generate test codes
🏷️ /label - add labels
⏪ /rebase - rebase master

internal/info/info_test.go

codecov · 2020-11-27T06:41:21Z

Codecov Report

Merging #862 (a6f2b59) into master (d113363) will increase coverage by 0.15%.
The diff coverage is 89.15%.

@@            Coverage Diff             @@
##           master     #862      +/-   ##
==========================================
+ Coverage   14.67%   14.82%   +0.15%     
==========================================
  Files         495      496       +1     
  Lines       28363    28337      -26     
==========================================
+ Hits         4161     4201      +40     
+ Misses      23953    23885      -68     
- Partials      249      251       +2

Impacted Files	Coverage Δ
internal/errors/info.go	`0.00% <0.00%> (ø)`
internal/info/info.go	`92.96% <92.18%> (+62.96%)`	⬆️
internal/info/option.go	`100.00% <100.00%> (ø)`
internal/errors/runner.go	`0.00% <0.00%> (-100.00%)`	⬇️
internal/errgroup/group.go	`92.42% <0.00%> (-1.52%)`	⬇️
internal/net/net.go
internal/worker/worker.go	`83.33% <0.00%> (+1.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d113363...1c95c7f. Read the comment docs.

kevindiu · 2020-11-30T01:45:11Z

/rebase

vdaas-ci · 2020-11-30T01:45:46Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/info

hlts2 · 2020-11-30T02:36:51Z

@kevindiu
Could you please fix failing test for TestDetail_Get??
https://github.com/vdaas/vald/pull/862/checks?check_run_id=1462354206#step:5:49

internal/info/info.go

internal/info/info_test.go

kevindiu · 2020-12-15T05:12:00Z

/rebase

vdaas-ci · 2020-12-15T05:12:36Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/info

internal/info/info.go

kevindiu · 2020-12-16T01:56:07Z

/rebase

vdaas-ci · 2020-12-16T01:56:41Z

[REBASE] Rebase triggered by kevindiu for branch: test/internal/info

kevindiu · 2020-12-16T01:59:39Z

internal/info/info.go

+func Init(name string) {
+	once.Do(func() {
+		infoProvider, _ = New(WithServerName(name))
+	})
+}


@hlts2 should we return the error in this function?

Suggested change

func Init(name string) {

once.Do(func() {

infoProvider, _ = New(WithServerName(name))

})

}

func Init(name string) error {

ip, err = New(WithServerName(name))

if err != nil {

return err

}

once.Do(func() {

infoProvider = ip

})

}

Hmmmm.. 🤔
I agree with your opinion.
but if you fix it, the interface will change, so it may be necessary to discuss it at first.

yes I think so. what do you think?
@vankichi @hlts2

I think it would be better retuning the error.
when you apply this implementation, how this change will affect another current implementations?

@vankichi @hlts2
changes of this function will cause the refactoring on 1 another file.
https://github.com/vdaas/vald/search?q=%22info.init%22

The image of the changes is like this, and this changes will cause all of the vald component not be able started if the name is not set. Or we can only log the error without returning the error here?

if err := info.Init(r.name); err != nil { // maybe log the error only? return err }

I think the Init() functions should not return error, same as the current implementation of master branch.
If the New() error functions returns some error inside the Init() function, it means the arguments passed in Init() function is invalid = it means there're bugs in our codes. So it is better to use log.Fatal like the following:

func Init(name string) error { ip, err = New(WithServerName(name)) // the passed arguments of the New function is just controlled by our code here! if err != nil { log.Fatal("info.Init failed") }

The behavior of the Init() function cannot be controlled by the config.yaml, I think it is okay to panic here.

@rinx I am not sure if your suggestion works, because the logger log.Init() is executed after the info is initialized.

vald/internal/runner/runner.go

Lines 65 to 74 in b54d583

if err := info.Init(r.name); err != nil {

return err

}

p, isHelp, err := params.New(

params.WithConfigFileDescription(fmt.Sprintf("%s config file path", r.name)),

).Parse()

if isHelp || err != nil {

log.Init(log.WithLevel(level.FATAL.String()))

What do you think?

@rinx what do you think about this changes?
237248d
cc: @kpango

thank you :)

Co-authored-by: Hiroto Funakoshi <[email protected]>

Signed-off-by: kevindiu <[email protected]>

Signed-off-by: vdaas-ci <[email protected]>

Co-authored-by: Hiroto Funakoshi <[email protected]>

Signed-off-by: kevindiu <[email protected]>

vdaas-ci · 2021-02-18T08:12:54Z

[FORMAT] Updating license headers and formatting go codes triggered by kevindiu.

kpango

LGTM

github-actions bot added area/internal team/set SET team size/XL type/test Adding tests labels Nov 27, 2020

kevindiu requested a review from hlts2 November 27, 2020 06:34

github-actions bot added the priority/low label Nov 27, 2020

github-actions bot reviewed Nov 27, 2020

View reviewed changes

internal/info/info_test.go Outdated Show resolved Hide resolved

github-actions bot reviewed Nov 30, 2020

View reviewed changes

hlts2 previously approved these changes Dec 1, 2020

View reviewed changes

kevindiu requested a review from vankichi December 1, 2020 02:44

vankichi reviewed Dec 1, 2020

View reviewed changes

kevindiu dismissed hlts2’s stale review via 2ffc08e December 1, 2020 03:28

kevindiu changed the title ~~Implement internal/info/info test~~ [WIP] Implement internal/info/info test Dec 2, 2020

github-actions bot added size/XXL and removed size/XL labels Dec 8, 2020

kevindiu changed the title ~~[WIP] Implement internal/info/info test~~ Implement internal/info/info test Dec 15, 2020

kevindiu requested a review from hlts2 December 15, 2020 05:11

github-actions bot force-pushed the test/internal/info branch from 9f55bd3 to bd90c25 Compare December 15, 2020 05:12

hlts2 reviewed Dec 16, 2020

View reviewed changes

internal/info/info.go Show resolved Hide resolved

github-actions bot force-pushed the test/internal/info branch from bd90c25 to 9495f9b Compare December 16, 2020 01:56

kevindiu commented Dec 16, 2020

View reviewed changes

kevindiu and others added 22 commits February 18, 2021 08:11

refactor info.go and add test case

251dc95

add comments for option

4c109e2

rename interface name

ac8187b

handle error in init()

ae52b50

Apply suggestions from code review

c52fa97

Co-authored-by: Hiroto Funakoshi <[email protected]>

fix info

4d02f8d

Signed-off-by: kevindiu <[email protected]>

add missing file

b05d269

Signed-off-by: kevindiu <[email protected]>

fix test case

c5078da

fix test

9ade6b1

add test

0fbf658

fix test cases

7dfebbc

refactor info detail object

07dbf5f

add test to StackTrace.String()

b6d0962

🤖 Update license headers / Format go codes and yaml files

75ff153

Signed-off-by: vdaas-ci <[email protected]>

fix failed test

e9bf91f

Update internal/errors/info.go

865ceaa

Co-authored-by: Hiroto Funakoshi <[email protected]>

fix test case name & add test case for test coverage

11b1b03

add comments and fix ci suggestions

46567dd

return error when runtime func is nil & fix comments

2227d56

Signed-off-by: kevindiu <[email protected]>

Update internal/info/info.go

0bdb204

fix

d5faf59

fix safety package error

c3e9328

vdaas-ci force-pushed the test/internal/info branch from 3e7b3bb to c3e9328 Compare February 18, 2021 08:12

kevindiu and others added 2 commits February 18, 2021 17:18

Merge branch 'master' into test/internal/info

d76d74b

Merge branch 'master' into test/internal/info

1c95c7f

kpango approved these changes Feb 19, 2021

View reviewed changes

kpango merged commit 878059d into master Feb 19, 2021

kpango deleted the test/internal/info branch February 19, 2021 01:05

vdaas-ci mentioned this pull request Feb 19, 2021

[patch] v1.0.3 release #1026

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement internal/info/info test #862

Implement internal/info/info test #862

kevindiu commented Nov 27, 2020 •

edited

Loading

vdaas-ci commented Nov 27, 2020

codecov bot commented Nov 27, 2020 •

edited

Loading

kevindiu commented Nov 30, 2020

vdaas-ci commented Nov 30, 2020

hlts2 commented Nov 30, 2020 •

edited

Loading

kevindiu commented Dec 15, 2020

vdaas-ci commented Dec 15, 2020

kevindiu commented Dec 16, 2020

vdaas-ci commented Dec 16, 2020

kevindiu Dec 16, 2020

hlts2 Dec 16, 2020

kevindiu Dec 16, 2020

vankichi Dec 18, 2020

kevindiu Dec 21, 2020 •

edited

Loading

rinx Dec 24, 2020

kevindiu Dec 28, 2020

kevindiu Jan 12, 2021

rinx Jan 12, 2021

kevindiu Jan 12, 2021

vdaas-ci commented Feb 18, 2021

kpango left a comment

-func Init(name string) {
-	once.Do(func() {
-		infoProvider, _ = New(WithServerName(name))
-	})
-}
+func Init(name string) error {
+        ip, err = New(WithServerName(name))
+        if err != nil {
+            return err
+        }
+	once.Do(func() {
+		infoProvider = ip
+	})
+}

	if err := info.Init(r.name); err != nil {
	return err
	}

	p, isHelp, err := params.New(
	params.WithConfigFileDescription(fmt.Sprintf("%s config file path", r.name)),
	).Parse()

	if isHelp \|\| err != nil {
	log.Init(log.WithLevel(level.FATAL.String()))

Implement internal/info/info test #862

Implement internal/info/info test #862

Conversation

kevindiu commented Nov 27, 2020 • edited Loading

Description:

Related Issue:

How Has This Been Tested?:

Environment:

Types of changes:

Changes to Core Features:

Checklist:

vdaas-ci commented Nov 27, 2020

codecov bot commented Nov 27, 2020 • edited Loading

Codecov Report

kevindiu commented Nov 30, 2020

vdaas-ci commented Nov 30, 2020

hlts2 commented Nov 30, 2020 • edited Loading

kevindiu commented Dec 15, 2020

vdaas-ci commented Dec 15, 2020

kevindiu commented Dec 16, 2020

vdaas-ci commented Dec 16, 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

kevindiu Dec 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdaas-ci commented Feb 18, 2021

kpango left a comment

Choose a reason for hiding this comment

kevindiu commented Nov 27, 2020 •

edited

Loading

codecov bot commented Nov 27, 2020 •

edited

Loading

hlts2 commented Nov 30, 2020 •

edited

Loading

kevindiu Dec 21, 2020 •

edited

Loading