Code review comment for lp:~salgado/storm/fix-timeout-tracer

On 27/06/12 21:31, Christopher Armstrong wrote:
> Review: Approve
>
> [1] I would prefer for the new tests in test_tracer (test_connection_commit_hook and test_connection_rollback_hook) to be blackbox instead of whitebox, since it doesn't look like that should be too hard (or at least *less* whiteboxy; something that actually invokes some connection logic and observes that the bug that you observed does not occur). Even if you don't change it, it looks like storm has a convention of using "test_wb_" for whitebox tests which you should use here.

Ok, I rewrote them. They're now in a separate TestCase since I'm calling
conn.execute()/etc against a real DB, so I'm not sure tests/tracer.py is
the right home for that. Also, I think I could simplify its
setUp/tearDown a bit by inheriting from DatabaseTest, if you think it's
worth it.

> [2] There are no tests for the changes to DebugTracer (and there is already a suite of tests for it, so it should be easy to add).

I completely missed that; just added a couple more tests there.

Thanks for the review!