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

[Lukáš Doktor]

Dne 23.3.2017 v 10:18 Amador Pahim napsal(a):

On Thu, Mar 23, 2017 at 7:10 AM, Lukáš Doktor <ldok...@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).

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.

signature.asc
Description: OpenPGP digital signature