Merge lp://staging/~mbp/rabbitfixture/rabbit-startup into lp://staging/rabbitfixture

This looks like it'll work, but it's ugly:

- It relies upon the fact that fixture clean-ups are not run when
  setUp fails.

- When these errors appear as part of a test run they're not
  particularly useful (useless in ec2 if the instance has already
  terminated) and could be misleading (for example, someone might
  believe that they have to capture the log when its contents are
  actually already in a test detail).

I still think that it would be much better to fix runlaunchpad.py
rather than muddy things in rabbitfixture. So, approved, but it's a
+0.1 rather than a +1, and I'd want to revert these changes when
runlaunchpad is fixed.

Fixture cleanup when setUp fails should really be invoked - the code
is careful to make sure that that is reasonable, and if we're not
cleaning up, we'll be leaving crud behind.

(e.g. the runlaunchpad code should have a try: finally:). If this
branch is dependent on runlaunchpad *having* that defect, I think we
should reject it.

On 25 October 2011 21:21, Gavin Panella <email address hidden> wrote:
> Review: Approve
>
> This looks like it'll work, but it's ugly:
>
> - It relies upon the fact that fixture clean-ups are not run when
>  setUp fails.
>
> - When these errors appear as part of a test run they're not
>  particularly useful (useless in ec2 if the instance has already
>  terminated) and could be misleading (for example, someone might
>  believe that they have to capture the log when its contents are
>  actually already in a test detail).
>
> I still think that it would be much better to fix runlaunchpad.py
> rather than muddy things in rabbitfixture. So, approved, but it's a
> +0.1 rather than a +1, and I'd want to revert these changes when
> runlaunchpad is fixed.

I don't understand. With this patch, it raises an exception in just
the same cases as before, but now the exception has a more useful
message. How does that change the interaction with tearDown, and why
would you ever want to revert it?

I take your point about ec2 and the whole disk disappearing, and
perhaps there printing the log would be better.

The log is put into a temporary directory that is ordinarily erased
during fixture clean-up. At the moment fixture clean-ups are not run
if setUp() raises an exception, but, as Rob says, clean-ups should
really be run. When python-fixtures adopts that behaviour, telling the
user the location of the log file will be pointless because it will
have been erased by the time they see the message.

I see, thanks.

The change isn't in python-fixtures so much as in callers of setUp -
e.g. TestCase.useFixture() should handle exceptions by calling
cleanUp(). fixtures will need to do this too, for its implementation
of useFixture. runlaunchpad or whatever is calling setUp is the
immediate place to do this though.

Can you give me a clue how this ought to be handled in runlaunchpad?

> Can you give me a clue how this ought to be handled in runlaunchpad?

In runlaunchpad.py there's a start_launchpad() function. It starts up
services here:

    with nested(*services):

Each member of services is a Service, which is subclass of Fixture,
which means it responds to getDetails().

Something like the following might work:

    # This is for inspiration; it's not necessarily right.
    from testtools.testresult.real import _details_to_str

    try:
        with nested(*services):
            ...
    except Exception:
        for service in services:
            print _details_to_str(service.getDetails())
        raise

Fixtures are also used by some layers that just call setUp() and
cleanUp() directly, and keep no track of details. I don't really know
how to address that yet, but it can wait.

based on Gavin's +0.1 I'm going to merge this, even though it's not a total fix.

> > Can you give me a clue how this ought to be handled in runlaunchpad?
>
> In runlaunchpad.py there's a start_launchpad() function. It starts up
> services here:
>
> with nested(*services):
>
> Each member of services is a Service, which is subclass of Fixture,
> which means it responds to getDetails().
>
> Something like the following might work:
>
> # This is for inspiration; it's not necessarily right.
> from testtools.testresult.real import _details_to_str
>
> try:
> with nested(*services):
> ...
> except Exception:
> for service in services:
> print _details_to_str(service.getDetails())
> raise
>
> Fixtures are also used by some layers that just call setUp() and
> cleanUp() directly, and keep no track of details. I don't really know
> how to address that yet, but it can wait.

Ok, and so then you'd also want me to attach the contents of the log file in to the service? Why would that be better than putting them in to the exception?

On Mon, Oct 31, 2011 at 8:46 PM, Martin Pool <email address hidden> wrote:
>
> Ok, and so then you'd also want me to attach the contents of the log file in to the service?  Why would that be better than putting them in to the exception?

Because when something else fails, the exception raised won't have the
log files. But they may still be useful.

On 31 October 2011 18:52, Robert Collins <email address hidden> wrote:
> On Mon, Oct 31, 2011 at 8:46 PM, Martin Pool <email address hidden> wrote:
>>
>> Ok, and so then you'd also want me to attach the contents of the log file in to the service?  Why would that be better than putting them in to the exception?
>
> Because when something else fails, the exception raised won't have the
> log files. But they may still be useful.

That makes sense. I see they are even read in at the moment.

I'm not planning to pursue this any more at the moment, but I'll just mention that I think the thing of putting it into runlaunchpad won't work, as I hit this while running tests. It is more like we need to capture the details from failures during useFixture. But python-fixtures trunk at least does seem to do that. So, I'm not sure.

... for the record, I did actually hit this again and ended up doing https://code.launchpad.net/~mbp/launchpad/892427-service-failure/+merge/82766 to put Gavin's suggestion in to lp.