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

unit-test: cleaning up stale files under /tmp #2399

Merged
merged 3 commits into from
Jan 20, 2020

Conversation

Pennyzct
Copy link
Contributor

@Pennyzct Pennyzct commented Jan 17, 2020

I've found a few unit tests which were generating stale files under /tmp

- Using defer in TestMain.
os.Exit will skip all deferred instructions.
So we should reconstruct TestMain to leave all setup-related code in setup(), and all cleanup-related code in shutdown().

- Missing deleting what ioutil.TempDir() creates
Normally, ioutil.TempDir will create a new temporary dir under /tmp.

- Missing deleting what ioutil.TempFile() creates
ioutil.TempFile creates a new temporary file in the directory /tmp.
It is the caller's responsibility to remove the file when no longer needed.

os.Exit will skip all deferred instructions.
So we should reconstruct TestMain to leave all setup-related
code in setup(), and all cleanup-related code in shutdown().

Fixes: kata-containers#2398

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Normally, ioutil.TempDir will create a new temporary
dir under /tmp.
And we should do cleaning up after ioutil.TempDir().

Fixes: kata-containers#2398

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
@Pennyzct
Copy link
Contributor Author

/test

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #2399 into master will increase coverage by 0.92%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2399      +/-   ##
==========================================
+ Coverage   50.07%   50.99%   +0.92%     
==========================================
  Files         112      112              
  Lines       16178    16148      -30     
==========================================
+ Hits         8101     8235     +134     
+ Misses       7063     6897     -166     
- Partials     1014     1016       +2

ioutil.TempFile creates a new temporary file in the directory dir.
It is the caller's responsibility to remove the file
when no longer needed.

Fixes: kata-containers#2398

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Pennyzct - lgtm - nice work!

setup()
rt := m.Run()
shutdown()
os.Exit(rt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOI, in case you have never seen this - we check for no uses of os.Exit() in the main codebase in the CI:
https://github.com/kata-containers/runtime/blob/master/.ci/go-no-os-exit.sh

(I think that is a feature we've had for a very very long time and is little seen, so just raising here for general awareness).
but, maybe that does not apply to test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi~ @grahamwhaley Thanks for the notification. ;).
I'm afraid that it's not applicable for go unit test code, referring from Go.Doc:

TestMain runs in the main goroutine and can do whatever setup and teardown 
is necessary around a call to m.Run. 
It should then call os.Exit with the result of m.Run.

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vijaydhanraj
Copy link
Contributor

Thanks @Pennyzct . acrn test changes look fine to me.

@Pennyzct
Copy link
Contributor Author

/test

@Pennyzct
Copy link
Contributor Author

firecracker CI failed irrelevantly. All the other are green~~~

@devimc devimc merged commit df802cc into kata-containers:master Jan 20, 2020
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.

5 participants