Opened 5 years ago

Last modified 5 years ago

#6435 defect new

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

Reported by: Glyph Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

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.

Change History (4)

comment:1 Changed 5 years ago by Jean-Paul Calderone

See also #5354, #5353, #3325

comment:2 Changed 5 years ago by Tom Prince

I think there may be some disagreement on whether it is an exception to the compatibility policy (see the comment on #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)

comment:3 Changed 5 years ago by Jean-Paul Calderone

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.
    2. 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.

comment:4 in reply to:  3 Changed 5 years ago by Glyph

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.
    2. 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...

Note: See TracTickets for help on using tickets.