Code review comment for lp:~wallyworld/launchpad/codehost-error-trackback-leak

On 06/12/10 14:41, Tim Penhey wrote:
> Review: Needs Fixing
> +1 on Graham's comments.
>
> Have you tried this interactively to see what you get?
>

Yes. But that was before the changes to trunk (see below). I can re-do
the interactive test to double check its behaviour.

> I'm wondering if we really need the print to sys.stderr. I'm not sure
> though.
>

I didn't need the explicit print to stderr when the branch was started.
Then some changes were made in trunk to the twisted infrastructure and
log.err no longer echoed to stderr after trunk was merged into the branch.

> Someone told me we had a monkeypatch helper method that added the cleanup
> code. If that isn't the case, you could still to the cleanup with addCleanup
> rather than adding a tearDown method.
>

The implementation used mirrors what is done already in the current
tests for this part of the code. If I am to change the implementation as
per the review recommendations, the other existing tests should also be
changed to maintain consistency. I thought it best to use code similar
to what was already there rather than change unrelated code and increase
the diff size etc. I'll look into making the required changes though.

> # Before changing the stderr, do this.
> self.addCleanup(setattr, sys, 'stderr', sys.stderr)
>
> The test class has self.oopses that records oopses generated by the global
> error utility, and should be used insetad of getLastOopsReport().
> getLastOoopsReport can fail in unexpected ways if the oops was generated on
> one side of midnight, and the method is called just after midnight. This was
> a cause of quite some pain and debugging, and is the reason that the testcase
> has self.oopses and records them based on generated events.
>

Ok. I'll look at this approach. Thanks.
I'm still concerned about the need to generate a 2nd oops but can't see
another way around it. I'd be happy to be shown a better approach.