[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context m
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers |
Date: |
Fri, 25 Aug 2017 15:32:29 +0800 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
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
>
- [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers, Stefan Hajnoczi, 2017/08/24
- [Qemu-devel] [PATCH 2/3] iotests.py: add FilePath context manager, Stefan Hajnoczi, 2017/08/24
- [Qemu-devel] [PATCH 1/3] qemu.py: make VM() a context manager, Stefan Hajnoczi, 2017/08/24
- [Qemu-devel] [PATCH 3/3] qemu-iotests: use context managers for resource cleanup in 194, Stefan Hajnoczi, 2017/08/24
- Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers, Fam Zheng, 2017/08/24
- Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers, Markus Armbruster, 2017/08/28
Re: [Qemu-devel] [PATCH 0/3] iotests: clean up resources using context managers, Stefan Hajnoczi, 2017/08/31