Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers

[Stefan Hajnoczi]

On Thu, Aug 24, 2017 at 04:38:43PM +0800, Fam Zheng wrote:
> On Thu, 08/24 08:21, Stefan Hajnoczi wrote:
> > Tests should declare resources upfront in a with statement.  Resources are
> > automatically cleaned up whether the test passes or fails:
> > 
> >   with FilePath('test.img') as img_path,
> >        VM() as vm:
> >       ...test...
> >   # img_path is deleted and vm is shut down automatically
> 
> Looks good but still requires test writers to learn and remember to use 
> FilePath
> and with.

You cannot forget to use FilePath() unless you love typing at
os.path.join(iotests.test_dir, 'test.img').  It's much better than open
coding filename generation!

> These are still boilerplates.  Here goes my personal oppinion, so may
> not be plausible:
> 
> - For VM() maybe add an atexit in the launch() method also makes sure the VM 
> is
>   eventually terminated.
> 
>   This means vm.shutdown() is still needed in tearDown() if there are multiple
>   test methods and each of them expects a clean state, but that is probably
>   still less typing (and also indenting) than the with approach, and also easy
>   to remember (otherwise a test will fail).

I looked into atexit before going this route.  atexit does not have an
unregister() API in Python 2.  This makes it ugly to use because some
tests do not want the resource to remain for the duration of the
process.

A related point is that the Python objects used by atexit handlers live
until the end of the process.  They cannot be garbage collected because
the atexit handler still has a reference to them.

The with statement's identation is annoying for straightforward scripts.
More complex tests use functions anyway, so the indentation doesn't
matter there - it can be hidden by a parent function or even a
decorator.

> - For scratch how about adding atexit in iotests.main to clean up everything 
> in
>   the scratch directory? The rationale is similar to above.

If we decide to clear out TEST_DIR then it should be done in ./check,
not by iotests.py, so that all tests get the same file cleanup behavior,
regardless of the language they are written in.

Also, we can then chdir(iotests.test_dir) so filename generation isn't
necessary at all.  Tests can simply use 'test.img'.  I guess there may
be some cases where absolute paths are necessary, but for the most part
this would be a win.

Kevin may have an opinion on whether TEST_DIR should be cleared out or
not.

Stefan