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

[Lucas Meneghel Rodrigues]

On Wed, Mar 22, 2017 at 7:05 PM Lukáš Doktor <ldoktor at redhat.com> 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.
>
> 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).
>

That's a better solution than what we currently have, and I personally had
to resort to the workarounds described above. +1.

So in summary, anything that lets you clean up resources created/used
during the test without manual intervention is a great thing to have.
Having to manually clean up resources is a fairly unpleasant experience,
and we should help the user with that to a reasonable extent.


> 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 --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20170323/e0f12805/attachment.htm>