Fix integration suite and propagate failures#34682
Conversation
|
Ah, the behavior of You should be able to do something like |
|
(For reference, the Bash docs about these settings are in https://www.gnu.org/software/bash/manual/bashref.html#index-set, for anyone who doesn't already have that bookmarked. 😇) |
|
@tianon thanks! I'll try the |
e26654b to
8d02d0b
Compare
|
confirmed |
|
Integration test build here: https://jenkins.dockerproject.org/job/docker-integration-tests/103/ |
|
LGTM |
| grep -vE '^(./integration$|./integration/util)')"} | ||
|
|
||
| run_test_integration() { | ||
| local flags="-test.v -test.timeout=${TIMEOUT} $TESTFLAGS" |
There was a problem hiding this comment.
Can we use a different variable for the new test suite?
Running:
TESTFLAGS='-check.f DockerSuite.TestBuild*' ./hack/make.sh binary test-integrationFails due to the new test-runner here.
There was a problem hiding this comment.
When running tests using the code from this PR and the code from docker/cli master branch, we have these failures:
INFO: Testing against a local daemon
flag provided but not defined: -check.f
Usage of ./test.main:
-httptest.serve string
if non-empty, httptest.NewServer serves on this address and blocks
-test.bench regexp
run only benchmarks matching regexp
...
There was a problem hiding this comment.
@seemethere @andrewhsu that is not related to this PR, that change is already in master.
We can definitely fix that problem, but I don't think we should block these fixes. I will open a new PR to fix that problem.
Instead of using a separate flag I think I'll just skip running the new suite entirely if the TESTFLAG contains -check.f. I think that's the expected behaviour because if you're providing -check.f then you're expecting to target a specific suite, that isn't in the integration/ packages.
On a related note, I don't think this integration is working out too well. We need a better CI interface or we will continue to run into problems like this. This integration is running a completely different command from what the moby/moby CI is running, so it will keep breaking. We could change the name of these suites at any time, or add new ones. We've already added new API suites that are not being run.
I think we need to adopt the e2e interface of running an image with a bind mounted socket, which should be a lot more reliable, especially if we run that image in moby/moby CI as well.
There was a problem hiding this comment.
We want to be able to parallelize them as well which is why we're using check.f in the first place to target specific suites to run.
Since integration and integration-cli are completely different test suites there should be no reason to force us to run both at the same time if we just want to run one, and having that behavior rely on a regex pattern in TESTFLAGS is an anti-pattern in my opinion. Wouldn't it be better to split them up again but still provide the common integration interface? Where I can run integration-cli or integration-e2e, or run both with integration?
It'd make it less of a headache later on when we're trying to parallelize it downstream for Docker CE build jobs.
There was a problem hiding this comment.
We want to be able to parallelize [the test suites]
I think that's great, but we should do that in moby/moby as well. If we try to run things from a different entrypoint it's going to keep breaking. We need to have the moby/moby CI and test-integration use the same entrypoint. Let's get the parallelization into the moby/moby CI.
Since integration and integration-cli are completely different test suites there should be no reason to force us to run both at the same time if we just want to run one
Yes, that's what I'd like to accomplish. We can do that with a single TESTFLAGS:
- run a suite in the legacy
integration-cli-TESTFLAGS=-check.f DockerSuite - run a package in the new
integration/-TESTFLAGS=-check.f None TEST_INTEGRATION_DIR=./integration/service
There was a problem hiding this comment.
TEST_INTEGRATION_DIR Also needs to be added to the Makefile in order to account for this functionality.
There was a problem hiding this comment.
Also running with that configuration results in:
$ make TESTFLAGS="-check.f None" TEST_INTEGRATION_DIR=./integration/service test-integration
...(truncated)
Running /go/src/github.com/docker/docker/integration/container
INFO: Testing against a local daemon
flag provided but not defined: -check.f
Usage of ./test.main:
...(truncated)
---> Making bundle: .integration-daemon-stop (in bundles/17.06.0-dev/test-integration)
Removing test suite binaries
make: *** [test-integration] Error 1
There was a problem hiding this comment.
Right, I still need to add the fix to skip the first suite if -check.f is set in TESTFLAGS, that's not done yet.
Good call on adding the environment var to the make file. I'll need to fix that here.
|
z seems to fail constantly, is this related to this PR ? |
|
I believe |
seemethere
left a comment
There was a problem hiding this comment.
This needs the fix for TESTFLAGS and an extra environment variable for TEST_INTEGRATION_DIRS added to the makefiles before we can add this in without breaking downstream test runners for Docker CE.
Failures from the integration suite were not propagating to the outter shell for some reason. Handle the failure with an if exit 1. Signed-off-by: Daniel Nephin <dnephin@docker.com>
|
Those 2 fixes are in |
8d02d0b to
96707bc
Compare
The new
./integrationsuite was failing silently. This PR fixes a couple problemsdefer setupTest(t)to fix the failure, by loading the frozen imagesExample failure here: https://jenkins.dockerproject.org/job/Docker-PRs/45163/consoleFull
The
TEST_INTEGRATION_DIRis to allow someone to run just a single package instead of every package.