Questions after testtools merge

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jonathan Lange wrote:
> On Fri, Jan 1, 2010 at 3:27 AM, John Arbash Meinel
> <john at arbash-meinel.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> Vincent Ladeuil wrote:
>>>>>>>> "jam" == John Arbash Meinel <john at arbash-meinel.com> writes:
>>> <snip/>
>>>
>>>     jam> So I'm pretty sure that always calling tearDown *was* something that we
>>>     jam> had to explicitly put into our .run() method. And it certainly could
>>>     jam> have been lost in the transition.
>>>
>>> I'd be surprised if that was the case but if it's verified it's indeed a
>>> very serious bug.
>>>
>>> I'm surprised that no test catches that though...
>> IIRC stock unittest does not call tearDown if setUp does not complete.
>>
> 
> Correct. testtools follows its lead.
> 
>> Looking at the code in testtools.runtest there is _run_core which has:
>>
>>        if self.exception_caught == self._run_user(self.case._run_setup,
>>            self.result):
>>            # Don't run the test method if we failed getting here.
>>            self.case._runCleanups(self.result)
>>            return
>>
>> So *if* an exception is caught during setup, it runs cleanups, but not
>> teardowns.
> 
> Yes, this is a deliberate design decision -- we felt that there should
> be a way of guaranteeing that a cleanup gets run.


I would mention that this is a regression for the *bzrlib* test suite.
Which always ran tearDown even if setUp did not complete. (As evidenced
by Martin's test suite failures after the merge.)

> 
>> Because later on we have:
>>
>>        try:
>>            if self.exception_caught == self._run_user(
>>                self.case._run_test_method, self.result):
>>                failed = True
>>        finally:
>>            try:
>>                if self.exception_caught == self._run_user(
>>                    self.case._run_teardown, self.result):
>>                    failed = True
>>            finally:
>>                try:
>>                    if not self._run_user(
>>                        self.case._runCleanups, self.result):
>>                        failed = True
>>                finally:
>>                    if not failed:
>>                        self.result.addSuccess(self.case,
>>                            details=self.case.getDetails())
>>
>> Also, I think we have a bug, looking at addCleanups, it says:
>>    def addCleanup(self, function, *arguments, **keywordArguments):
>>        """Add a cleanup function to be called before tearDown.
>>
>>
>> However, if I read the above correctly, teardown is clearly called
>> *before* cleanups. Which doesn't quite fit with the fact that setUp is
>> called before addCleanup, etc.
>>
> 
> The docstring is definitely wrong. Given that you had to delve so
> deeply into the code, it's also inadequate. I'll fix this now.
> 
> jml

Well, I believe I generally consult the code first anyway. Docstrings
have a habit of going stale (case in point).

As for whether tearDown comes before or after cleanups... I favor it
coming after, but arguably it could go either. Most resources work very
well in the LIFO that addCleanup provides, and it is unclear how that
interacts with tearDown. (You can't put the child classes tearDown
before the parent classes cleanups but after its tearDown, etc.)

I would say a reasonable fix for *bzrlib* is to just deprecate tearDown
entirely. (put it in our base TestCase), and then fix everything to use
addCleanup.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAks+v4UACgkQJdeBCYSNAAPAxACeNBXF9kUeqPUBxR514tKUFz2a
Xo8AoLqJfoS9bk3jsu+nm+7szp7W9Xys
=OfYG
-----END PGP SIGNATURE-----