[Twisted-Python] Getting rid of "DirtyReactorWarning"
I have a ticket open for allowing tests that look like this, where each run has a new reactor instance: def test_somethingShouldHappen(self, reactor): result = [] reactor.listenTCP(...) self.runReactor(reactor) self.assertEqual(result, ["hooray"]) Further thought suggests however that this doesn't add much, necessarily, over the current "return a Deferred" idiom for most users. Experience with ReactorBuilder framework that has similar testing mechanism suggests it's far too easy to fail to stop the reactor. Moreover, things like coiterate() don't work since they rely on pre-imported global reactor, and given the "from twisted.internet import reactor" idiom this testing style won't work well for many people's code. But! ReactorBuilder tests are superior in one way: no DirtyReactorWarning. You don't have to wait until all connections are closed, which is difficult, or make sure to cancel every timeout. This makes test writing simpler. Perhaps we should get rid of DirtyReactorWarning for regular Deferred-returning tests as well, then. We have all the infrastructure we need, after all, for canceling scheduled events and disconnecting all descriptors. We might want to log when we do so, to help debug things, but we already have cleanup code mostly implemented in trial anyway, and it seems like DirtyReactorWarning doesn't usually make future tests fail anyway since we do run cleanup. Threaded code might still be a problem, but that's the case for current tests now, and even switching to a new style of tests as above would only somewhat mitigate the issue. Thoughts?
On 15 May, 10:26 am, itamar@itamarst.org wrote:
I have a ticket open for allowing tests that look like this, where each run has a new reactor instance:
def test_somethingShouldHappen(self, reactor): result = [] reactor.listenTCP(...) self.runReactor(reactor) self.assertEqual(result, ["hooray"])
Further thought suggests however that this doesn't add much, necessarily, over the current "return a Deferred" idiom for most users. Experience with ReactorBuilder framework that has similar testing mechanism suggests it's far too easy to fail to stop the reactor. Moreover, things like coiterate() don't work since they rely on pre-imported global reactor, and given the "from twisted.internet import reactor" idiom this testing style won't work well for many people's code.
But! ReactorBuilder tests are superior in one way: no DirtyReactorWarning. You don't have to wait until all connections are closed, which is difficult, or make sure to cancel every timeout. This makes test writing simpler.
Perhaps we should get rid of DirtyReactorWarning for regular Deferred-returning tests as well, then. We have all the infrastructure we need, after all, for canceling scheduled events and disconnecting all descriptors. We might want to log when we do so, to help debug things, but we already have cleanup code mostly implemented in trial anyway, and it seems like DirtyReactorWarning doesn't usually make future tests fail anyway since we do run cleanup.
Threaded code might still be a problem, but that's the case for current tests now, and even switching to a new style of tests as above would only somewhat mitigate the issue.
I think you should talk to jml about his ideas for a new base `TestCase`-like class which is better factored with respect to its handling of Twisted-related reponsibilities. I don't think just removing `DirtyReactorWarning` from all the places it is currently emitted is a good idea (if only because it is providing *us* value, and because new behavior merits new interfaces). More generally, I don't think ReactorBuilder is the best approach for unit testing anything except reactor implementations, just as I don't think using a real reactor (with real time, real networking, etc) is the best way approach for unit testing application code that uses the reactor. We need to keep improving our test doubles, documenting their usage, and porting our own test suite (the application-y parts, at least) to them. Jean-Paul
On Fri, May 18, 2012 at 9:21 PM, <exarkun@twistedmatrix.com> wrote:
On 15 May, 10:26 am, itamar@itamarst.org wrote:
I have a ticket open for allowing tests that look like this, where each run has a new reactor instance:
def test_somethingShouldHappen(self, reactor): result = [] reactor.listenTCP(...) self.runReactor(reactor) self.assertEqual(result, ["hooray"])
Further thought suggests however that this doesn't add much, necessarily, over the current "return a Deferred" idiom for most users. Experience with ReactorBuilder framework that has similar testing mechanism suggests it's far too easy to fail to stop the reactor. Moreover, things like coiterate() don't work since they rely on pre-imported global reactor, and given the "from twisted.internet import reactor" idiom this testing style won't work well for many people's code.
But! ReactorBuilder tests are superior in one way: no DirtyReactorWarning. You don't have to wait until all connections are closed, which is difficult, or make sure to cancel every timeout. This makes test writing simpler.
Perhaps we should get rid of DirtyReactorWarning for regular Deferred-returning tests as well, then. We have all the infrastructure we need, after all, for canceling scheduled events and disconnecting all descriptors. We might want to log when we do so, to help debug things, but we already have cleanup code mostly implemented in trial anyway, and it seems like DirtyReactorWarning doesn't usually make future tests fail anyway since we do run cleanup.
Threaded code might still be a problem, but that's the case for current tests now, and even switching to a new style of tests as above would only somewhat mitigate the issue.
I think you should talk to jml about his ideas for a new base `TestCase`-like class which is better factored with respect to its handling of Twisted-related reponsibilities.
In a nutshell, TestCase.run is delegated to separate strategy object. Relevant code is: TestCase: http://bazaar.launchpad.net/~testtools-committers/testtools/trunk/view/head:... (grep -i runtest) RunTest: http://bazaar.launchpad.net/~testtools-committers/testtools/trunk/view/head:... (that's the strategy object) Twisted RunTest: http://bazaar.launchpad.net/~testtools-committers/testtools/trunk/view/head:... reactor control code: http://bazaar.launchpad.net/~testtools-committers/testtools/trunk/view/head:... The last is significantly better factored and tested than the equivalent code in trial. This is faint praise. jml
participants (3)
-
exarkun@twistedmatrix.com -
Itamar Turner-Trauring -
Jonathan Lange