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

Revision history for this message
Tim Penhey (thumper) wrote :

+1 on Graham's comments.

Have you tried this interactively to see what you get?

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

I found the variable name 'tb' a little jarring. It caused me to pause to
work out what it might be. Longer more explicit names are good. Perhaps
'traceback'?

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.

  # 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.

review: Needs Fixing

« Back to merge proposal