testtools-dev team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Review: Approve
I think in _clean's docstring its worth mentioning that at least *a* iteration happens. Or perhaps not.

run_with_log_observers looks like a fixture just wanting to break out. I think it would be cleaner and reusable as two fixtures:
IsolateTwistedLogging
UseTwistedLogObserver

Content(UTF8_TEXT, lambda: [full_log.getvalue()])) will be cleaner as
Content(UTF8_TEXT, full_log.readlines)
An empty log is info itself, I'd add unconditionally (even on success, to debug unexpected successes).

The thing about flush_logged_errors seems to be the singleton nature of _log_observer? Perhaps it could be per-test?

We should spell
-        self.assertThat(list(error.keys()), Equals(['traceback']))
316	+        self.assertThat(
317	+            sorted(error.keys()), Equals(['traceback', 'twisted-log']))
as
  self.assertThat(error, HasKeys('traceback', 'twisted-log'))

None of these thing are deep or mandatory; it seems a sensible patch to me.
-- 
https://code.launchpad.net/~jml/testtools/learning-from-launchpad/+merge/39697
Your team testtools developers is subscribed to branch lp:testtools.