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 unit test case for func ParseEnv #2584

Merged
merged 1 commit into from
Dec 20, 2018
Merged

test: add unit test case for func ParseEnv #2584

merged 1 commit into from
Dec 20, 2018

Conversation

jahentao
Copy link
Contributor

Signed-off-by: jahentao jahentao@qq.com

Ⅰ. Describe what this PR did

test: add unit test case for func ParseEnv

Ⅱ. Does this pull request fix one issue?

NONE

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

added

Ⅳ. Describe how to verify it

go test -v -timeout 30s github.com/alibaba/pouch/apis/opts -run "^(TestParseEnv)$"

Ⅴ. Special notes for reviews

Signed-off-by: jahentao <jahentao@qq.com>
@codecov
Copy link

codecov bot commented Dec 19, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@3470d89). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2584   +/-   ##
=========================================
  Coverage          ?   69.11%           
=========================================
  Files             ?      278           
  Lines             ?    18686           
  Branches          ?        0           
=========================================
  Hits              ?    12914           
  Misses            ?     4296           
  Partials          ?     1476
Flag Coverage Δ
#criv1alpha1test 31.28% <ø> (?)
#criv1alpha2test 35.65% <ø> (?)
#integrationtest 40.71% <ø> (?)
#nodee2etest 32.71% <ø> (?)
#unittest 26.66% <ø> (?)

@pouchrobot
Copy link
Collaborator

We found this is your first time to contribute to Pouch, @jahentao
👏 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! 🍻

@jahentao
Copy link
Contributor Author

How can i ask for a review?

@houstar
Copy link
Contributor

houstar commented Dec 19, 2018

@jahentao I'd like to use git blame to see who is the originally wrote the code. then ask the maintainer to assign this PR to @HIM review. :-)

{"test multiple '=' envs", args{env: []string{"A=1=2"}}, map[string]string{"A": "1=2"}, false},
{"test nil env", args{env: nil}, map[string]string{}, false}, // empty map
{"test empty env", args{env: []string{""}}, nil, true},
{"test empty blank", args{env: []string{" "}}, nil, true},
Copy link
Contributor

Choose a reason for hiding this comment

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

how about add a duplicate env test case?

Copy link
Contributor Author

@jahentao jahentao Dec 20, 2018

Choose a reason for hiding this comment

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

@ZYecho Thanks for your reviewing. Maybe it not sufficiently test before :-), such as nil and empty env situation, and i need a easy entry point for my first contributing. I will do better later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, you can simply make another commit for this pr.

Copy link
Collaborator

@allencloud allencloud Dec 20, 2018

Choose a reason for hiding this comment

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

Thanks a lot for adding more test cases. @jahentao
The code quality is quite important for us.
Like @ZYecho said, a duplicate env test is better. While we encourage you to improve this project in any kind of work, and feel free to tell us if you have any questions. @jahentao

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 20, 2018
@allencloud allencloud merged commit 7783bb1 into AliyunContainerService:master Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/test LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants