Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

tests: do cleanUp() always in the end #1423

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

WeiZhang555
Copy link
Member

Fixes: #1422

Detect failing test case:

....
=== RUN   TestEnterContainerFailingContNotStarted
--- PASS: TestEnterContainerFailingContNotStarted (0.01s)
=== RUN   TestEnterContainer
--- FAIL: TestEnterContainer (0.00s)
 Error Trace: sandbox_test.go:1154
 Error:      	Expected value not to be nil.
 Messages:   	Entering non-running container should fail
 Error Trace: sandbox_test.go:1157
 Error:      	Expected nil, but got: &errors.errorString{s:"Can not
move from running to running"}
 Messages:   	Failed to start sandbox: Can not move from running to
running
FAIL

TestEnterContainerFailingContNotStarted calls cleanUp at function
begging but it doesn't clean its garbage after it ends.
TestEnterContainer only call cleanUp in the end but it doesn't do
cleanUp in the begging, that gives first test case a chance to impact
latter one.

This commit modifies all the test cases, let them all do the cleanUp()
in the end.

The policy here is: "everyone needs to take their garbage away when they
leave" :)

Signed-off-by: Wei Zhang [email protected]

Fixes: kata-containers#1422

Detect failing test case:

```
....
=== RUN   TestEnterContainerFailingContNotStarted
--- PASS: TestEnterContainerFailingContNotStarted (0.01s)
=== RUN   TestEnterContainer
--- FAIL: TestEnterContainer (0.00s)
 Error Trace: sandbox_test.go:1154
 Error:      	Expected value not to be nil.
 Messages:   	Entering non-running container should fail
 Error Trace: sandbox_test.go:1157
 Error:      	Expected nil, but got: &errors.errorString{s:"Can not
move from running to running"}
 Messages:   	Failed to start sandbox: Can not move from running to
running
FAIL
```

`TestEnterContainerFailingContNotStarted` calls `cleanUp` at function
begging but it doesn't clean its garbage after it ends.
`TestEnterContainer` only call `cleanUp` in the end but it doesn't do
cleanUp in the begging, that gives first test case a chance to impact
latter one.

This commit modifies all the test cases, let them all do the cleanUp()
in the end.

The policy here is: "everyone needs to take their garbage away when they
leave" :)

Signed-off-by: Wei Zhang <[email protected]>
@WeiZhang555
Copy link
Member Author

WeiZhang555 commented Mar 26, 2019

The policy here is: "everyone needs to take their garbage away when they leave" :)

So if we treat the unit test as a "clean room", some test case enter the room, then do something, it should take its garbage away(defer cleanUp()) from the room when it leaves.
If the room isn't clean when one test case enters, we should blame the last one, and the new test case can choose one extra cleanUp() in the begging if it likes, I'm not against this but it's not truly necessary :-) .
That is something like:

func Testxxxx(){
cleanUp()
defer cleanUp()
....
}

@WeiZhang555
Copy link
Member Author

Another issue is Jenkins doesn't run the failing test case at all, see kata-containers/tests#1355

@WeiZhang555
Copy link
Member Author

/test

@WeiZhang555
Copy link
Member Author

/retest

@bergwolf bergwolf merged commit 432eda0 into kata-containers:master Apr 2, 2019
@WeiZhang555 WeiZhang555 deleted the fix-failing-test branch April 2, 2019 11:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants