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

Revision history for this message
Christopher Armstrong (radix) wrote :

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

[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).

Looks good!

review: Approve

« Back to merge proposal