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: add test cases for parse_test.go #2600

Merged
merged 1 commit into from
Dec 23, 2018

Conversation

WillSmisi
Copy link
Contributor

Ⅰ. Describe what this PR did

test: add test cases for parse_test.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 22, 2018

Codecov Report

Merging #2600 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2600      +/-   ##
==========================================
- Coverage   69.47%   69.37%   -0.11%     
==========================================
  Files         279      279              
  Lines       18825    18825              
==========================================
- Hits        13079    13060      -19     
- Misses       4280     4297      +17     
- Partials     1466     1468       +2
Flag Coverage Δ
#criv1alpha1test 31.69% <ø> (-0.18%) ⬇️
#criv1alpha2test 36.14% <ø> (-0.14%) ⬇️
#integrationtest 41.14% <ø> (+0.01%) ⬆️
#nodee2etest 33.3% <ø> (+0.06%) ⬆️
#unittest 26.78% <ø> (+0.02%) ⬆️
Impacted Files Coverage Δ
cri/ocicni/cni_manager.go 58.82% <0%> (-11.77%) ⬇️
pkg/streams/utils.go 82.14% <0%> (-7.15%) ⬇️
cri/stream/portforward/httpstream.go 70.94% <0%> (-6.84%) ⬇️
cri/stream/runtime.go 67.85% <0%> (-2.39%) ⬇️
daemon/mgr/container.go 58.79% <0%> (-0.22%) ⬇️
cri/v1alpha2/cri.go 69.67% <0%> (+0.12%) ⬆️
cri/v1alpha2/cri_utils.go 89.92% <0%> (+0.28%) ⬆️
apis/server/utils.go 75% <0%> (+3.84%) ⬆️

@pouchrobot
Copy link
Collaborator

We found this is your first time to contribute to Pouch, @WillSmisi
👏 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 22, 2018

CLA assistant check
All committers have signed the CLA.

@WillSmisi WillSmisi force-pushed the dev branch 2 times, most recently from 402996e to 88567a0 Compare December 22, 2018 11:22
@ihac
Copy link
Contributor

ihac commented Dec 22, 2018

Seems that you signed off with a wrong email? The commit would not be attributed to you. Just let you know, and you might wanna have a double check on it. (Refer to Setting your commit email address in Git)

@WillSmisi
Copy link
Contributor Author

@ihac Thank you for telling the message about my mistakes.🤓

wantErr bool
}{
{
"mapping keys are in the accepted set",
Copy link
Contributor

Choose a reason for hiding this comment

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

@WillSmisi since there are many fields for the test case, could we add the filed name when we do assignment? something like that

{
  name: xxx,
  testArgs: yyy,
  accepted: zzz,
 ...
}

It can be more clear for other. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fuweid Thank you for suggestions.
I saw other test files also miss the filed names and The IDE that I used can autocomplete them, so I guessed maybe this is a special style. It seems I'm wrong. Ahh~
Now I know this is not good for others, I 'll follow your advice right now.
thank you again!

Copy link
Contributor

Choose a reason for hiding this comment

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

both styles are ok to golint/gofmt. But I think it can be better if the assignment is not one-line. Thanks for the update.

Signed-off-by: WillSmisi <764947976@qq.com>
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 563237f into AliyunContainerService:master Dec 23, 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.

6 participants