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 api tests for some commands #1747

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

xiechengsheng
Copy link
Contributor

@xiechengsheng xiechengsheng commented Jul 17, 2018

Signed-off-by: xiechengsheng XIE1995@whut.edu.cn

Ⅰ. Describe what this PR did

add api tests for some commands.

  1. add state check in api pause test.
  2. add remove image case in api rmi test.
  3. add api test for image load and save functionality.

Ⅱ. Does this pull request fix one issue?

NONE

Ⅲ. Describe how you did it

Thanks for @fuweid 's help.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Jul 19, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1747      +/-   ##
==========================================
- Coverage    62.8%   62.72%   -0.09%     
==========================================
  Files         200      200              
  Lines       15567    15567              
==========================================
- Hits         9777     9764      -13     
- Misses       4544     4552       +8     
- Partials     1246     1251       +5
Flag Coverage Δ
#criv1alpha1test 33.24% <ø> (-0.04%) ⬇️
#criv1alpha2test 33.73% <ø> (ø) ⬆️
#integrationtest 37.9% <ø> (-0.03%) ⬇️
#unittest 20.7% <ø> (ø) ⬆️
Impacted Files Coverage Δ
apis/server/utils.go 61.9% <0%> (-4.77%) ⬇️
daemon/logger/jsonfile/utils.go 71.54% <0%> (-1.63%) ⬇️
ctrd/container.go 48.79% <0%> (-1.38%) ⬇️
daemon/mgr/container.go 53.69% <0%> (-0.32%) ⬇️
cri/v1alpha1/cri.go 65.04% <0%> (-0.19%) ⬇️

@xiechengsheng
Copy link
Contributor Author

cc @fuweid PTAL, thanks.

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.

It seems that the change is not only for load and save. Could you mind to change the git message and the PR title? Thanks

loadImageName := "load-helloworld"
q = url.Values{}
q.Set("name", loadImageName)
q.Set("quiet", "0")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we support the quiet query here 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right.

query := request.WithQuery(q)
resp, err := request.Get("/images/save", query)
c.Assert(err, check.IsNil)
defer resp.Body.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

could we use the tmpfile to store the intermediate result? It seems that it is tricky to use one response body as other request body. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will done soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that save the body data into tmpFile not the var

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 I don't think we should create a new file to store the response data stream, because this is a API test, not client test. In API test I think we handle the http response data is enough. And both save and load commands handle the data stream to tar file(or tar file to data stream) in client side, not the http response side. And in other API test, such as api_image_inspect_test and api_container_inspect_test, we just use the HTTP response data to check the correctness of this API function. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

client test includes the api test, I think. cli wrap the client to provide better UEX. for this cases, the api test is same to the cli test. However, you connect two http connections, which is pretty hacking in testing. So I want to save the data in the local and send it again.

@xiechengsheng xiechengsheng changed the title test: add api test for image load and save test: add api tests for some commands Jul 23, 2018
@chuanchang
Copy link
Contributor

It works for me.

@xiechengsheng
Copy link
Contributor Author

@fuweid Have fixed all bugs, PTAL again, thanks a lot~

@allencloud allencloud assigned chuanchang and fuweid and unassigned chuanchang Jul 29, 2018
query := request.WithQuery(q)
resp, err := request.Get("/images/save", query)
c.Assert(err, check.IsNil)
defer resp.Body.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that save the body data into tmpFile not the var

Signed-off-by: xiechengsheng <XIE1995@whut.edu.cn>
@allencloud
Copy link
Collaborator

Unfortunately, The code check CI fails:

gometalinter
gometalinter --config .gometalinter.json ./...
test/api_image_save_and_load_test.go:1::warning: file is not gofmted with -s (gofmt)
test/api_image_save_and_load_test.go:1::warning: file is not goimported (goimports)
Makefile:122: recipe for target 'gometalinter' failed
make: *** [gometalinter] Error 1

pity 📡 @xiechengsheng

@xiechengsheng
Copy link
Contributor Author

Yes, just now this pr fails on code check, it's my mistake.
@fuweid Have updated codes according to your meaning, PTAL, thanks~

@xiechengsheng
Copy link
Contributor Author

@fuweid Please take a look this pr, thanks a lot~

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 5ed312e into AliyunContainerService:master Aug 1, 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