Ticket #5392 enhancement closed fixed

Opened 2 years ago

Last modified 2 years ago

Testing infrastructure for ReactorMixin tests: hooking up two protocols to each other

Reported by: itamar Owned by: itamar
Priority: normal Milestone:
Component: core Keywords:
Cc: exarkun Branch: branches/connect-protocols-5392-4
(diff, github, buildbot, log)
Author: itamar, washort Launchpad Bug:

Description

I've implemented the following code a few times: run one protocol on server, another protocol connecting to it as client, run the reactor until they both disconnect and then gathering the resulting disconnect reasons. There should be utility methods for easily doing this, and at least a couple of the existing implementations should be ported to use this as a demonstration that the API is generic enough.

Change History

1

Changed 2 years ago by itamarst

  • branch set to branches/connect-protocols-5392
  • branch_author set to itamarst

(In [33177]) Branching to 'connect-protocols-5392'

2

Changed 2 years ago by itamar

  • cc exarkun added

Ugh. ConnectionTestMixins does this in a half-assed, badly factored unhelpful sort of way. My code is better.

Do we need backwards compatibility for ConnectionTestMixins or can I refactor it?

3

Changed 2 years ago by itamar

My end goal, BTW, is to refactor both the abort tests and pretty much everything in test_tls to use the new utility method. But if ConnectionTestMixins should stay as is, I might stop after the abort tests.

4

Changed 2 years ago by exarkun

Test packages aren't public.

5

Changed 2 years ago by itamar

  • keywords review added

Ready for review. I've refactored some, but not all, tests in twisted.internet.test.test_tcp, test_unix and test_tls to use this new infrastructure, and it will also be useful for future work on reactor tests (e.g. a thorough half-close test suite as part of #5341, or an update to #5347 if this gets merged first).

I am mystified by the PortableGtkReactor failures ( http://buildbot.twistedmatrix.com/builders/winxp32-py2.7/builds/622). Any ideas?

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392

6

Changed 2 years ago by itamar

Ah. Because PortableGtkReactor is a broken piece of crap with minimum latency of 100ms between any network event loop iteration, and so (a) it's slow (b) it will fail tests with short timeouts. The easy fix is to make it have less crappy latency (which will also speed up the test suite). But perhaps that should be in a different ticket?

7

Changed 2 years ago by glyph

  1. I found it a bit odd that you called the arguments serverProtocolFactory and clientProtocolFactory, despite the fact that they're not, well, protocol factories. I'd suggest renaming them to something like
  2. the PortableGtkReactor failures do need to be addressed, since that's a supported platform. File another ticket, and maybe I'll have a look at that so I can get credit for this review? ;-)

Otherwise it looks pretty good and " 6 files changed, 270 insertions(+), 295 deletions(-)" speaks for itself. If PortableGtkReactor can be made to pass its tests, then this can land.

8

Changed 2 years ago by glyph

  • keywords review removed
  • owner set to itamar

Actually I guess I should move it out of review, since other people should look at other tickets first.

9

Changed 2 years ago by itamarst

  • branch changed from branches/connect-protocols-5392 to branches/connect-protocols-5392-2

(In [33445]) Branching to 'connect-protocols-5392-2'

10

Changed 2 years ago by itamar

  • owner itamar deleted
  • keywords review added

I renamed the parameters, and tests now pass on portable gtk reactor:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-2

Putting up for review again (which can be quick) because I additionally also refactored test_newtls.py to use the new code.

11

Changed 2 years ago by exarkun

  • owner set to exarkun
  1. It doesn't look like the parameters were renamed.
  2. The proliferation of new timeouts with new values is bad. Any test that involves real I/O is going to take an unpredictable amount of time. If we are ever going to deal with it, we really need to avoid having twenty different timeouts spread out all over the place. Beyond that, tests with 1 second timeouts are going to fail on the buildbots, a lot (The abortConnection tests with their custom 10 second timeout are already failing a lot).
  3. Why is this a ReactorBuilder method? Making it so provides an extremely short-term convenience (lots of existing test methods can start using it with minimum typing) but makes it inaccessible to any other code and collides with any potential future kind of connection testing any of these tests might want to do (not catastrophically, but I can imagine a future with 5 different connectiong testing helpers, confusing people about which one they want to use). The implementation doesn't really need to be part of ReactorBuilder - it could even accept a reactor as an argument and not need to be mixed in to a ReactorBuilder/EndpointCreator either. The extra assertions about removeAll() and getDelayedCalls() seem orthogonal, too.

12

Changed 2 years ago by exarkun

ugh, accidental early submit.

13

Changed 2 years ago by thijs

  • keywords review removed
  • owner changed from exarkun to itamar

14

Changed 2 years ago by itamarst

(In [33590]) Apparently I forgot to commit the renamed parameters change. Refs #5392

15

Changed 2 years ago by itamar

  • owner itamar deleted
  • keywords review added

I moved connectProtocols out of the class and made it a function, and removed the timeout parameter.

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-2

(I think the API documentation buildslave failure is unrelated to this branch, since it talks about Agent.)

16

Changed 2 years ago by itamar

Getting this in would be helpful for testing #3204.

17

Changed 2 years ago by washort

  • branch changed from branches/connect-protocols-5392-2 to branches/connect-protocols-5392-3
  • branch_author changed from itamarst to itamarst, washort

(In [33717]) Branching to connect-protocols-5392-3

18

Changed 2 years ago by washort

(In [33723]) Apparently I forgot to commit the renamed parameters change. Refs #5392

19

Changed 2 years ago by washort

Okay my use of bzr rebase was a little clumsy but I resolved a conflict with trunk and put it into connect-protocols-5392-3.

20

Changed 2 years ago by exarkun

  • owner set to exarkun
  • status changed from new to assigned

21

Changed 2 years ago by exarkun

  • keywords review removed
  • owner changed from exarkun to itamar
  • status changed from assigned to new

Thanks! I'm really happy to see this refatoring. Overall I think it's a great change. I see a few not-so-major things with the code, though:

  1. in twisted/internet/test/connectionmixins.py
    1. There's some random whitespace change in the serverFactoryFor docstring
  2. in twisted/internet/test/reactormixins.py
    1. Potential error-handling shortcomings in the implementation of connectProtocols:
      1. serverEndpoint.listen Deferred could fail, the result is a hang I think
      2. Same goes for clientEndpoint.connect
    2. A 5-tuple is not a radical data structure. Perhaps an epsilon.structlike.record? Oh wait that's not part of Twisted yet. Well, something with attributes instead of indexes would be nice.
  3. in twisted/internet/test/test_tls.py
    1. WrapperFactory.buildProtocol's self_ argument could probably be better named (I imagine someone coming along and deleting an apparently accidental stray _ and then being confused). "wrapperSelf" or something like that would probably do nicely.
    2. Also, WrapperFactory is gross :( I liked StartTLSClientEndpoint better before. :/ Maybe we can figure out a way to clean it up more later, though.
    3. StartTLSCreator only uses startTLS on the client side, so the name is a little misleading.
  4. Since this is private infrastructure, do we really want to include it in the news file?
  5. Also, what do you think of adjusting the name of this new helper function? connectProtocols doesn't suggest anything about running a reactor or waiting for a full connection/session to complete. unfortunately all of the other names that come to mind have "and" in them at least once, which isn't any better.

I also see some possible future improvements:

  1. Using twisted.protocols.policies to remove the need for subclassing to use connectProtocols.
  2. Allowing protocol instances to be passed in, instead of callable factories (simplifies more user code than it complexifies, I think)

22

Changed 2 years ago by itamar

  • keywords review added
  • owner itamar deleted
  • branch_author changed from itamarst, washort to itamar, washort

I addressed all the review comments except WrapperFactory being gross. I also changed the API so protocol instances are passed in, which simplified things quite a bit. Started buildbot here:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-3

23

Changed 2 years ago by itamarst

  • branch changed from branches/connect-protocols-5392-3 to branches/connect-protocols-5392-4
  • branch_author changed from itamar, washort to itamarst, itamar, washort

(In [34147]) Branching to 'connect-protocols-5392-4'

24

25

Changed 2 years ago by exarkun

  • owner set to exarkun
  • status changed from new to assigned

26

Changed 2 years ago by exarkun

  • owner changed from exarkun to itamar
  • status changed from assigned to new
  • keywords review removed

Thanks!

  1. twisted/internet/test/reactormixins.py
    1. The docs for ConnectableProtocol should probably point out that two of its attributes get set by some outside code. Perhaps it's worth changing this, even, and adding a method to ConnectableProtocol for setting these.
    2. The docstring also says L{ReactorBuilder.runProtocolsWithReactor}, no longer accurate.
    3. It looks like it would be easy to pull Factory out of runProtocolsWithReactor.
    4. runProtocolsWithReactor will fail if the endpoint listen call returns a synchronously failing Deferred (eg because a port couldn't be bound). finished will run and call reactor.stop() before reactor.run() has happened. twisted.internet.test.connectionmixins.needsRunningReactor was recently added to help handle this case in some other tests.
    5. The isinstance in finished can be avoided if log.err were to be added as a separate errback prior to the reactor.stop()-calling callback.
    6. Also, we should really, really stop adding log.err calls without a 2nd argument.
  2. twisted/internet/test/test_tcp.py
    1. twisted.internet.test.test_tcp.AbortConnectionTestCase_Win32Reactor.test_resumeProducingThrows seems to fail a lot on the Windows Python 2.6 builder.
    2. The TCPCreator class docstring has C{ReactorBuilder.runProtocolsWithReactor}. This is the wrong name now, and it should probably be L{} instead.
    3. TCP4ClientTestsBuilder needs a class docstring
    4. TCPClientTestsBuilder and TCP6ClientTestsBuilder need better class docstrings.
    5. I think TCPClientTestsBuilder is (still) misnamed. It's not a builder in the same way as the other things in this file. It is intentionally not used to generate test cases with makeTestCaseClasses.
    6. Some (technically pre-existing) issues with runAbortTest:
      1. The comment above getDelayedCalls() has a mistake - it says buildReactor, but I'm pretty certain it means runReactor. A way to avoid this special check would be to cancel the timeout at the end of runReactor, I think. Maybe file a ticket for that?
      2. I think that the return value of flushLoggedErrors should basically always be checked, at least for its length.
  3. twisted/internet/test/test_tls.py
    1. StartTLSClientEndpoint would be improved by the switch protocol ticket, right? Can you add a link to that ticket above WrapperFactory?
  4. I think there are a few more tests that can be converted to use this new API. Can you identify one or two and file tickets for converting them?

27

Changed 2 years ago by itamar

  • owner itamar deleted
  • keywords review added
  • branch_author changed from itamarst, itamar, washort to itamar, washort
  • 2.1 will be addressed by #5393.
  • 2.6.1 will be addressed by #5634.
  • 4 is addressed by #5635. I also want to use this infrastructure in #3204, and I also got the impression that this would supersede #5622.

All other comments were addressed, I hope. I started another buildbot run:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/connect-protocols-5392-4

28

Changed 2 years ago by exarkun

  • owner set to exarkun
  • status changed from new to assigned

29

Changed 2 years ago by exarkun

  • keywords review removed
  • status changed from assigned to new
  • owner changed from exarkun to itamar

Thanks! Looks great, please merge.

30

Changed 2 years ago by itamarst

  • status changed from new to closed
  • resolution set to fixed

(In [34184]) Merge connect-protocols-5392-4: Utility function for connecting two protocols.

Author: itamar, washort Review: exarkun Fixes: #5392

Add new utility method to reactormixins test infrastructure, runProtocolsWithReactor. This connects two protocols with two endpoints, allowing the simplification of many of our more convoluted reactor tests.

Note: See TracTickets for help on using tickets.