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: refactor the cli stop test #2372

Merged
merged 1 commit into from
Oct 28, 2018

Conversation

ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Oct 27, 2018

Signed-off-by: zhangyue [email protected]

Ⅰ. Describe what this PR did

  1. add inspect filter func
  2. use 1 to refactor the test case in cli stop test
  3. add some clean up operation

Ⅱ. Does this pull request fix one issue?

None.

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

None.

Ⅳ. Describe how to verify it

go test -gocheck.f PouchStopSuite

Ⅴ. Special notes for reviews

None.

@codecov
Copy link

codecov bot commented Oct 27, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2372      +/-   ##
==========================================
+ Coverage   66.82%   68.27%   +1.44%     
==========================================
  Files         265      265              
  Lines       18211    18211              
==========================================
+ Hits        12169    12433     +264     
+ Misses       4637     4355     -282     
- Partials     1405     1423      +18
Flag Coverage Δ
#criv1alpha1test 31.68% <ø> (+0.19%) ⬆️
#criv1alpha2test 35.48% <ø> (-64.52%) ⬇️
#integrationtest 39.6% <ø> (+0.04%) ⬆️
#nodee2etest 33.02% <ø> (+0.03%) ⬆️
#unittest 25.52% <ø> (ø) ⬆️
Impacted Files Coverage Δ
ctrd/container.go 59.76% <0%> (+0.47%) ⬆️
cri/v1alpha1/cri.go 61.03% <0%> (+0.64%) ⬆️
daemon/mgr/container.go 59.39% <0%> (+0.86%) ⬆️
cri/v1alpha2/cri.go 67.95% <0%> (+2.9%) ⬆️
cri/stream/remotecommand/httpstream.go 46.63% <0%> (+3.1%) ⬆️
cri/v1alpha2/cri_utils.go 90.57% <0%> (+3.28%) ⬆️
ctrd/watch.go 83.33% <0%> (+4.54%) ⬆️
daemon/mgr/container_storage.go 59.81% <0%> (+8.27%) ⬆️
cri/v1alpha2/cri_wrapper.go 61.2% <0%> (+8.8%) ⬆️
storage/quota/prjquota.go 27.51% <0%> (+12.08%) ⬆️
... and 2 more

test/utils.go Outdated
format := fmt.Sprintf("{{%s}}", filter)
result := command.PouchRun("inspect", "-f", format, name)
if result.Error != nil || result.ExitCode != 0 {
return "", fmt.Errorf("failed to inspect %s: %s", name, result.Combined())
Copy link
Collaborator

@allencloud allencloud Oct 28, 2018

Choose a reason for hiding this comment

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

could you update the error message to insert more info about filter? @ZYecho
like fmt.Errorf("failed to inspect container %s via filter %s: %s", name, filter, result.Combined())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@@ -95,3 +97,12 @@ func IsTLSExist() bool {
}
return true
}

func inspectFilter(name, filter string) (string, error) {
Copy link
Collaborator

@allencloud allencloud Oct 28, 2018

Choose a reason for hiding this comment

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

This encapsulated function looks so good to me. 😄 🌹 🍻
How about adding a comment for this function to tell others that, no matter what kind of filter input, the result will be translated into a type of string. This is different from the original way that using unmarshal way will get the real type of construct filed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@allencloud
Copy link
Collaborator

This is really what we need in pouchcontainer's test. Thanks a lot for your contribution. @ZYecho

@allencloud
Copy link
Collaborator

2018-10-28 2 57 56

I search all the test case file, and I found that there are more than 10 times to appear output := command.PouchRun("inspect", name).Stdout(). And I think there are some cases in which we could take advantages of your encapsulated function inspectFilter.

We could replace those codes with inspectFilter.
Maybe in next pull request. @ZYecho 🍻

@ZYecho
Copy link
Contributor Author

ZYecho commented Oct 28, 2018

2018-10-28 2 57 56 I search all the test case file, and I found that there are more than 10 times to appear `output := command.PouchRun("inspect", name).Stdout()`. And I think there are some cases in which we could take advantages of your encapsulated function `inspectFilter`. We could replace those codes with `inspectFilter`. Maybe in next pull request. @ZYecho

Thanks for your imformation.
I will work on it in next pr.

@ZYecho ZYecho force-pushed the add-inspect-utils branch from 63009bd to 799051d Compare October 28, 2018 07:28
@ZYecho
Copy link
Contributor Author

ZYecho commented Oct 28, 2018

@allencloud Sometimes the cri test in CI may fail, could you help to check this?

@allencloud
Copy link
Collaborator

Sometimes the cri test in CI may fail, could you help to check this?

It is an existing issue of PouchContainer. However it will hardly influence user's experience on PouchContainer. @ZYecho
For what causes this, it is IO handling of container. And @fuweid is working on it with the newly pr #2375 put forward .
we have already recorded the the flaky test in #2290.

@allencloud
Copy link
Collaborator

LGTM. Thanks again. I am merging this.

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Oct 28, 2018
@allencloud allencloud merged commit fc4396f into AliyunContainerService:master Oct 28, 2018
@ZYecho ZYecho deleted the add-inspect-utils branch November 6, 2018 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
areas/test kind/refactor LGTM one maintainer or community participant agrees to merge the pull reuqest. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants