Introduce `test-integration` target by dnephin · Pull Request #34000 · moby/moby

dnephin · 2017-07-06T21:55:39Z

Addressed the feedback from #33344 and made some more changes to the hack scripts.

There is a single test target test-integration which runs all the new packages, as well as the old integration-cli package.

Not many test code changes yet. I'd like to merge the hack changes first, and iterate on the tests.

Also cleaned up verbose test output, can be enabled with TESTDEBUG=1

dnephin · 2017-07-06T21:56:55Z

hack/make/.integration-daemon-start

Setting a trap from some random function is not really correct or safe in bash. There can be only one trap per signal so this could override some other trap, or be overridden anywhere.

Instead the new script runs in a subshell and always runs cleanup even if the subshell exits non-zero.

vdemeester · 2017-07-07T08:01:46Z

22:34:43 FAIL: docker_cli_inspect_test.go:24: DockerSuite.TestInspectImage
22:34:43 
22:34:43 docker_cli_inspect_test.go:33:
22:34:43     id := inspectField(c, imageTest, "Id")
22:34:43 docker_utils_test.go:109:
22:34:43     c.Assert(err, check.IsNil)
22:34:43 ... value *errors.errorString = &errors.errorString{s:"failed to inspect emptyfs: \nError: No such object: emptyfs\n"} ("failed to inspect emptyfs: \nError: No such object: emptyfs\n")

Looking at that error, somehow there you miss the load of the image 👼

dnephin · 2017-07-07T14:15:18Z

Ya, the question is how 😄 I verified that the script still runs. I need to look into it further

thaJeztah · 2017-07-10T00:49:50Z

ping @tianon @albers PTAL as well ❤️

albers · 2017-07-11T17:55:50Z

As in #33987, I was not able to verify functionality. The script changes taken for themselves LGTM.

dnephin · 2017-07-12T22:02:50Z

Ok, the problem was the TODO about protecting images. Now the new suite will protect existing images as well.

AkihiroSuda · 2017-07-13T04:39:47Z

integration-cli-on-swram needs fix as well, but #dibs after this PR gets merged

dnephin · 2017-07-13T15:40:55Z

Build is green, this is ready for review

aaronlehmann

I still think it is better to rename integration-cli to integration and make gradual improvements to it. I think if we establish a second set of tests it will lead to a lot of confusion (lack of clarity on where coverage for a particular feature exists, risk of overlapping coverage, people adding new tests to integration-cli instead of integration, etc). See #33344 (comment)

aaronlehmann · 2017-07-13T22:00:21Z

hack/make/.integration-test-helpers

Does cd ever write to stdout?

Oops, I had this as pushd and forgot to remove the > /dev/null when i changed it back to cd. Will remove it

@dnephin ping :)

aaronlehmann · 2017-07-13T22:04:11Z

hack/make/.integration-test-helpers

Does it make sense to build a binary for each package? I imagine that each binary will be quite large, so this may take a lot of space and could be fairly slow.

I believe we prebuild the integration-cli binary as a convenience so that syntax errors are caught before the dockerd binary is built. If there are going to be many binaries, maybe this doesn't make sense anymore, and it would be better to invoke tests with go test after the daemon is running.

Also, is there value in separating the integration tests into different packages if the script doesn't let you build/test packages individually?

Does it make sense to build a binary for each package?

I think the advantages out-weight the single disadvantage (disk space which is recovered when the test run ends).

I imagine that each binary will be quite large, so this may take a lot of space

Yes, I think this is the only disadvantage. I don't think the disk space will be all that significant, but it will definitely grow as we add more packages. I expect it might take 200M or so once we have everything split into separate packages. If it becomes an issue we can add a flag to disable the build-first so they can be cleaned up after each package suite runs, assuming 200M actually becomes a problem for someone. Right now I don't think it's worth adding that complexity.

and could be fairly slow

It shouldn't be any slower than running the complete test suite. go test builds this binary anyway, it just hides it. When we add support for only running specific packages it will be trivial to only build the binaries we are going to run. For now it's not any slower than not pre-building them, and it lets us fail fast when there are syntax errors, which is a big win.

I believe we prebuild the integration-cli binary as a convenience so that syntax errors are caught before the dockerd binary is built.

Not before the dockerd binary is built, but before we run the slow test setup parts of starting the daemon and loading in fixtures.

If there are going to be many binaries, maybe this doesn't make sense anymore, and it would be better to invoke tests with go test after the daemon is running.

If anything it's more important now. I don't want to run half the test suite then have it die because of a syntax error. It should build all the binaries first, then setup the environment, and finally run the tests. When we support running one package (or a subset of packages) at a time we can make sure it only builds the appropriate binaries. That feature should be easy to implement, just allow integration_api_dirs to be set by the user.

is there value in separating the integration tests into different packages if the script doesn't let you build/test packages individually?

Yes, I think there is a lot of value in this, it's one of the more important parts of this change. In the future, when we have more packakges, we can enable support for build/testing a single package or a subset of packages (as mentioned it should be really easy to add).

Until then I believe we still benefit from improved caching. As I understand it, if a package isn't modified go is able to use a cached version of it. By having more packages (as long as the packages are well separated) we know that all but one package should be cached on most runs. This should greatly reduce the startup time of the integration test suite (even without the further optimisation of running just a single package).

dnephin · 2017-07-14T04:49:30Z

I still think it is better to rename integration-cli to integration and make gradual improvements to it.

The task/target has already been renamed, so I guess you mean rename the package? I think keeping a single package and just renaming makes all the problems worse.

I think if we establish a second set of tests it will lead to a lot of confusion

I think mixing them will lead to a lot more confusion. By having two suites it is easier to communicate and discover what is legacy and what is the new way of writing an integration test. If we mix them it becomes a lot more difficult for users to discover correct examples. They'll mostly see a bunch of bad examples of the old way of doing things. It's much easier to say "everything in integration/ is a good example to follow", instead of "look for a few small examples mixed in with hundreds of bad examples".

Mixing the suites would also prevent us from doing a bunch of other necessary cleanup (splitting into multiple packages, ditching gocheck, improving the test setup/teardown).

We can setup checks that ensure no new tests are added to the old suite and that no tests in the new suite use the CLI.

lack of clarity on where coverage for a particular feature exists, risk of overlapping coverage

This is already a major problem with integration-cli. The test files are so massive, and the test are so convoluted that it's nearly impossible to discover if we already have a test for something. We just keep adding more overlapping tests because we can't figure out what exists now.

Adding a new suite will actually improve the situation. We will setup a pattern where tests are split into packages by component, and files by feature. That way all the tests for a specific feature are in the same file.

This doesn't solve the problem of overlap with integration-cli/, but we know if the test doesn't exist in integration/ that at least there is some work to do. Either to port an old test, or write a fresh one. I don't think keeping all the tests in a single package solves the problem either. It just continues to make it difficult to discover what exists, and it makes it more difficult to discover what has to be converted.

people adding new tests to integration-cli instead of integration

I don't think this will be a problem. We should be able to communicate to all maintainers of this repo that nothing should be added to integration-cli.

We can also write a hack/validate check that fails the build if anything new is added to the integration-cli suite. It's much harder to ensure that no new cli tests are added if we mix the suites.

vieux · 2017-08-08T17:06:08Z

I think this changes make sense. ping @moby/moby-maintainers let's move this forward

tiborvass · 2017-08-08T22:49:18Z

Agreed, +1

tiborvass · 2017-08-08T23:16:33Z

Dockerfile

@dnephin I understand that we're deprecating test-integration-cli, but I'm not sure I would like people to not run them just yet.

They are still run by test-integration it's just a new target name to better reflect the suite

mlaventure

LGTM

the cd integration-cli > /dev/null nit, is of no consequence to me ;-)

…ration-cli`) This adds a new package `integration` where `engine` 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>

Signed-off-by: Daniel Nephin <dnephin@docker.com>

mlaventure · 2017-08-11T15:22:56Z

ping @tiborvass

tiborvass · 2017-08-11T17:39:18Z

LGTM

dnephin requested a review from vdemeester July 6, 2017 21:55

GordonTheTurtle added the status/0-triage label Jul 6, 2017

dnephin added status/1-design-review and removed status/0-triage labels Jul 6, 2017

dnephin commented Jul 6, 2017

View reviewed changes

dnephin added the rebuild/windowsRS1 label Jul 6, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Jul 6, 2017

dnephin added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 7, 2017

dnephin force-pushed the test-integration-api branch from f3e8ed3 to a3f7e7c Compare July 12, 2017 22:02

dnephin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jul 12, 2017

dnephin requested a review from aaronlehmann July 13, 2017 15:40

aaronlehmann reviewed Jul 13, 2017

View reviewed changes

dnephin force-pushed the test-integration-api branch from 0f8fb5b to ab4a484 Compare July 18, 2017 18:44

dnephin added rebuild/powerpc labels Jul 19, 2017

GordonTheTurtle removed rebuild/powerpc labels Jul 19, 2017

GordonTheTurtle assigned coolljt0725 Jul 21, 2017

dnephin force-pushed the test-integration-api branch 2 times, most recently from 7877a99 to cad56e4 Compare July 27, 2017 16:50

dnephin requested review from thaJeztah and tianon as code owners July 27, 2017 16:50

dnephin force-pushed the test-integration-api branch from cad56e4 to 1b7ffb9 Compare July 27, 2017 16:53

dnephin mentioned this pull request Aug 4, 2017

[Proposal] Document testing guidelines #34407

Merged

tiborvass added status/2-code-review and removed status/1-design-review labels Aug 8, 2017

tiborvass reviewed Aug 8, 2017

View reviewed changes

mlaventure approved these changes Aug 9, 2017

View reviewed changes

vdemeester and others added 6 commits August 9, 2017 11:02

Remove test-integration-cli and references to it.

bc82b13

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Add integration suite

de46fb7

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Cleanup test output, and binaries.

4308e88

Signed-off-by: Daniel Nephin <dnephin@docker.com>

Protect images in the integration/ suite

5fa134b

Signed-off-by: Daniel Nephin <dnephin@docker.com>

move api test client setup to a package.

c66c044

Signed-off-by: Daniel Nephin <dnephin@docker.com>

dnephin force-pushed the test-integration-api branch from 1b7ffb9 to e593b72 Compare August 9, 2017 15:03

Fix exclude list in make.ps1

e593b72

Signed-off-by: Daniel Nephin <dnephin@docker.com>

tiborvass merged commit f34e4d2 into moby:master Aug 11, 2017

thaJeztah added the area/testing label Aug 14, 2017

dnephin deleted the test-integration-api branch August 21, 2017 16:20

This was referenced Aug 22, 2017

Replace integration-cli suite with API test suite #32866

Open

Stop using deprecated SockRequest #33534

Merged

Conversation

dnephin commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Jul 7, 2017

Uh oh!

dnephin commented Jul 7, 2017

Uh oh!

thaJeztah commented Jul 10, 2017

Uh oh!

albers commented Jul 11, 2017

Uh oh!

dnephin commented Jul 12, 2017

Uh oh!

AkihiroSuda commented Jul 13, 2017

Uh oh!

dnephin commented Jul 13, 2017

Uh oh!

aaronlehmann 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!

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!

dnephin Jul 14, 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!

dnephin commented Jul 14, 2017

Uh oh!

vieux commented Aug 8, 2017

Uh oh!

tiborvass commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

mlaventure commented Aug 11, 2017

Uh oh!

tiborvass commented Aug 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

dnephin Jul 14, 2017 •

edited

Loading