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

[Ademar Reis]

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.

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

 skip (test runner decision)
 skipIf() (decorator)
   - Test is completely skipped by the test runner
   - Test status = SKIP
   - Neither setUp() nor tearDown() are run.

 Uncaught error/exception
   - test status = ERROR
   - tearDown() is run

 self.cancel()
   - Can be called during any stage of the test execution.
     Results in test status = CANCEL.
   - tearDown() is run.

 Test is interrupted (ctrl+C or timeout)
   - test status = INTERRUPT
   - tearDown() is not run

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




-- 
Ademar Reis
Red Hat

^[:wq!