Closed jml closed 8 years ago
If this can be landed now, lets do that and avoid juggling conflicts
Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions.
_testtools/_deferreddebug.py, line 7 [r1] (raw file):_ No all ?
_testtools/_deferreddebug.py, line 11 [r1] (raw file):_ PEP 297 (I think?) Specifically shows """ Foo bar baz""" for one liners. I value vertical space a lot - I realise its not a functional thing, but I'd like to keep this consistent, and I'm pretty sure we've done genuine one liners for everything in the code base...
_testtools/_deferreddebug.py, line 17 [r1] (raw file):_ Since this is using _setUp, the dependency on fixtures should be versioned with minimum appropiate to the introduction of that API.
_testtools/_deferreddebug.py, line 23 [r1] (raw file):_ I think this is fine as-is. It occurred to me reading it that you could instead say
def __init__(....)
super
self._deferred_debug = MoneyPatch('twisted.internet.defer.Deferred.debug', debug)
self._delayedcall_debug = MonkeyPatch('twisted.internet.base.DelayedCall.debug', debug)
def _setUp(self):
self.useFixture(self._deferred_debug)
self.useFixture(self._delayedcall_debug)
Which seems a little clearer to me, but YMMW
Comments from the review on Reviewable.io
Thanks for the review. I've updated the docstring, and will merge.
Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions.
_testtools/_deferreddebug.py, line 7 [r1] (raw file):_ No. I don't think it's quite so important for internal code. Can add one later if you feel strongly about it, or we have a coherent standard written somewhere.
_testtools/_deferreddebug.py, line 11 [r1] (raw file):_ Yeah, we are. flocker follows this (Twisted-style) approach to docstrings and it creeped over.
_testtools/_deferreddebug.py, line 17 [r1] (raw file):_ Done.
Comments from the review on Reviewable.io
Review status: 0 of 6 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.
_testtools/_deferreddebug.py, line 23 [r1] (raw file):_ The way I wanted to write it was:
def debug_twisted(debug):
return CompoundFixture([
MonkeyPatch('twisted....', debug),
MonkeyPatch('twisted....', debug),
])
It was this second use-case which provoked me to testing-cabal/fixtures#22
Comments from the review on Reviewable.io
Merged at 91c195c
Some of our tests assume that debugging is not enabled in Twisted. This makes that assumption explicit by disabling debugging for the duration of the test.
It does this by:
Fixture
) when debugging is not setDebugTwisted(False)
Testing strategy has been to run tests with
trial --debug-stacktraces
. I don't think we need explicit tests for the debugging behavior. There are other issues preventing trial from running in full (due to Trial's weird reporter API), but that's a separate PR for another time.Will conflict with #171, which also adds fixtures as a dependency, and (in spirit) with #202, which moves all Twisted code to its own package.