test: refactor test package and add cases #331

fuweid · 2017-12-15T08:21:12Z

Signed-off-by: Wei Fu fhfuwei@163.com

1.Describe what this PR did

Refactor test package and add more tests

2.Does this pull request fix one issue?
none

3.Describe how you did it

runCmd wraps icmd and does the same thing as icmd. In order to make maintainer eaiser, I remove the runCmd and create command package in test package.
test package not just test the cli command, but also test the pouch api. For this reason, I create request package to handle the request. And then the host address is stable. We can do initailization work in request package so that case doesn't need to do the duplicate work about initialization.
Before this commit, the almost cases doesn't care the result. We need to inspect the result. In this commit, the trick one is pouch start -a -i. In order to test attach and tty, we can create pty to write and read the content.

4.Describe how to verify it
none

5.Special notes for reviews

During testing, I meets some issues:

The pouch pull requires the pty/tty device as STDIN. [bug] pouch pull can't be run by using exec package #313
The pouch rm -f cannot clear /var/lib/pouch/containers/{containerID} folder. The reason is [bug] The phantom container's meta data #326
The pouch {command} "" is valid. The "" is emtpy and invalid. pouch pull "" should equal pouch pull. But pouch pull "" runs sucessfully.

Ping @Letty5411 @allencloud

allencloud · 2017-12-18T06:49:27Z

test/environment/cleanup.go

+)
+
+// PruneAllImages deletes all images from pouchd.
+func PruneAllImages(apiClient client.ImageAPIClient) error {


Why func PruneAllImages(apiClient client.ImageAPIClient) error?
Not func (apiClient client.ImageAPIClient) PruneAllImages() error?

Original idea is that the PruneAllImages is only used in test package. It comes from docker system prune. If you need this in pouch, we can implement it in cli package.

It is not in pouch's first stage.
Maybe in the future, we will support this.

Got it. Make senses

allencloud · 2017-12-18T08:06:37Z

All test case fails with the stderr output:

    t.Fatalf("at %s:%d - %s\n", filepath.Base(file), line, err.Error())
... Error: at pouch_cli_create_test.go:25 - 
Command:  /usr/local/bin/pouch pull registry.hub.docker.com/library/busybox:latest
ExitCode: 2
Error:    exit status 2
Stdout:   
Stderr:   panic: provided file is not a console

allencloud · 2017-12-18T08:07:53Z

test package not just test the cli command, but also test the pouch api. For this reason, I create request package to handle the request. And then the host address is stable. We can do initailization work in request package so that case doesn't need to do the duplicate work about initialization.

I totally support this idea.

sunyuan3 · 2017-12-18T09:02:14Z

@0x04C2 The CI is failed.

https://travis-ci.org/alibaba/pouch/builds/316819586?utm_source=github_status&utm_medium=notification

You can debug your code in travis according to the following document.
https://github.com/alibaba/pouch/blob/master/docs/test/debug_in_travis.md

fuweid · 2017-12-18T10:12:10Z

Hi @sunyuan3 and @allencloud , the CI is expected to be failed. I try to fix this issue in #345

allencloud · 2017-12-19T02:20:23Z

#345 has been merged. Please rebase this. @0x04C2

fuweid · 2017-12-19T02:35:23Z

Got it. Hold on a sec.

Signed-off-by: Wei Fu <fhfuwei@163.com>

fuweid · 2017-12-19T05:02:12Z

@allencloud , has been rebased. Please check it.

allencloud · 2017-12-19T06:02:04Z

test/pouch_cli_create_test.go

+		// {name: "missing image name", args: ""},
+	} {
+		res := command.PouchRun("create", tc.args)
+		c.Assert(res.Error, check.NotNil, check.Commentf(tc.name))


Yes, I think we should use more c.Assert in test cases. It is much better now. 👍

allencloud · 2017-12-19T06:04:24Z

test/pouch_cli_images_test.go

-	args := map[string]bool{
-		"": false,
+// getImageInfo is used to retrieve the information about image.
+func getImageInfo(apiClient client.ImageAPIClient, name string) (types.ImageInfo, error) {


We need to add an API GET /images/{idOrName}/json 😄 I recorded this in issue #138

On my plan. Haha

allencloud · 2017-12-19T06:08:26Z

test/pouch_cli_ps_test.go

+
+	// running
+	{
+		command.PouchRun("start", name).Assert(c, icmd.Success)


We need to use pouch run to replace create and start, if issue #351 is solved. Just leave a comment to record this.

I think there is no need to use run command, because I want to check the created, running and stopped. If I use run, I will miss the created status. :)

allencloud · 2017-12-19T06:17:35Z

test/request/request.go

+	"github.com/alibaba/pouch/test/environment"
+)
+
+// Get sends request to the default pouchd server with custom request options.


I like this package 😄

allencloud · 2017-12-19T06:19:22Z

vendor/vendor.json

@@ -507,6 +507,12 @@
 			"revision": "b5461758462b97cff258bf388ca42110f529d747",
 			"revisionTime": "2017-11-07T15:27:38Z"
 		},
+		{
+			"checksumSHA1": "jHOLsfeMPcX84YQB2MXBqXFntBg=",
+			"path": "github.com/kr/pty",


This package is licensed under MIT.
Just comment to mark this.

In the future, we will list all the licenses of vendored package.

Oh. I don't realize this. Sorry about that.

It seems that we can do this without pr/tty. I will try.

It is all right. Maybe in the future. We need these test very much. I think we could merge this ASAP.

allencloud · 2017-12-19T06:35:28Z

LGTM. Generally.

While I have some tiny suggestion for this PR. These advices all help users could learn test easily.

the PouchBinary = "/usr/local/bin/pouch" is a little bit weird. Although it is totally correct, it is not so straight forward for users. However we can prepare that for users, such as, locate binary pouch in $PATH, and then could let user execute cmd := exec.Command("pouch", "start", "-a", "-i", name) rather than exec.Command(environment.PouchBinary, "start", "-a", "-i", name);
PouchRun could be ambiguous for users, because it will make users think of command pouch run which will be implemented in the future;
Can we find out a way to simplify the Suite configuring thing? Since I think coming to test files to add test cases is the best way for users to quick start.

All the points above is a record, not blocking this pr. We can handle this in follow-ups absolutely.

/cc @sunyuan3 @Letty5411 WDYT?

allencloud · 2017-12-19T08:31:23Z

Thanks again for your contribution. I am going to merge this. 🍻

pouchrobot added areas/test kind/refactor size/XXL labels Dec 15, 2017

allencloud assigned sunyuan3 and Letty5411 Dec 15, 2017

allencloud reviewed Dec 18, 2017

View reviewed changes

test: refactor test package and add cases

6f9bf74

Signed-off-by: Wei Fu <fhfuwei@163.com>

allencloud reviewed Dec 19, 2017

View reviewed changes

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 19, 2017

allencloud merged commit 2664092 into AliyunContainerService:master Dec 19, 2017

pouchrobot mentioned this pull request Dec 22, 2017

Weekly Report of Pouch from 2017-12-15 to 2017-12-22 #385

Closed

fuweid deleted the test_update_integration_test branch August 3, 2018 02:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: refactor test package and add cases #331

test: refactor test package and add cases #331

fuweid commented Dec 15, 2017 •

edited

Loading

allencloud Dec 18, 2017

fuweid Dec 18, 2017

allencloud Dec 19, 2017

fuweid Dec 19, 2017

allencloud commented Dec 18, 2017 •

edited

Loading

allencloud commented Dec 18, 2017 •

edited

Loading

sunyuan3 commented Dec 18, 2017

fuweid commented Dec 18, 2017

allencloud commented Dec 19, 2017

fuweid commented Dec 19, 2017

fuweid commented Dec 19, 2017

allencloud Dec 19, 2017

allencloud Dec 19, 2017 •

edited

Loading

fuweid Dec 19, 2017

allencloud Dec 19, 2017 •

edited

Loading

fuweid Dec 19, 2017

allencloud Dec 19, 2017

allencloud Dec 19, 2017

allencloud Dec 19, 2017

fuweid Dec 19, 2017

allencloud Dec 19, 2017

allencloud commented Dec 19, 2017 •

edited

Loading

allencloud commented Dec 19, 2017

test: refactor test package and add cases #331

test: refactor test package and add cases #331

Conversation

fuweid commented Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Dec 18, 2017 • edited Loading

allencloud commented Dec 18, 2017 • edited Loading

sunyuan3 commented Dec 18, 2017

fuweid commented Dec 18, 2017

allencloud commented Dec 19, 2017

fuweid commented Dec 19, 2017

fuweid commented Dec 19, 2017

Choose a reason for hiding this comment

allencloud Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allencloud commented Dec 19, 2017 • edited Loading

allencloud commented Dec 19, 2017

fuweid commented Dec 15, 2017 •

edited

Loading

allencloud commented Dec 18, 2017 •

edited

Loading

allencloud commented Dec 18, 2017 •

edited

Loading

allencloud Dec 19, 2017 •

edited

Loading

allencloud Dec 19, 2017 •

edited

Loading

allencloud commented Dec 19, 2017 •

edited

Loading