Introduce `test-integration` target (and deprecate/freeze `test-integration-cli`) by vdemeester · Pull Request #33344 · moby/moby

vdemeester · 2017-05-22T22:16:09Z

This adds a new package integration where moby integration tests
should live. Those integration tests should not depends on any cli
components (except from the dockerd daemon for now — to actually
start a daemon).

This is a simple, naive approach and is a middle ground between current integration-cli and unit test.

The related topics is Evolution of testing @moby, issue #32866.

setup/cleanup defer should be extracted in another package
same fo createClient
should restore the protectXX too
integration-cli/environment and /integration-cli/fixtures/load should move to integration package
remove docker binary dependency from integration-cli/environment (the cleanup is using docker commands for now)

Signed-off-by: Vincent Demeester vincent@sbr.pm

aaronlehmann · 2017-05-22T22:24:02Z

integration/container/create_test.go

strconv.FormatInt

mlaventure

couple of nits

mlaventure · 2017-05-22T22:21:51Z

integration/container/create_test.go

nit: can be removed

mlaventure · 2017-05-22T22:21:59Z

hack/make/.integration-test-helpers

maybe the reverse? if I specify -test.timeout in my TESTFLAGS it'll be overridden by the script if I'm not mistaken.

@mlaventure hum right 👼 I just copied the others bundle_xx but yep, make sense

mlaventure · 2017-05-22T22:26:11Z

22:23:52 ---> Making bundle: test-unit (in bundles/17.06.0-dev/test-unit)
22:23:52 /go/src/github.com/docker/docker/hack/make/test-unit: line 40: syntax error near unexpected token `|'

Janky failure

aaronlehmann · 2017-05-22T22:26:50Z

integration/container/create_test.go

strconv.FormatInt

aaronlehmann · 2017-05-22T22:27:43Z

integration/container/create_test.go

Do you expect to plumb context through the test cases at some point? Otherwise, I think TODO is not the right context to use here (we should probably have ctx := context.Background() above.

aaronlehmann · 2017-05-22T22:41:16Z

I'm wondering if it would be simpler to rename integration-cli to integration and gradually migrate existing tests to use the API directly, and/or move them elswhere.

It would be nice to avoid having two packages/targets with similar names that run different sets of tests. I could see confusion coming up between the two, such as people accidentally adding tests to integration-cli, or running only one of the two to validate changes.

vdemeester · 2017-05-23T20:31:06Z

@aaronlehmann

I'm wondering if it would be simpler to rename integration-cli to integration and gradually migrate existing tests to use the API directly, and/or move them elsewhere.

Right, that's more or less what I was going for in #33124. The thing is, rewriting them (and/or extracting them) will take a long time, and depending on where we go with those new tests (i.e. using testing like this PR, or keep using go-check) the way we should run them could be really different.

It would be nice to avoid having two packages/targets with similar names that run different sets of tests. I could see confusion coming up between the two, such as people accidentally adding tests to integration-cli, or running only one of the two to validate changes.

Yeah I can see confusing coming up too. I'm still not 100% sold on having those two separate either, it's just a proposal 👼.

What I would like to be discussed and decided on for this PR are the following :

test-integration & test-integration-cli or have only a test-integration package with all of them, but some freezed and being worked on to move elsewhere or rewrote ?
testing (like the current proposal, with the use of subtest, etc..) or go-check ?

From there, this PR will be the start of the work and examples 😉

…ration-cli`) This adds a new package `integration` where `moby` integration tests should live. Those integration tests should not depends on any `cli` components (except from the `dockerd` daemon for now — to actually start a daemon). Signed-off-by: Vincent Demeester <vincent@sbr.pm>

aaronlehmann · 2017-05-23T20:39:26Z

test-integration & test-integration-cli or have only a test-integration package with all of them, but some freezed and being worked on to move elsewhere or rewrote ?

I prefer to have a single package during the transition.

dnephin · 2017-05-23T20:59:48Z

+1 for testing , go-check doesn't seem to provide much value.

A single package is a problem. It makes running a subset of the tests much slower than it needs to be. We should really have a package for each test suite, and a suite for each component.

A single target for CI SGTM.

aaronlehmann · 2017-05-23T21:01:34Z

A single package is a problem. It makes running a subset of the tests much slower than it needs to be. We should really have a package for each test suite, and a suite for each component.

A single target for CI SGTM.

That also sounds good to me. What I meant was that I'd like to have a single CI target.

vdemeester · 2017-05-23T21:02:29Z

A single package is a problem. It makes running a subset of the tests much slower than it needs to be. We should really have a package for each test suite, and a suite for each component.

@dnephin Either way, it will be splitted into several packages for sure. A single package is just for the main package, i.e. integration or integration-cli (i.e. any new tests written should target the api and be on sub package of integration).

thaJeztah · 2017-06-02T14:15:37Z

Makefile

 test-docker-py: build ## run the docker-py tests
 	$(DOCKER_RUN_DOCKER) hack/make.sh dynbinary test-docker-py

+# Deprecated: use `test-integration` instead


Can you add the "deprecation" message in the comment below? the ## run the integration tests comment is also used for outputting the help message (make help)

thaJeztah · 2017-06-09T00:06:35Z

@dnephin @aaronlehmann what's the status on this one?

vdemeester · 2017-06-09T07:52:57Z

@thaJeztah I need to re-work that one 👼

vdemeester · 2017-06-19T08:31:15Z

Closing this temporarly 👼

vdemeester requested review from aaronlehmann, cpuguy83, dnephin, mlaventure, thaJeztah, tiborvass and yongtang May 22, 2017 22:16

GordonTheTurtle added the status/0-triage label May 22, 2017

aaronlehmann reviewed May 22, 2017

View reviewed changes

integration/container/create_test.go Outdated

Copy link
Copy Markdown

aaronlehmann May 22, 2017

Choose a reason for hiding this comment

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

strconv.FormatInt

vdemeester reacted with heart emoji

mlaventure suggested changes May 22, 2017

View reviewed changes

aaronlehmann reviewed May 22, 2017

View reviewed changes

integration/container/create_test.go Outdated

Copy link
Copy Markdown

aaronlehmann May 22, 2017

Choose a reason for hiding this comment

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

strconv.FormatInt

aaronlehmann reviewed May 22, 2017

View reviewed changes

vdemeester force-pushed the test-integration-api branch from 0bd4ae1 to 5a90d23 Compare May 22, 2017 22:32

vdemeester added status/1-design-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels May 23, 2017

vdemeester force-pushed the test-integration-api branch from 5a90d23 to 0635b83 Compare May 23, 2017 00:50

vdemeester mentioned this pull request May 23, 2017

[test-integration] improvements #31410

Closed

15 tasks

vdemeester force-pushed the test-integration-api branch from 0635b83 to a9524bd Compare May 23, 2017 20:22

vdemeester force-pushed the test-integration-api branch from a9524bd to d5b3c31 Compare May 23, 2017 20:32

vdemeester added rebuild/windowsRS1 labels May 26, 2017

vdemeester removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 26, 2017

GordonTheTurtle removed rebuild/windowsRS1 labels May 26, 2017

thaJeztah reviewed Jun 2, 2017

View reviewed changes

GordonTheTurtle assigned tiborvass Jun 6, 2017

vdemeester closed this Jun 19, 2017

This was referenced Jul 6, 2017

Move test-unit out of hack/make #33987

Merged

Introduce test-integration target #34000

Merged

vdemeester deleted the test-integration-api branch February 28, 2018 08:10

Conversation

vdemeester commented May 22, 2017 • edited by dnephin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented May 22, 2017

Uh oh!

vdemeester commented May 23, 2017

Uh oh!

aaronlehmann commented May 23, 2017

Uh oh!

dnephin commented May 23, 2017

Uh oh!

aaronlehmann commented May 23, 2017

Uh oh!

vdemeester commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 9, 2017

Uh oh!

vdemeester commented Jun 9, 2017

Uh oh!

vdemeester commented Jun 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vdemeester commented May 22, 2017 •

edited by dnephin

Loading

vdemeester May 22, 2017 •

edited

Loading