#4771 release blocker: regression closed fixed (fixed)
jstrports and therefore buildServiceManager is broken by Twisted 10.2, and has no tests
Reported by: | Glyph | Owned by: | |
---|---|---|---|
Priority: | highest | Milestone: | |
Component: | words | Keywords: | |
Cc: | ralphm, sergej, Todd Pratt | Branch: | |
Author: |
Description
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.
Attachments (2)
Change History (23)
comment:1 Changed 7 years ago by
Component: | core → words |
---|---|
Owner: | changed from Glyph to Jean-Paul Calderone |
comment:2 Changed 7 years ago by
Owner: | changed from Jean-Paul Calderone to Glyph |
---|
comment:3 Changed 7 years ago by
comment:4 Changed 7 years ago by
Cc: | ralphm added |
---|
comment:6 Changed 7 years ago by
Cc: | sergej added |
---|
comment:7 Changed 7 years ago by
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:
sofar = () for (type, value) in _tokenize(description):
if type is _STRING:
sofar += (value,)
elif value==':':
add(sofar) sofar = ()
add(sofar) return args, kw
comment:8 Changed 7 years ago by
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 7 7 sufficient use cases get identified """ 8 8 9 from twisted. application import strports9 from twisted.internet.endpoints import _parse 10 10 11 11 def _parseTCPSSL(factory, domain, port): 12 12 """ For the moment, parse TCP or SSL connections the same """ … … 22 22 23 23 24 24 def parse(description, factory): 25 args, kw = strports._parse(description)25 args, kw = _parse(description) 26 26 return (args[0].upper(),) + _funcs[args[0]](factory, *args[1:], **kw) 27 27 28 28 def client(description, factory):
the real question here is: what should a test assert about this fix?
Changed 7 years ago by
Attachment: | test_jabberjstrports.py added |
---|
comment:10 follow-up: 11 Changed 7 years ago by
That's not bad, as far as it goes. It would be nice to test the interesting functionality of this module as well, though.
comment:11 Changed 7 years ago by
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))
comment:12 Changed 7 years ago by
Sorry:
def test_client(self): from twisted.application.internet import TCPClient got = jstrports.client("tcp:DOMAIN:65535", "Factory") self.assertTrue(isinstance(got, TCPClient))
comment:13 Changed 7 years ago by
Keywords: | review added |
---|
comment:14 Changed 7 years ago by
Keywords: | review removed |
---|
There's a trailing space on the import in jstrports.py, other than that +1
Changed 7 years ago by
Attachment: | twisted-ticket-4771.patch added |
---|
removed trailing space in import of _parse in jstrports.py
comment:15 Changed 7 years ago by
Keywords: | review added |
---|
comment:16 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Glyph to Jean-Paul Calderone |
Status: | new → assigned |
- Copyright date should only include years in which the file actually existed, so test_jabberjstrports.py should just have 2011
- All modules need a docstring. The module docstring for test modules typically points out which implementation module the tests are for.
- All test methods need docstrings explaining what functionality or behavior is being tested.
assertEquals
is preferred overassertEqual
assertIsInstance
is preferred overassertTrue(isinstance(...))
- Nested imports should be avoided where possible
- Since jstrports.py is being updated, its copyright date should be updated
- 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!
comment:17 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:18 Changed 7 years ago by
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?
comment:19 follow-up: 20 Changed 7 years ago by
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?
comment:20 Changed 7 years ago by
comment:21 Changed 7 years ago by
Owner: | Jean-Paul Calderone deleted |
---|
Some notes in case anyone wants to tackle this...
t.w.p.j.jstrports.parse
uses the no-longer-presenttwisted.applications.strports._parse
. This could probably be replaced withtwisted,internet.endpoints._parse
for now.jstrports.parse
should probably be deprecated though.jstrports.parse
is fixed, thenjstrports.client
might start to work again. Although withjstrports.parse
deprecated, that would mean thatjstrports.client
should be deprecated as well, I think.clientFromString(...).connect(factory)
). This service would parallelStreamServerEndpointService
, which does exist, but which applications are expected to use directly (egStreamServerEndpointService(serverFromString(...), factory)
). So recommending a parallel, direct usage of a newStreamClientEndpointService
seems appropriate.