[Avocado-devel] miniRFC: Call `tearDown` after `setUp` failure
Lukáš Doktor
ldoktor at redhat.com
Thu Mar 23 10:04:08 UTC 2017
Dne 23.3.2017 v 10:18 Amador Pahim napsal(a):
> On Thu, Mar 23, 2017 at 7:10 AM, Lukáš Doktor <ldoktor at redhat.com> wrote:
>> Dne 22.3.2017 v 20:00 Ademar Reis napsal(a):
>>>
>>> On Wed, Mar 22, 2017 at 07:05:48PM +0100, Lukáš Doktor wrote:
>>>>
>>>> Hello guys,
>>>>
>>>> I remember early in development we decided to abort the execution when
>>>> `setUp` fails without executing `tearDown`, but over the time I keep
>>>> thinking about it and I don't think it's optimal. My main reason is to
>>>> simplify `setUp` code, let me demonstrate it on an example.
>>>
>>>
>>> So you're basically proposing that setUp() and tearDown() are
>>> always executed "together". In other words, if setUp() is called,
>>> then tearDown() will also be called, even if setUp() resulted in
>>> error, exception or cancellation.
>
> Yep, agreed. This makes a lot of sense and will simplify our code. Let
> me include this in the PR I'm working to adjust SKIP and CANCEL
> behavior, since this is pretty much related.
>
I'd prefer merging the SKIP/CANCEL PR first as it corrects behavior
introduced in this sprint. The new change of behavior could be brought
up during the public sprint review as it requires modifications to tests
(the assumption that tearDown is not executed can lead to
double-failures, which is harmless, but could surprise people).
Lukáš
>>>
>>> I think it makes perfect sense.
>>>
>>> The only problem I see is in the case of test interruption
>>> (ctrl+c or timeout). I think this will have to be the only
>>> exception. In case of interruption, we can't wait to run
>>> tearDown().
>>>
>>> In summary, the behavior would be:
>>>
>>> normal execution
>>> - test status = PASS | FAIL | WARN
>>> - setUp() is run
>>> - tearDown() is run
>>
>> yep
>>
>>>
>>> skip (test runner decision)
>>
>> this one is tricky, but we can refine it when we have dep solver...
>>
>>> skipIf() (decorator)
>>> - Test is completely skipped by the test runner
>>> - Test status = SKIP
>>> - Neither setUp() nor tearDown() are run.
>>
>> yep
>>
>>>
>>> Uncaught error/exception
>>> - test status = ERROR
>>> - tearDown() is run
>>
>> This is the same behavior as PASS | FAIL | WARN, right?
>>
>>>
>>> self.cancel()
>>> - Can be called during any stage of the test execution.
>>> Results in test status = CANCEL.
>>> - tearDown() is run.
>>
>> Again, the same behavior as PASS | FAIL | WARN.
>>
>>>
>>> Test is interrupted (ctrl+C or timeout)
>>> - test status = INTERRUPT
>>> - tearDown() is not run
>>
>> On timeout I agree, on single ctrl+c we wait till the test is over. I think
>> it'd make sense to let the tearDown finish as well unless the double ctrl+c
>> is pressed, what do you think?
>>
>> Lukáš
>>
>>
>>>
>>> Thanks.
>>> - Ademar
>>>
>>>>
>>>> Currently when you (safely) want to get a few remote servers, nfs share
>>>> and
>>>> local service, you have to:
>>>>
>>>> class MyTest(test):
>>>> def setUp(self):
>>>> self.nfs_share = None
>>>> self.remote_servers = []
>>>> self.service = None
>>>> try:
>>>> for addr in self.params.get("remote_servers"):
>>>> self.remote_servers.append(login(addr))
>>>> self.nfs_share = get_nfs_share()
>>>> self.service = get_service()
>>>> except:
>>>> for server in self.remote_servers:
>>>> server.close()
>>>> if self.nfs_share:
>>>> self.nfs_share.close()
>>>> if self.service:
>>>> self.service.stop()
>>>> raise
>>>>
>>>> def tearDown(self):
>>>> if self.nfs_share:
>>>> self.nfs_share.close()
>>>> for server in self.remote_servers:
>>>> server.close()
>>>> if self.service:
>>>> self.service.stop()
>>>>
>>>>
>>>> But if the tearDown was also executed, you'd simply write:
>>>>
>>>> class MyTest(test):
>>>> def setUp(self):
>>>> self.nfs_share = None
>>>> self.remote_servers = []
>>>> self.service = None
>>>>
>>>> for addr in self.params.get("remote_servers"):
>>>> self.remote_servers.append(login(addr))
>>>> self.nfs_share = get_nfs_share()
>>>> self.service = get_service()
>>>>
>>>> def tearDown(self):
>>>> if self.nfs_share:
>>>> self.nfs_share.close()
>>>> for server in self.remote_servers:
>>>> server.close()
>>>> if self.service:
>>>> self.service.stop()
>>>>
>>>> As you can see the current solution requires catching exceptions and
>>>> basically writing the tearDown twice. Yes, properly written tearDown
>>>> could
>>>> be manually executed from the `setUp`, but that looks a bit odd and my
>>>> experience is that people usually just write:
>>>>
>>>> class MyTest(test):
>>>> def setUp(self):
>>>> self.remote_servers = []
>>>> for addr in self.params.get("remote_servers"):
>>>> self.remote_servers.append(login(addr))
>>>> self.nfs_share = get_nfs_share()
>>>> self.service = get_service()
>>>>
>>>> def tearDown(self):
>>>> self.nfs_share.close()
>>>> for server in self.remote_servers:
>>>> server.close()
>>>> self.service.stop()
>>>>
>>>> which usually works but when the `get_nfs_share` fails, the
>>>> remote_servers
>>>> are not cleaned, which might spoil the following tests (as they might be
>>>> persistent, eg. via aexpect).
>>>>
>>>> Kind regards,
>>>> Lukáš
>>>>
>>>> PS: Yes, the tearDown is unsafe as when `nfs_share.close()` fails the
>>>> rest
>>>> is not cleaned up. This is just a demonstration and the proper tearDown
>>>> and
>>>> a proper setUp for the current behavior would be way more complex.
>>>>
>>>
>>>
>>>
>>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 502 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20170323/b2bbb636/attachment.sig>
More information about the Avocado-devel
mailing list