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

[Ademar Reis]

On Thu, Mar 23, 2017 at 07:10:23AM +0100, Lukáš Doktor 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.
> > 
> > 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...

My understanding is that there are cases where the skip happens
as a decision from the test runner, such as during job replay
and/or timeout. The behavior of such a skip should be exactly the
same as when skipIf() is used.

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

Right, I see no reason why not.

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

Ditto.

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

Agree. What I had in mind was the double ctrl+c.

Thanks.
   - Ademar

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




-- 
Ademar Reis
Red Hat

^[:wq!