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

integration-cli: Better test cleanup with defer #10909

Merged
merged 1 commit into from Feb 21, 2015
Merged

integration-cli: Better test cleanup with defer #10909

merged 1 commit into from Feb 21, 2015

Conversation

@ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Feb 20, 2015

This fixes a few misuses of deleteAllContainers() cleanup
method in integration-cli suite by moving call to the
beginning of the method and guaranteeing their execution
(including panics) with defers. Most calls were not actually
happening in case of t.Fatal.

Also added some forgotten cleanup calls while I'm at it.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @unclejack @duglin @tianon @jfrazelle

@ahmetb ahmetb changed the title Better test cleanup with defer integration-cli: Better test cleanup with defer Feb 20, 2015
@duglin
Copy link
Contributor

@duglin duglin commented Feb 20, 2015

I wonder if it makes sense to create a generic cleanup() func that will do things like erase all images and containers and then make all tests start with a defer cleanup(). Then when we want to add more cleanup its only one spot that needs to be modified.

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 20, 2015

@duglin pretty much right, but I'll leave this to another PR as this makes the job easy for that.

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Feb 20, 2015

Also, there is some predefined images for us - busybox. We can write helper which will remove all images apart from busybox. But that's for another PR too.

@@ -304,6 +305,8 @@ func TestPsListContainersFilterStatus(t *testing.T) {
// FIXME: this should test paused, but it makes things hang and its wonky
// this is because paused containers can't be controlled by signals
deleteAllContainers()
Copy link
Contributor

@LK4D4 LK4D4 Feb 20, 2015

looks like redundant now.

Copy link
Contributor Author

@ahmetb ahmetb Feb 20, 2015

@LK4D4 I'm not sure, this guy might have a reason to ask for preemptive deletion of call containers 'cause it would absolutely fail in case of a leak and it'd be hard to trace which test case has leaked a container.

Copy link
Contributor

@LK4D4 LK4D4 Feb 20, 2015

I can say this about all tests. We should fix our suite, not hack it.

Copy link
Contributor Author

@ahmetb ahmetb Feb 20, 2015

Yes. Though, this PR is not a hack, it's actually removing ~250 lines of potentially dead code. That deleteAllContainers() without defer is totally harmless and this specific test case is more vulnerable to leftover contention from other tests, that's why I am keeping the status quo.

Copy link
Contributor

@LK4D4 LK4D4 Feb 20, 2015

But that deleteAllContainers a hack. And it used only for this test, though another Ps tests is the same vulnerable to leaked containers, so I don't see a point for keeping it.

Copy link
Contributor Author

@ahmetb ahmetb Feb 20, 2015

fixed.

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Feb 20, 2015

@ahmetalpbalkan Thank you so much.

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 20, 2015

@LK4D4 my pleasure. For deleteImages, we're doing a cleanup at the end and tests requiring an empty image list probably clean the env before they start. However we may care more about cleaning containers between test methods might be important as we sometimes use a hardcoded container name or port number. It'll increase execution time per test case.

Also the CI suite has a cleanup, so that serves the same purpose.

I'm now proposing a "webserver" image that shouldn't also be deleted like "busybox', in the meanwhile (#10893) so I am not really in for creating a deleteAllImagesExceptSome() route.

@tianon
Copy link
Member

@tianon tianon commented Feb 20, 2015

So, we're essentially saying we can't run any of our tests in parallel since each test might blow away resources in use by the others instead of just resources it individually creates?

@tianon
Copy link
Member

@tianon tianon commented Feb 20, 2015

It's less error-prone overall, but also a lot less flexible IMO. 😕

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Feb 20, 2015

@tianon I don't understand what you saying :) It was always like this.

@ahmetb
Copy link
Contributor Author

@ahmetb ahmetb commented Feb 20, 2015

@tianon we can add some check like DCO validaton and force every test to begin with "defer cleanup()" in the future.

@tianon
Copy link
Member

@tianon tianon commented Feb 20, 2015

@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Feb 20, 2015

I'm not sure that we can run some tests parallel at all. Those tests which requires some clean state, which can't be cleaned by hands(events for example) should use separate daemon.

This fixes a few misuses of `deleteAllContainers()` cleanup
method in integration-cli suite by moving call to the
beginning of the method and guaranteeing their execution
(including panics) with `defer`s.

Also added some forgotten cleanup calls while I'm at it.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@LK4D4
Copy link
Contributor

@LK4D4 LK4D4 commented Feb 21, 2015

LGTM

1 similar comment
@jessfraz
Copy link
Contributor

@jessfraz jessfraz commented Feb 21, 2015

LGTM

jessfraz pushed a commit that referenced this issue Feb 21, 2015
integration-cli: Better test cleanup with defer
@jessfraz jessfraz merged commit b062ef0 into moby:master Feb 21, 2015
1 check passed
@ahmetb ahmetb deleted the run_test-defer branch Feb 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants