[Twisted-Python] Removing _wait from Trial

[glyph at divmod.com]

On Sun, 24 Sep 2006 17:26:10 +1000, Jonathan Lange <jml at mumak.net> wrote:
>So, I would like to remove _wait() from Trial and make it work like a
>regular Twisted application. I've had all this stuff on my brain, and
>have decided to dump it here for feedback. Here's how the plan looks
>at the moment[1]:

>1. Deprecate the use of reactor-spinning things within tests. (Twisted 2.5)

YES.

>Unfortunately, some of Trial's cleanup code spins the reactor (see
>t.trial.util._Janitor.do_cleanPending). This has to go.

You are saying this part of the plan will happen post-2.5, I assume?

>2. Tighten test cleanup errors. (Twisted 2.5)
>
>#2091 (ready for review) fulfils a long-standing promise. The "reactor
>unclean" warnings have been turned into errors.

I would really like it to be possible to turn these errors into warnings by passing a command-line flag, mainly for the migration path between 2.4 and 2.5.  If you upgrade an application to Twisted 2.5 and then run your tests, and you get 100 failures, it would be nice to be able to run "trial --unclean=warn_only", and focus on fixing any issues with the upgrade of APIs rather than just the test tool.  Even nicer to see that your tests still pass and you just need to update the tests to clean up after themselves.

Twisted's buildslaves should _NOT_ use this flag, nor should it still be present in 2.6.  I am suggesting it _ONLY_ as a migration tool.

>To remove _wait, we'll also have to remove the two calls to
>reactor.iterate() from _Janitor (see #2092). That means that many
>tests that *thought* they were cleaning up the reactor properly will
>find out that they weren't. We can reduce the number of errors by
>simulating reactor.iterate() using callLater. However, experiments
>show that even with this bandaid, there will still be new errors in
>the Twisted suite.

It sounds like a good process for getting changes like this through the Twisted suite might be:

 * prepare the branch
 * get it reviewed, but don't fix the tests yet
 * have a "fix trial party" where we encourage lots of other hackers to get together on IRC and contribute to the same branch
 * get it reviewed by someone who couldn't participant (bonus: incentive to participate is that you don't have to review a massive branch...)

>That means we will have to either:
>a) surprise our users with new errors in their tests
>b) find some way to introduce this gradually.

I think this might be another use-case for a flag.  Really, developers should be alerted to these changes as soon as possible (warnings are too easily ignored), but have a way to give themselves a little leeway to fix the problems before the next release.

>Obviously everyone[2] would prefer b). I've filed #2124 explaining how
>that might work.

>3. Introduce asynchronous APIs to Trial (Twisted 2.6)
>
>Currently TestCase.run() is a blocking call. I wish it could remain a
>blocking call, but it can't.

Why do you say you wish it could?  Isn't the whole point here to make it not be a blocking call?

>The branch for #1781 makes a SyncTestCase and an AsyncTestCase. run()
>works in SyncTestCase and doesn't work in AsyncTestCase. For
>AsyncTestCase, one uses runDeferred().

This all sounds fine.

>Although the branch for #1781 will need to be abandoned, these new
>classes should be brought in for Twisted 2.6 (without breaking run()).
>Naturally, this affects many, many tests in Trial.

Another potential place where a 'fix trial party' might be useful.

>4. Deprecate TestSuite.run and TestCase.run. (Twisted 2.6)
>
>Neither of these will work without wait[3] and they are both public
>APIs. Also remove the calls to iterate() from _Janitor at this point.

In what sense are these APIs "public"?  Are there really people calling them outside of the 'trial' tool?

>5. Actually remove _wait. (Twisted 2.7)

>Add runDeferred() to TestSuite, make run() raise NotImplementedErrors
>for TestCase and TestSuite. Alter TrialSuite to start and stop the
>reactor.

Is there something significant about raising NotImplementedError?  Why aren't you just going to remove the method?

>I can't find a theory to explain why this change will break unexpected
>tests in the Twisted suite. However, it will certainly happen. That
>means that some tests in user code will probably break as well. I
>don't know how we could make this any smoother.

Until we have a theory about how and why things are going to break, we shouldn't worry about making it smoother.  That's not to say that things aren't going to break, but the plan for smoothing things out is likely to depend upon the details of the breakage.

>Questions:
>- What should I do with the ticket #1781?

It looks like it should have a milestone of Twisted-2.7, and stay as it is.

>- What should I do with the branch for #1781?

I'd have to review a bunch of other code to be sure, and I haven't, but I'd say "keep merging it forward".  Maybe just dropping it and rewriting it later makes more sense, though.

>- What more can we do to reduce the number of tests that will break
>when _wait is removed?

See above.

>[1] Incidentally, all of this would be superfluous if reactors were 
>restartable.

A clean restart() API would be a possible way of fixing some things, but I think you are underestimating how much code would break with a clean restart() method, too.  For example: the threadpool would be stopped after each test.  Would that make any existing Twisted tests fail?  I don't know, but operating under your (probably correct) pessimistic assumption that any change to the test framework's interaction with the reactor will break some things, I'd have to say yes.

Then there is the issue that Bob recently brought up, which is that in a 'normal' Twisted application the reactor is only started once, and so it's not really a representative test to constantly be pausing and resuming it.  If we implement a restartable reactor it would probably be a good torture-test though.

Thanks for all the work on this issue.  Every time I look at it, I'm shocked by just how difficult this transition has been, and all the more thankful for your capable stewardship of it.