[Avocado-devel] miniRFC: Call `tearDown` after `setUp` failure

[Amador Pahim]

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