testtools-dev team mailing list archive

[Jonathan Lange <jml@xxxxxxxxx>]

On Sat, Oct 30, 2010 at 9:46 PM, Robert Collins
<robertc@xxxxxxxxxxxxxxxxx> wrote:
> Review: Approve
> I think in _clean's docstring its worth mentioning that at least *a* iteration happens. Or perhaps not.
>

Agreed. Added.

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

I had been avoiding use of Fixtures in order to minimize the
dependencies, which helps our users a little bit and also makes it
easier to upstream this code. Perhaps I'm being too fussy. I've made
no changes as yet.

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

Fair enough. Changed.

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

Well, it *could* be per-test, but there's no obvious way of me
building a per-test feature into a RunTest object and exposing it to
the test author.  The best way that occurs to me is monkey-patching a
method on to the self.case object. Not only is this a touch
distasteful, it also makes the method harder to discover.

> 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'))
>

I've done this, except that I've called it KeysEqual, so as to avoid
the implication of containment.

> None of these thing are deep or mandatory; it seems a sensible patch to me.

Thanks,
jml
-- 
https://code.launchpad.net/~jml/testtools/learning-from-launchpad/+merge/39697
Your team testtools developers is subscribed to branch lp:testtools.