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

[Fam Zheng <famz@redhat.com>]

On Thu, 08/24 19:04, Stefan Hajnoczi wrote:
> 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.

I think this shortcoming can be solved with a clean up list ("all problems in
computer science can be solved by another level of indirection"):

_clean_up_list = set()
def _clean_up_handler():
    for i in _clean_up_list:
        try:
            i()
        except:
            pass

atexit.register(_clean_up_handler)

class VM(...):

    def launch():
        ...
        _clean_up_list.add(self.launch)

    def shutdown():
        _clean_up_list.remove(self.launch)
        ...

> 
> 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.

Good point, ./check can even invoke scripts with scratch as their working
directory.

Fam

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