proto_helpers should be moved somewhere outside of a 'test' module

[twisted-trac]

	@glyph reported
Trac ID	trac#6435
Type	defect
Created	2013-04-15 19:51:33Z

proto_helpers is an unfortunate sore thumb sticking out in CompatibilityPolicy; it's the only thing in a "test" package you can import. Especially as it grows, this is going to become awkward. Plus, it's in twisted.test which is a module that, for all intents and purposes, is itself deprecated, because new tests should always go into a module for an appropriate package.

Having made this exception, we should, of course, keep supporting it by having aliases there and not removing them for a good long while. However, we should also find a place with a more obviously public name to put things.

Searchable metadata

trac-id__6435 6435
type__defect defect
reporter__glyph glyph
priority__normal normal
milestone__ 
branch__ 
branch_author__ 
status__closed closed
resolution__fixed fixed
component__core core
keywords__None None
time__1366055493000000 1366055493000000
changetime__1563642382682377 1563642382682377
version__None None
owner__hawkowl hawkowl

[twisted-trac]

	@exarkun commented

See also #5354, #5353, #3325

[twisted-trac]

	@tomprince commented

I think there may be some disagreement on whether it is an exception to the compatibility policy (see the comment on [ticket:6251#comment:4 #6251].

Also, a couple of questions:

1. Are the apis in there the ones we want to support, or should they be improved before moving somewhere public.
2. Do we want to move everything at once, or piecemeal. (Are there tests and documentation for everything in there? If not, it seems like doing that as we move things out seems like a good idea, and suggests doing this in multiple tickets)

[twisted-trac]

	@exarkun commented

Are the apis in there the ones we want to support, or should they be improved before moving somewhere public.

They sorta suck. A lot of them are very specific to a particular case. Many of them encourage poor testing practices. Across the entire module, it's a total hodge podge. You might get something handy and clever, or you might get something that's nonsense.

I'm going to be a bit of a jerk and suggest that some serious thought should go into the APIs we introduce elsewhere, rather than just moving the implementation of these things into a non-test package. See below for more, though.

Do we want to move everything at once, or piecemeal. (Are there tests and documentation for everything in there? If not, it seems like doing that as we move things out seems like a good idea, and suggests doing this in multiple tickets)

There's test coverage for almost none of it. There's documentation for slightly more of it than that. I'd definitely encourage doing this in multiple steps. In this case, not because there's going to be a huge number of lines in the resulting diff, but because the work to figure out whether each piece is good or needs improvement is largely unrelated to the work for each other piece (however, at the same time, many of these APIs need to be considered in conjunction with each other - for example, MemoryReactorClock is an example of what we get if no thought goes into the necessary interactions).

As far as the improvements to be made, I will offer one suggestion to start things off. StringTransport is by far the most widely used API from twisted.test.proto_helpers. It has been around longer than most things in proto_helpers and has had the most enhancements for general utility made to it. It generally gets the job done. However, it has a few problems:

1. It has a lot of public attributes and methods that aren't part of ITransport.
2. Real transports have careful coordination between ITransport.loseConnection and IConsumer.registerProducer (a registered producer holds a connection open). StringTransport doesn't know about this or do anything to help tests account for it.
3. The existence of StringTransportWithDisconnection is somewhat unfortunate for two reasons.
   1. The fact that StringTransport never calls any protocol methods, including connectionLost, makes it a somewhat meager implementation of ITransport. Putting this functionality into a separate class seems like it shouldn't be the best solution to this shortcoming.
   1. The actual way connectionLost is called is unlike any real ITransport implementation in that it is re-entrant. This is only sometimes a problem, but when it is it is a pretty terribly subtle one. It's not necessarily wrong for this call to be re-entrant, but if this difference from real transports is retained, it at least needs to be highlighted (rather than left entirely undocumented, along with the rest of StringTransportWithDisconnectiong).

At the risk of swinging back towards the jerk end of the spectrum, twisted.test.iosim should probably also be considered at the same time as StringTransport - to determine if there is any consolidation that makes sense, to determine if the implementations should be unified, or to determine if these are just two different useful testing styles that should both be retained and separately encouraged.

[twisted-trac]

	@glyph commented

Replying to exarkun:

Are the apis in there the ones we want to support, or should they be improved before moving somewhere public.

They sorta suck.

I'm starting to think we should have a rule where nobody can ever say "X sucks" (or even "X is great"). If we have a specific criticism (or praise), we ought to say what it is. For example:

A lot of them are very specific to a particular case.

It's OK to have specific things for specific cases. Do we need some general ones?

Many of them encourage poor testing practices.

What poor testing practices?

While I can see that many things in this module might encourage imperfect testing practices, it strikes me that everything in here encourages better testing practices than the other options, such as:

1. maintaining your own test fakes
2. returning real Deferreds everywhere and using real reactor/transport implementations
3. not testing things

Across the entire module, it's a total hodge podge. You might get something handy and clever, or you might get something that's nonsense.

I'm going to be a bit of a jerk and suggest that some serious thought should go into the APIs we introduce elsewhere, rather than just moving the implementation of these things into a non-test package. See below for more, though.

This is only jerky if you insist that it's necessary. If you're just suggesting that it might be good, it's helpful, that's true, and I agree :).

Do we want to move everything at once, or piecemeal. (Are there tests and documentation for everything in there? If not, it seems like doing that as we move things out seems like a good idea, and suggests doing this in multiple tickets)

There's test coverage for almost none of it. There's documentation for slightly more of it than that. I'd definitely encourage doing this in multiple steps. In this case, not because there's going to be a huge number of lines in the resulting diff, but because the work to figure out whether each piece is good or needs improvement is largely unrelated to the work for each other piece (however, at the same time, many of these APIs need to be considered in conjunction with each other - for example, MemoryReactorClock is an example of what we get if no thought goes into the necessary interactions).

I completely agree. I really should have put something about doing this piecemeal into the ticket description originally; this is basically how I assumed we'd do it, sorry for not saying so.

As far as the improvements to be made, I will offer one suggestion to start things off. StringTransport is by far the most widely used API from twisted.test.proto_helpers. It has been around longer than most things in proto_helpers and has had the most enhancements for general utility made to it. It generally gets the job done. However, it has a few problems:

This is a really great set of criticisms, thanks a lot - so good as to almost negate the unfortunate "sucks" comment up top ;-).

1. It has a lot of public attributes and methods that aren't part of ITransport.

Here's a suggestion of a pattern I think we ought to be following for this:

When you have a test fake with some attributes that are useful for "externally" verifying things (things that would normally be I/O to other systems) those attributes ought to go onto a separate object, and the factories for getting your test fakes ought to give you that other objects. So rather than:

MemoryReactor().connectTCP(...)
# ...
stringTransport = self.transport
test.assertEquals(expected, stringTransport.someInterestingTCPThing)

something more like:

reactorTester = ReactorTester()
mr = reactorTester.reactor
mr.connectTCP(...)
# ...
stringTransport = reactorTester.infoForTransport(self.transport)
test.assertEquals(expected, stringTransport.someInterestingTCPThing)

thoughts?

1. Real transports have careful coordination between ITransport.loseConnection and IConsumer.registerProducer (a registered producer holds a connection open). StringTransport doesn't know about this or do anything to help tests account for it.
2. The existence of StringTransportWithDisconnection is somewhat unfortunate for two reasons.
   1. The fact that StringTransport never calls any protocol methods, including connectionLost, makes it a somewhat meager implementation of ITransport. Putting this functionality into a separate class seems like it shouldn't be the best solution to this shortcoming.
   1. The actual way connectionLost is called is unlike any real ITransport implementation in that it is re-entrant. This is only sometimes a problem, but when it is it is a pretty terribly subtle one. It's not necessarily wrong for this call to be re-entrant, but if this difference from real transports is retained, it at least needs to be highlighted (rather than left entirely undocumented, along with the rest of StringTransportWithDisconnectiong).

At the risk of swinging back towards the jerk end of the spectrum, twisted.test.iosim should probably also be considered at the same time as StringTransport - to determine if there is any consolidation that makes sense, to determine if the implementations should be unified, or to determine if these are just two different useful testing styles that should both be retained and separately encouraged.

And let's not forget twisted.test.test_pb.IOPump...

[twisted-trac]

	eevelweezel commented

Link to github PR: #1137

[twisted-trac]

	@glyph commented

I hope to have some more time to give this a full review, but I do have one thought: can we call this module twisted.internet.testing, rather than a util module?

• it's for testing twisted.internet
• there's some precedent here in treq.testing which has worked out pretty well as a package

Thanks for this contribution!

[twisted-trac]

	eevelweezel commented

I can certainly rename the module to twisted.internet.testing. It will likely be tomorrow evening or Saturday before I can get to this, however.

[twisted-trac]

	@glyph commented

Replying to Heather White:

I can certainly rename the module to twisted.internet.testing. It will likely be tomorrow evening or Saturday before I can get to this, however.
No rush - after 6 years, we can wait 72h :)�.

[twisted-trac]

	eevelweezel commented

Alright, done. BTW, I'd welcome any suggestions for further work on this, ticket-wise.

[twisted-trac]

	@glyph set owner to eevelweezel

Reviewed over on Github. Thanks again!

[twisted-trac]

	eevelweezel removed owner

[twisted-trac]

	eevelweezel commented

Ok, believe I've addressed all the requested changes from GitHub.

[twisted-trac]

	@glyph commented

Thanks Heather! I hope we can get this re-reviewed / integrated soon.

[twisted-trac]

	@twm set owner to eevelweezel

Reviewed: #1137

[twisted-trac]

	eevelweezel removed owner

Apologies for the delay, I was camping. I believe I've addressed all the PR feedback.

[twisted-trac]

	@hawkowl set owner to @hawkowl

Looks good to me!

Thanks!

[twisted-trac]

	Heather White set status to closed

In changeset 4cade8b

#!CommitTicketReference repository="" revision="4cade8bb1e5ce45a86962fe2548470a413c8ce7a"
Merge eevelweezel:move-proto_helpers-6435: Move t.test.proto_helpers to t.internet.testing

Author: eevelweezel
Reviewers: glyph, twm, hawkowl
Fixes: ticket:6435