Trial tutorial

[therve]

Trial is great, it needs a documentation for Twisted users, probably in the form of a tutorial. It should cover all the common usecases:

• return Deferred from test
• setUp/tearDown
• testing a protocol without creating a client/server
• cleanups after tests (addCleanup)
• using task.Clock to test callLater/LoopingCall

I don't know though where should be the frontier between 'Trial tutorial' and 'unit-tests tutorial'.

[jml]

I think the tutorial should be "Test-driven development with Twisted", not "How to use Trial". We don't want explain all the cool stuff that Trial can do, we want to show all the cool things that you can do do with Trial.

Although it's a little redundant, I reckon it's worth explaining how to do unit tests and the basics of test-driven development. Lots of people come to Twisted with only a little knowledge of Python, and the goal of this document should be to get them writing tests as quickly as possible.

[jml]

Also, thanks for doing this. If ever you are in Australia, I will buy you a beer.

[radix]

I just want to point out that work on this has started in the trial-tutorial-2443 branch in doc/core/howto/trial.xhtml

[therve]

OK, this is far from being complete, but I'd need some feedback. Comments and/or direct changes from native english speakers would be great :).

[antoine]

Hi,

Generally speaking I think this tutorial is a very good idea. I've made a few random fixes (spelling, adding import lines, small improvements) that you'll find attached.

In its current form I think the document still has some rough edges. For someone not used to unit tests, I think it's not clear from reading the code snippets where they have to be inserted. Also, the "testing scheduling" and "code coverage" parts probably need to be made more understandable.

[terrycojones]

I spent most of yesterday afternoon/night working fleshing out the tutorial. It would be good to speak/IM with someone who understands all the examples. I'm about 2/3rds of the way through, but I don't know enough about what's going on in the example code to be able to write about it clearly. It's good that I'm doing this because I'm completely new to Trial (though not to unittest), so I can try to explain a lot of what the current draft leaves out. But I need help to understand it myself....

[therve]

Replying to terrycojones:

I spent most of yesterday afternoon/night working fleshing out the tutorial. It would be good to speak/IM with someone who understands all the examples. I'm about 2/3rds of the way through, but I don't know enough about what's going on in the example code to be able to write about it clearly. It's good that I'm doing this because I'm completely new to Trial (though not to unittest), so I can try to explain a lot of what the current draft leaves out. But I need help to understand it myself....

That's great, thanks a lot for going so far. Sorry for this week-end, I'll be mostly available on IRC from mondy to friday 9-21 UTC.

[terrycojones]

I'm attaching a zip of an svn diff showing where I'm up to. It's quite incomplete but there are many changes. It now builds successfully (to html) when you run admin/process-docs.

Terry

[terrycojones]

Snapshot of fleshing out of the tutorial. Highly incomplete.

[terrycojones]

Hi therve

doc/core/howto/listings/trial/calculus/test/test_{base,remote}.py should go away, as they're the original code examples.

You've taken the 'called' variable out of test_remote_2.py and as a result it's now identical to test_remote_3.py. So test_remote_3.py could go away too. I think this is a good simplification at that point in the tutorial. Maybe you intend to put something back into test_remote_2.py though to confirm that the callback has fired?

Here's a reminder that it would be nice to have the example code for talking to the RemoteCalculationProtocol class.

I'd like to add an example that addresses what I would like to do with trial. I have a web2 server and I want to test it. The current tutorial gives me some ideas about how to do that (e.g., use a proto_helpers.StringTransport for the transport), but I still don't know how to write the tests. This may seem like a Twisted thing (I mean it may seem like I want it because I don't know enough about Twisted), but I don't see any harm in another example. For me the point of revising the tutorial (apart from helping others etc) is to learn how to test my http server, and I don't yet know exactly how to do that.

I'm around for the next 2 hours, then away for 4, and will probably be up most of the night. I'd be happy to push all the way through revising the tutorial ASAP.

[therve]

That's not in review for now.

[ralphm]

I've seen recent activity in the associated branch for this ticket. @therve: are you actively working on this?

[therve]

Replying to ralphm:

I've seen recent activity in the associated branch for this ticket. @therve: are you actively working on this?

I had a bunch of motivation last week or so. I removed some big errors, and made some cleanups. I think this is in a correct shape, but any comment is welcome :). I don't plan on working on it soon if I don't get some feedback.

[therve]

thijs, do you think you could have a look at the current branch? I'd like to revive it.

[termim]

The example with timeout (client_3.py) is not quite correct IMHO: RemoteCalculationClient.timeOut collides with TimeoutMixin.timeOut. As a result timeout is set right only for the first message after connect. For the rest of messages timeout is None. The following test case (added to test_client_3.py ) verifies that (fails) for me:

    def test_timeout_2(self):
        def cb(x):
            #self.proto.timeOut=60
            return self.proto.add(9, 4)
        d = self._test('divide', 14, 3, 4)
        d.addCallback(cb)
        self.clock.advance(60)
        return self.assertFailure(d, ClientTimeoutError)

It passes if I uncomment the commented line.

IMHO the proper solution would be to make TimeoutMixin.timeOut private i.e. change it to TimeoutMixin._timeOut.

[thijs]

Replying to therve:

thijs, do you think you could have a look at the current branch? I'd like to revive it.

I made some adjustments in the branch in r24468. Let me know if there's anything else I can do.

[CaptSolo]

This is a very nice tutorial and would be good to get it reviewed and published (btw, glyph recently did some fixes to this doc in [26254]).

I hope that my feedback (from beginner's point of view) can help.

Ch. "Making the tests pass": link to test_base_2 follows right after link to base_2.py and readers can misunderstand which file does "with the import changed" refer to (at least i did). Suggest to add some text before the link to test_base_2 (to visually separate two links) and clarify text about importing. E.g.:

The file test_base_2.py is a copy of test_base_1, changed to import base_2.

Ch. "Twisted specific testing": links to basic twisted tutorials or documentation would be helpful, for those who came to the tutorial w/o knowledge of twisted.

Ch. "Creating and testing the server": the part about "it's just used to" in the following sentence is unclear to me - "Note that the address and port passed can have any value, it's just used to have a valid address".

So far all examples were clear.

As for remainder of examples (starting from test_client_1.py), they caused major headaches and constant going back and forth between different code listings to get an idea of what is happening. It would help a lot if some comments were added to the code. For example, this bit of code from client_2.py took a while to understand:

d, callID = self.results.pop(0)
callID.cancel()
d.callback(int(line))

Finally, "please give use your feedback!" can be improved by adding an email address or a link to where people can send feedback. a typo fix: "use" -> "us".

[glyph]

CaptSolo, thanks very much for your comments.

I discussed this briefly with JP on IRC; since this has been in process-limbo for way too long, I'm going to treat your feedback here as a review, address the comments you've made, and merge it after I've done so.

[glyph]

(In [26487]) Branching to 'trial-tutorial-2443-2'

[glyph]

(In [26490]) re #2443 address "Making the tests pass" comment.

[glyph]

(In [26491]) re #2443 address "Twisted specific testing" comment: mention potential dependencies.

[glyph]

(In [26492]) re #2443 address "Creating and testing the server" comment: re-word explanation of the fabricated address to explain what it's for.

[glyph]

(In [26493]) re #2443 address "Finally" comment: correct typo, and add a link to the website where users should file tickets for doc improvements.

[glyph]

OK, the last comment I find difficult to address directly. I guess this still isn't quite ready to merge. I'm going to put it into review to solicit some feedback, both on what's there and what I plan to do about it.

The code which CaptSolo suggests is confusing also recommends the use of twisted.test.proto_helpers in third-party code.

As a result, I've added some notes about test packages and private attributes to CompatibilityPolicy. I don't want to encourage anyone to import any code out of a test package, since arguably the whole point of putting it in a test package is to avoid anyone importing it yet. There's not much point in having a policy and making decisions about it if our official documentation just says "nudge nudge, wink wink, nobody cares about the policy anyway".

My recommended course of action here is to replace the "twisted specific testing" portions of the tutorial with a much shorter, simpler section that simply explains returning a Deferred and does a few brief examples using "real" I/O, explaining dirty reactor detection. I think this would test some very naive code which calls getPage and deferLater; no client-server hookup, just "You got this deferred, now what?".

An important thing I would emphasize in this section is that you shouldn't (can't) run the reactor in your tests, as you would in example code, that trial does it for you if it needs to.

I realize that this is not the current accepted best practice, but for people just learning how to do TDD, I feel it would be very useful. In many cases when introducing Twisted users to unit testing, they are facing an API which returns a Deferred, and they hit a wall. In some cases this Deferred is coming from a client implementation for which there is no Twisted, or even Python, server implementation. There's a feeling of immense power, almost glee, for these users, when they realize that they can just barrel on through and issue a request in the middle of their unit tests without learning anything else. (As a data point, all of these users that I'm still in touch with have since moved on to using transport mocks.) I think this is an important intermediate step for someone learning TDD, especially learning in the context of a poorly-tested existing system (which, unfortunately, it seems like what everyone does). It's important to keep things moving along as you write your tests and not constantly get stuck on the need to develop more test infrastructure, which is exactly what trial allowing you to return a Deferred does.

However, since we all know that this approach is fraught with peril, I think this section should clearly explain dirty reactor errors and the risks associated with intermittent network- and timing-related failures, and explain how some good test objects can help you improve any tests you write to return real-IO deferreds.

There should then be two more tickets: one for "make proto_helpers public", one for "expand TDD with twisted tutorial to include documentation of how to use proto_helpers and task.Clock".

The tutorial can end with a link to that ticket, which can be replaced with a link to the new tutorial when the other ticket is finished.

So, to summarize:

1. Remove proto_helpers and task.Clock portions of this tutorial.
2. Add a new, shorter segment that just explains testing code which already calls existing high-level Deferred-returning APIs in test methods (and cleaning up after them), rather than on testing APIs which themselves create and manage Deferreds.
3. File a ticket to make proto_helpers public.
4. File a separate ticket to document using verified fakes for more reliable testing. Put the removed portions of this tutorial into that "part 2" document. ("Advanced Test-Driven Development with Twisted"). This ticket will depend on the proto_helpers ticket.

Please let me know what you think.

[glyph]

Remove tearDownClass from the description (ZOMG we should not document that), add addCleanup.

[glyph]

Okay. I just found  http://twistedmatrix.com/projects/core/documentation/howto/testing.html which is about 1/3 of what this needs to do, but doesn't make much sense. (It starts by describing the temporary directories that trial creates? In the index it's referred to as "tips" for writing tests, rather than a guide? It doesn't actually include any examples?)

This branch should also replace that document in the index.

[glyph]

Assuming washort's not going to do it, since it's been a few weeks.

[exarkun]

Some minor suggestions:

1. (and in fact subclasses) doesn't add much to the introduction, and since it may end up being inaccurate soon, I think it should just be removed.
2. If you're shell reports something like uses you're where it should use your
3. First trial tells you how many tests it detected.: trial doesn't actually do this anymore :( And the sample output in the document doesn't include this information.
4. In the list following The tests can be run by trial in multiple ways:, please strongly encourage the use of FQPN over filesystem paths. The former is much more reliable than the latter.
5. In base_3.py, please make the raise TypeError use () and include some information about what went wrong (just so that it is a good example of how to generate errors instead of a bad one, even though that's not the central point here)
6. After the paragraph ending with By setting up and using a fake transport, we can write 100% reliable tests., I would add some more text about how it is also important to test network failures, but mocking out the transport also allows us to do that deterministically and keep it separate from the success code-path tests.
7. Of course you can't let your users with this doesn't make sense - a word is missing or wrong or something?
8. We just factor operations, everything else should be obvious. is a pretty awkward sentence. Perhaps just omit it. Although the pipelining support is a little bit non-obvious, and not tested. ;) Ignore that if you want, though.
9. This one of the tough parts of Twisted - let's not go out of our way to suggest that Twisted is hard, at least not in the cases where it's not! Clock makes this really easy, as the document goes on to explain. I think the hard thing this sentence might have been referring to is that lots of existing APIs don't parameterize the clock. We can probably skip that here, since this is a document about how to do it right, not about all the things that do it wrong. :) Perhaps it would still be worth mentioning that if you find an API that doesn't parameterize it and you want to test it, you should first parameterize the clock.
10. will finish in the clock stack. - I would instead say something like 'will finish before clock.advance returns..
11. Passing an IReactorTime to __init__ is a bit better form than setting attributes, right? If you agree, please adjust the example to do it that way.
12. This remove the need of a tearDown - remove should be removes
13. in remote_2.py, the log.err call should pass a value for the message parameter.
14. The inline code in the Handling logged errors section has bad indentation.
15. test_client_2.py doesn't benefit from the use of StringTransportWithDisconnection (as opposed to just StringTransport) as far as I can tell. Am I right? Switch to StringTransport if so.
16. If test_client_3.py used StringTransport and checked StringTransport.disconnecting, it wouldn't need to use StringTransportWithDisconnection or clobber connectionLost like it does, which I think would be better. As a bonus, combined with the previous point, StringTransportWithDisconnection would no longer be used at all.

Aside from all that pretty simple stuff, I think the only reason not to merge this as-is is the point glyph raised about proto_helpers previously. I don't think overlap with testing.html, removing Clock, etc needs to be considered here. This is a big lump of new, good documentation. It's not the only possible lump of documentation, but that doesn't matter. File more tickets to write more, or something.

So, about proto_helpers. If StringTransport is the only thing that's used by these docs, then I think the thing to do is move StringTransport out of the test package into a real package. Of course, this will involve writing unit tests for it. Unfortunately, I think this should happen first so that we don't have docs pointing people at twisted.test APIs.

Aside from this and the above (fairly trivial, though not exactly short) list of points, I don't think anything else should block this from being merged.

[therve]

(In [26988]) Handle points 1-5, 7

Refs #2443

[vultaire]

I'd like to say that the tutorial is looking decent, but one thing which was left out was how to name modules to take advantage of running trial like this:

trial <package_name>.test

I couldn't find anywhere that specified that the modules within the test directory also had to have the "test" prefix on them to be found; this seems to just be assumed. I thought the "test" prefix only applied to the test methods in a TestCase class, until someone on the ML pointed out otherwise.

That's my two cents. Thanks.

[binjured]

For people looking to start using Twisted or start working on Twisted (important!), this is one of the most crucial pieces of documentation we don't have. I want to resolve the outstanding issues, but it would appear that relies on an agreement regarding the state of twisted.test—more specifically, the proto_helpers module and what needs to be done to either (a) accept that it's already basically public or (b) take the steps necessary to make it officially so.

I am more than happy to do what Glyph's reply suggests and talk about writings tests using the reactor and returned deferred's (I'd probably just use http://twistedmatrix.com/trac/browser/trunk/twisted/web/test/test_webclient.py#L264 as an example and explain the key points). Additionally, his experience seems to corroborate what I feel about proto_helpers: it's an oft-necessary step in one's maturity as a Twisted Tester. As such, I don't feel like this document will be complete until it can safely talk about transport mocks—wherever they may be imported from!

So, like, let's get on this; four years is long enough. *cracks whip*

[exarkun]

(In [30785]) Branching to 'trial-tutorial-2443-3'

[virtual]

I'm a complete Twisted beginner. I'm writing a project containing both clients and servers, which is getting to the point where I want some unit tests. I checked out this branch, hoping to get some hints on how to use Twisted.

Glad to see this branch is getting some attention from the core devs; I see parent is dated less than an hour ago :)

The tutorial would flow more nicely if the example code was located in the tutorial itself, rather than a separate file you have to download and open in a text editor. However, the separate file is nice if I want to actually run the code. Maybe we should have both?

---

The tutorial should make clear that test source files need to have names that start with "test_". I was using names that didn't follow this convention and trial <packagename> couldn't find the tests (it just immediately passed because it ran zero tests.)

---

In test_remote_1.py, the test_* methods have a body of the form "return self._test(...)". This is confusing for two reasons:

(1) There is no explicit return from self._test. Of course, this means self._test returns None when the end of the body is reached. But using the return value of a function with no return statement gives programmers coming from strongly typed languages a peculiar gut-wrenching negative reaction.

(2) In general, unit tests should not return values; *even if* substantial changes were made to the tutorial code and self._test returned some meaningful value, using a return statement to pass this value on to the testing framework from test_* is pointless and confusing. AFAIK all the testing frameworks I've worked with (JUnit, PyUnit, unittest2, Trial) just throw away the return values from test_ methods.

You could even add something to this effect to the tutorial: "For most well-written tests (including all of the tests in this tutorial), assertions and exceptions are the sole communications between the testing framework and the test_ methods. Trial will ignore any value returned from a test_ method."

[virtual]

You could even add something to this effect to the tutorial: "For most well-written tests (including all of the tests in this tutorial), assertions and exceptions are the sole communications between the testing framework and the test_ methods. Trial will ignore any value returned from a test_ method."

I just realized this isn't true, if you return a Deferred :)

I would edit this incorrect observation out of my above comment, but apparently comment editing is not a feature supported by Trac...

[glyph]

binjured asked for a pronouncement on this, so:

proto_helpers is public (unfortunately). Other projects are using it. We support them. It's the only way to do what it does.

There should be a ticket to move it to a new namespace where it can be more widely publicized, but it's public for now. Documentation should stress that it the only module in this namespace which is public.

[itamar]

Based on skimming the ticket, my understanding of its current state:

1. In order for this ticket to be put in final review, I think we just need to address comment #31 (items 11-16) and comment #33.

2. Once merged, we should open ticket to move proto_helpers out of twisted.trial, and open ticket to add more comments and docstrings to the sample code in the howto (the latter is a good idea but not a blocker for this ticket).

[itamar]

Oh, and of course pre-review all the code samples and tests have to be run and ensured to work :)

[binjured]

Issues fixed by the attached patch::

#31: 12-16 (11 is ambiguous to me; where are we setting attributes that we should pass IReactorTime?); #33; #39.

Tests are confirmed to work *as expected* (e.g. the first module is meant to fail), except for the failing test test_remote_3.test_invalidParameters where flushLoggedErrors() isn't doing its job for reasons I am unfamiliar with.

Patch created against branches/trial-tutorial-2443-3. Cheers!

[itamar]

For comment #31, issue 11, IIRC what is happening is that callLater is being overridden; instead the class should have a scheduler or reactor attribute that gets replaced with a Clock (e.g. by having overridden default argument in init).

[binjured]

I talked with itamar about the Clock issue and we seem to agree that it's unintuitive to override callLater() as it's being done and ultimately we should probably just remove TimeoutMixin from the protocol so we can stop doing that. We're not sure that doing so necessitates holding up this particular issue, though. Thoughts?

[exarkun]

(In [31493]) Some minor corrections and additions

1. we have more tests now (in other words, this branch was two thousand tests old)
2. tearDown is not run if setUp raises an exception
3. but cleanup functions (functions passed to addCleanup) are
4. Deferreds returned from cleanup functions are handled correctly

refs #2443

[antoine]

Sorry for the bother, but how do I remove myself from cc? I thought I did but I still receive notifications. Thank you ;)

[exarkun]

(In [31494]) lore2sphinx fixes

refs #2443

[exarkun]

I spoke to antoine out of band, and apologized for the fact that our issue tracker will not sending him email. We should take this as additional motivation to resolve this ticket quickly so he does not get too many more unwanted emails. :)

To that end:

1. I made a couple tweaks, not the ones linked above, those are bogus and I reverted them. Instead, r31496, r31497, and r31498. Some for content, some for Sphinx. Please look them over, but I think they're fine.
2. I think the TimeoutMixin stuff is fine. When TimeoutMixin supports a better way to do this, we can document that instead.

This document looks good to me in its current state. Please add a .doc fragment and merge the branch! Also don't forget to file the tickets requested in earlier comments. Thank you!

[exarkun]

(In [31506]) Merge trial-tutorial-2443-3

Author: exarkun, therve, glyph, binjured, antoine, terrycojones, termim Reviewer: CaptSolo, exarkun, itamar Fixes: #2443

Add a test-driven development howto, based on trial.