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 >