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

[twisted-trac]

	ivank reported
Trac ID	trac#4349
Type	defect
Created	2010-03-05 15:58:57Z

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:

• 4349.patch (656 bytes) - added by MostAwesomeDude on 2011-03-23 05:17:09Z - Trivial fix

Searchable metadata

``` trac-id__4349 4349 type__defect defect reporter__ivank ivank priority__normal normal milestone__ branch__ branch_author__ status__new new resolution__None None component__conch conch keywords__ time__1267804737000000 1267804737000000 changetime__1301352949000000 1301352949000000 version__None None owner__MostAwesomeDude MostAwesomeDude ```

[twisted-trac]

	Automation removed owner

[twisted-trac]

	@exarkun set owner to @MostAwesomeDude

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.

[twisted-trac]

	@exarkun commented

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

[twisted-trac]

	@MostAwesomeDude commented

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?

[twisted-trac]

	@MostAwesomeDude commented

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