Opened 8 years ago

Last modified 7 years ago

#4349 defect new

twisted.conch.telnet doesn't call enableRemote when using python -O

Reported by: ivank Owned by: Corbin Simpson
Priority: normal Milestone:
Component: conch Keywords:
Cc: Branch:
Author:

Description

twisted.conch.telnet has an assert which should probably not be an assert:

assert self.enableRemote(option), "enableRemote must return True in this context (for option %r)" % (option,)

(after all, enableRemote might actually mutate something, and in this case, it looks like it does.)

python -O `which trial` twisted.conch.test.test_telnet
...

===============================================================================
[FAIL]: twisted.conch.test.test_telnet.TelnetTransportTestCase.testAcceptedEnableRequest

Traceback (most recent call last):
  File "/home/a/Projects/Twisted/twisted/conch/test/test_telnet.py", line 400, in <lambda>
    d.addCallback(lambda _:  self._enabledHelper(h, eR=['\x42']))
  File "/home/a/Projects/Twisted/twisted/conch/test/test_telnet.py", line 231, in _enabledHelper
    self.assertEquals(o.enabledRemote, eR)
twisted.trial.unittest.FailTest: not equal:
a = []
b = ['B']

===============================================================================
[FAIL]: twisted.conch.test.test_telnet.TelnetTransportTestCase.testNegotiationBlocksFurtherNegotiation

Traceback (most recent call last):
  File "/home/a/Projects/Twisted/twisted/conch/test/test_telnet.py", line 464, in <lambda>
    dR=['\x24']))
  File "/home/a/Projects/Twisted/twisted/conch/test/test_telnet.py", line 231, in _enabledHelper
    self.assertEquals(o.enabledRemote, eR)
twisted.trial.unittest.FailTest: not equal:
a = []
b = ['$']

Attachments (1)

4349.patch (656 bytes) - added by Corbin Simpson 7 years ago.
Trivial fix

Download all attachments as: .zip

Change History (6)

comment:1 Changed 7 years ago by <automation>

Owner: z3p deleted

Changed 7 years ago by Corbin Simpson

Attachment: 4349.patch added

Trivial fix

comment:2 Changed 7 years ago by Corbin Simpson

Keywords: review added

Attached a very trivial fix. Conveniently, the existing unit tests cover this just fine; just run them with python -O and they'll test this.

Any complaints?

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

Keywords: review removed
Owner: set to Corbin Simpson

The fix looks great, thanks! That reasoning about the test coverage has a flaw though. The tests will execute the line, but they'll never take the code path which leads to the TelnetError being raised. If they did, they would have already failed under python -O, but there isn't. So, can you add a test? Also, I wonder if raising an exception is really the sensible thing to do here. Alternatives are that we could just log a message saying the application did something ridiculous. On the other hand, it was raising an exception before, so it's fine to continue.

comment:4 Changed 7 years ago by Corbin Simpson

I think raising the exception is the correct thing to do, yes. TelnetError seems like the right choice for this section of code, too.

I am trying to grok these tests and failing hard; I'm hard-pressed to understand the relationship between raising this error and failing those tests. I'll get it eventually; it might just be a bit. :3

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

For the new test, I think you'll just want to trigger this behavior and assert that dataReceived raises the exception you expect.

Note: See TracTickets for help on using tickets.