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

test:supplement test cases for apis/opts/config/runtime.go #2603

Closed
wants to merge 1 commit into from

Conversation

scy1993
Copy link
Contributor

@scy1993 scy1993 commented Dec 23, 2018

Signed-off-by: scy1993 21851406@zju.edu.cn

Ⅰ. Describe what this PR did

test:supplement test cases for apis/opts/config/runtime.go

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov
Copy link

codecov bot commented Dec 23, 2018

Codecov Report

Merging #2603 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2603      +/-   ##
==========================================
+ Coverage   69.46%   69.49%   +0.02%     
==========================================
  Files         279      279              
  Lines       18825    18825              
==========================================
+ Hits        13077    13082       +5     
+ Misses       4281     4276       -5     
  Partials     1467     1467
Flag Coverage Δ
#criv1alpha1test 31.84% <ø> (-0.03%) ⬇️
#criv1alpha2test 36.26% <ø> (+0.04%) ⬆️
#integrationtest 41.15% <ø> (-0.01%) ⬇️
#nodee2etest 33.25% <ø> (ø) ⬆️
#unittest 26.85% <ø> (+0.07%) ⬆️
Impacted Files Coverage Δ
apis/opts/config/runtime.go 100% <ø> (+33.33%) ⬆️
pkg/streams/utils.go 89.28% <0%> (-2.39%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
cri/v1alpha2/cri_wrapper.go 65.59% <0%> (-1.21%) ⬇️
cri/v1alpha2/cri.go 69.3% <0%> (ø) ⬆️
daemon/mgr/container.go 59% <0%> (+0.21%) ⬆️
ctrd/container.go 59.28% <0%> (+0.39%) ⬆️

@pouchrobot
Copy link
Collaborator

We found this is your first time to contribute to Pouch, @scy1993
👏 We really appreciate it.
Just remind that you have read the contribution guide: https://github.com/alibaba/pouch/blob/master/CONTRIBUTING.md
If you didn't, you should do that first. If done, welcome again and please enjoy hacking! 🍻

@CLAassistant
Copy link

CLAassistant commented Dec 23, 2018

CLA assistant check
All committers have signed the CLA.

Signed-off-by: scy1993 <21851406@zju.edu.cn>
@@ -10,9 +10,11 @@ import (
func TestNewRuntime(t *testing.T) {
assert := assert.New(t)

var tmp = map[string]types.Runtime(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

please not use tmp as var name, since it was meaningless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing out where I need to improve. I will pay attention to it in the future. Since there is a PR #2329 that has already add test cases, I will close my PR. Thanks for you advice anyway. Have a nice day.

@@ -23,3 +25,156 @@ func TestNewRuntime(t *testing.T) {
assert.NoError(runtime.Set("foo=bar"))
}
}

func TestRuntime_Set(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

keep the func name consistent, such as TestRuntimeSet

}
}

func TestRuntime_String(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

}
}

func TestRuntime_Type(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

}

func TestRuntime_String(t *testing.T) {
type fields struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

this struct seems could be defined out of func?

@chuanchang
Copy link
Contributor

chuanchang commented Dec 24, 2018

@scy1993 Thank you for your contribution, in fact, it exists a PR #2329 to add test cases and go through uncovered code path, please make sure it's not duplicate testing firstly.

@scy1993
Copy link
Contributor Author

scy1993 commented Dec 24, 2018

@scy1993 Thank you for your contribution, in fact, it exists a PR #2329 to add test cases and go through uncovered code path, please make sure it's not duplicate testing firstly.

I just notice that you have already submitted the PR#2329, I will close my PR. Have a nice day.

@scy1993 scy1993 closed this Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants