jstrports and therefore buildServiceManager is broken by Twisted 10.2, and has no tests

[glyph]

In addition to the theoretical concern that this is a regression, at least  one piece of code actually uses this function, so it's broken by 10.2.

I think we should do a point-release to get this fix out once it's done.

[exarkun]

Some notes in case anyone wants to tackle this...

• t.w.p.j.jstrports.parse uses the no-longer-present twisted.applications.strports._parse. This could probably be replaced with twisted,internet.endpoints._parse for now. jstrports.parse should probably be deprecated though.
• If jstrports.parse is fixed, then jstrports.client might start to work again. Although with jstrports.parse deprecated, that would mean that jstrports.client should be deprecated as well, I think.
• The hole this leaves in the API is one shaped like a function that takes an endpoint description (and a reactor and a factory, likely) and returns an IService which uses that factory to connect to that endpoint (ie clientFromString(...).connect(factory)). This service would parallel StreamServerEndpointService, which does exist, but which applications are expected to use directly (eg StreamServerEndpointService(serverFromString(...), factory)). So recommending a parallel, direct usage of a new StreamClientEndpointService seems appropriate.

[sergej]

it also breaks PyICQ jabber transport.

[sergej]

I fixed it by copy-pasting following code from 10.1.0 into strports.py. At least pyicq transport works now.

_OP, _STRING = range(2) def _tokenize(description):

current = ops = ':=' nextOps = {':': ':=', '=': ':'} description = iter(description) for n in description:

if n in ops:

yield _STRING, current yield _OP, n current = ops = nextOps[n]

elif n=='\\':

current += description.next()

else:

current += n

yield _STRING, current

def _parse(description):

args, kw = [], {} def add(sofar):

if len(sofar)==1:

args.append(sofar[0])

else:

kw[sofar[0]] = sofar[1]

sofar = () for (type, value) in _tokenize(description):

if type is _STRING:

sofar += (value,)

elif value==':':

add(sofar) sofar = ()

add(sofar) return args, kw

[glyph]

There's actually a much smaller fix, since this code is still in Twisted:

twisted/words/protocols/jabber/jstrports.py

@@ -6 +6 @@
 """ A temporary placeholder for client-capable strports, until we
 sufficient use cases get identified """
 
-from twisted.application import strports
+from twisted.internet.endpoints import _parse
 
 def _parseTCPSSL(factory, domain, port):
     """ For the moment, parse TCP or SSL connections the same """
@@ -22 +22 @@
 
 
 def parse(description, factory):
-    args, kw = strports._parse(description)
+    args, kw = _parse(description)
     return (args[0].upper(),) + _funcs[args[0]](factory, *args[1:], **kw)
 
 def client(description, factory):

the real question here is: what should a test assert about this fix?

[tpratt]

I propose the attached test_jabberstrports.py

[exarkun]

That's not bad, as far as it goes. It would be nice to test the interesting functionality of this module as well, though.

[tpratt]

What if I add this:

def test_client(self):

from twisted.application.internet import TCPClient got = jstrports.client("tcp:DOMAIN:65535", "Factory") self.assertTrue(isinstance(got, TCPClient))

[tpratt]

Sorry:

    def test_client(self):
        from twisted.application.internet import TCPClient
        got = jstrports.client("tcp:DOMAIN:65535", "Factory")
        self.assertTrue(isinstance(got, TCPClient))

[gxti]

There's a trailing space on the import in jstrports.py, other than that +1

[tpratt]

removed trailing space in import of _parse in jstrports.py

[exarkun]

1. Copyright date should only include years in which the file actually existed, so test_jabberjstrports.py should just have 2011
2. All modules need a docstring. The module docstring for test modules typically points out which implementation module the tests are for.
3. All test methods need docstrings explaining what functionality or behavior is being tested.
4. assertEquals is preferred over assertEqual
5. assertIsInstance is preferred over assertTrue(isinstance(...))
6. Nested imports should be avoided where possible
7. Since jstrports.py is being updated, its copyright date should be updated
8. Changes should be summarized in news fragments

Minor points all, so mainly consider them for future patches. I'll make the fixes and apply the patch. Thanks!

[exarkun]

(In [30430]) Fix jstrports parser regression by using the endpoints parser

Author: tpratt, exarkun Reviewer: exarkun Fixes: #4771

Fix a regression in 10.2 which broke the jstrports APIs by switching to a different private parser API.

[glyph]

Given that this was a regression in 10.2, I think we should backport the fix and do a 10.2.1. Should there be a separate ticket for that?

[exarkun]

Should there be a separate ticket for that?

There is no defined workflow for tracking fix backports. How about let's say try that and then we'll see how it goes and consider basing future policy on the results?

[glyph]

Replying to exarkun:

Should there be a separate ticket for that?

There is no defined workflow for tracking fix backports. How about let's say try that and then we'll see how it goes and consider basing future policy on the results?

OK. #4798 and #4799 - one for the backport, one for the release.