Raise SetupError *after* handling of exceptions raised during _setUp() has completed

[freeekanayaka]

Tweak error handling logic in Fixture.setUp() to avoid raising SetupError while also handling the original exception from _setUp(). This avoids Python 3 attaching the original error as context attribute of the SetupError instance, which in turn would make the output of testtools.content.TracebackContent unnecessarily redundant and somehow confusing.

---

This change is

[freeekanayaka]

I actually have a kind of related question, that maybe Robert knows the answer of (and that might affect this branch too).

As far as I understand, the role of SetupError is to hold fixture details (added via the addDetail API), so consumers of fixture.setUp() can access not only the original exception but also the details that the fixture has published.

One important consumer of SetupError is arguably testtools.TestCase.useFixture(), which indeed catches MultipleExceptions and merges the fixture details into the TestCase details. Relevant code is:

        try:
            fixture.setUp()
        except MultipleExceptions as e:
            if (fixtures is not None and
                    e.args[-1][0] is fixtures.fixture.SetupError):
                gather_details(e.args[-1][1].args[0], self.getDetails())
            raise

See testcase.py at line 730 in testtools 2.2.0.

In case of a failure in _setUp(), the above code is enough in order to have the fixture details printed out by the test runner.

However testtools also include the SetupError exception itself as detail (with testtoools.content.TracebackContent):

    def _got_user_exception(self, exc_info, tb_label='traceback'):
        if exc_info[0] is MultipleExceptions:
            for sub_exc_info in exc_info[1].args:
                self._got_user_exception(sub_exc_info, tb_label)
            return self.exception_caught

See runtest.py at line 207 in testtools 2.2.0.

Now this means that even with the fix in this branch (which removes redundant SetUp context for Python 3), the typical output of a test run with a setUp failure some details would look like:

Tests running...
======================================================================
ERROR: test_foo.FooTest.test_foo
----------------------------------------------------------------------
foo: {{{bar}}}
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/fixtures/fixture.py", line 208, in setUp
    raise SetupError(details)
fixtures.fixture.SetupError: {'foo': <Content type=text/plain; charset="utf8", value=b'bar'>}
}}}

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/fixtures/fixture.py", line 197, in setUp
    self._setUp()
  File "/home/ubuntu/test_foo.py", line 48, in _setUp
    raise RuntimeError("boom")
RuntimeError: boom

Notice that the details are kind of duplicated, because they show up both as regular test case details and as "message" of the SetupError traceback being rendered.

My question is, is there any point in testtools adding SetupError as detail? Should we also change testtools to not do that, and rather only add the original error as detail?

[rbtcollins]

So there is a bug in frame capturing - its file on testtools I think, or perhaps fixtures. But to my mind what we have to cope with is:

• some N exceptions thrown from within a DAG of fixtures

We can in Python3 show either context or cause, but thats strictly linear. We either have to coerce all the exceptions into such a structure, or do what we're doing today and reflect a list of exceptions.

Given a set of exceptions with a common callstack, what I'd like to do is show the common prefix, then all the discrete child exceptions with the frames from that point down. The point where setUp / useFixture is initially called is a common prefix but not the longest common prefix.

Doing the work to do that would be awesome.

Separately, have we made you a committer yet?

[rbtcollins]

As for this specific change; I wonder if perhaps the thing you found confusing was seeing multiple exceptions raised when only a single exception triggered the situation. E.g. should we be looking for >1 exception before we raise MultipleExceptions, and not raising SetupError explicitly in that case? I think not, because then we can't look for it to pull the details out. That said we know when we raise SetupError that we've definitely had another exception occur, so perhaps not putting the SetupError backtrace into the error report would make sense as a step towards the more general thing I described above.

[freeekanayaka]

On 6 November 2016 at 02:42, Robert Collins [email protected]
wrote:

So there is a bug in frame capturing - its file on testtools I think, or
perhaps fixtures. But to my mind what we have to cope with is:

• some N exceptions thrown from within a DAG of fixtures

We can in Python3 show either context or cause, but thats strictly linear.
We either have to coerce all the exceptions into such a structure, or do
what we're doing today and reflect a list of exceptions.

Given a set of exceptions with a common callstack, what I'd like to do is
show the common prefix, then all the discrete child exceptions with the
frames from that point down. The point where setUp / useFixture is
initially called is a common prefix but not the longest common prefix.

Doing the work to do that would be awesome.

You're right. I didn't think about the case of multiple fixtures with
nested/independent failures (DAG style).

I'll think about that when I have some time.

Separately, have we made you a committer yet?

No, but would be awesome! :)

Also on a separate note, I have being working on a package that provides a
few convenient fixtures when writing tests for code that interact with
system resources (users, groups, filesystem, etc):

https://github.com/freeekanayaka/systemfixtures

I was wondering if it'd make sense to move this repository under
github.com/testing-cabal, for visibility and shared
maintainership/development.

Perhaps it'd be also nice to the have a section in fixture's README.rst
pointing to the home page of known domain-specific fixture implementations,
such as:

zope_fixtures
rabbitfixture
postgresfixture
systemfixtures

Thoughts?

[freeekanayaka]

On 6 November 2016 at 05:33, Robert Collins [email protected]
wrote:

As for this specific change; I wonder if perhaps the thing you found
confusing was seeing multiple exceptions raised when only a single
exception triggered the situation.

Yes, that's what I found confusing. At first I thought there was something
odd going on in my code.

E.g. should we be looking for >1 exception before we raise
MultipleExceptions, and not raising SetupError explicitly in that case?

I think not, because then we can't look for it to pull the details out.
That said we know when we raise SetupError that we've definitely had
another exception occur, so perhaps not putting the SetupError backtrace
into the error report would make sense as a step towards the more general
thing I described above.

Right. It feels a sensible first step.