Graphdriver test cleanups #2228
Conversation
os.MkdirTemp() already creates the directory. Signed-off-by: Han-Wen Nienhuys <[email protected]>
Introduce GetDriverNoCleanup to obtain an instance that should survive individual test cases. Signed-off-by: Han-Wen Nienhuys <[email protected]>
hanwen-flow
force-pushed
the
graphdriver
branch
from
7c30b8a to
1cd2a64
Compare
drivers/graphtest/graphtest_unix.go
Outdated
| os.RemoveAll(runroot) | ||
| os.RemoveAll(root) |
There was a problem hiding this comment.
I think the correct way is to call os.RemoveAll in the same function which calls MkdirTemp, i.e. newDriver.
In there, you can do something like
if t.Failed() || t.Skipped() {
os.RemoveAll...
}There was a problem hiding this comment.
Nah, that won't work.
In any case, it would be better to place the cleanup into the same function as the setup, and it might require some refactoring.
There was a problem hiding this comment.
Yes, this looks good and is actually working (I was wrong it won't), thanks!
In the default ('go test'), the /tmp/ is typically not on btrfs, and
running the test leaves the temp dirs in place.
Signed-off-by: Han-Wen Nienhuys <[email protected]>
Signed-off-by: Han-Wen Nienhuys <[email protected]>
hanwen-flow
force-pushed
the
graphdriver
branch
from
1cd2a64 to
193d890
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hanwen-flow, kolyshkin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
thanks! the 'tide' check says that it still needs the 'lgtm' label. |
These are a couple of things I saw when perusing the test code.