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

[Amador Pahim]

On Thu, Mar 23, 2017 at 11:04 AM, Lukáš Doktor <ldoktor at redhat.com> wrote:
> 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).

Ok. So, please create a card for it.

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