Opened 15 years ago

Closed 15 years ago

#2086 defect closed fixed (fixed)

t.words.basesupport attributeerror when "already connecting"

Reported by: Cory Dodt Owned by: Cory Dodt
Priority: highest Milestone:
Component: words Keywords:
Cc: corydodt@… Branch:
Author:

Description

When you attempt to connect a basesupport.AbstractAccount twice, you get an AttributeError, because it attempts to use t.i.error.ConnectionError. The correct exception is t.i.error.ConnectError.

Attaching patch with unit test.

Attachments (1)

basesupport.patch​ (1.3 KB) - added by Cory Dodt 15 years ago.

Download all attachments as: .zip

Change History (9)

Changed 15 years ago by Cory Dodt

Attachment: basesupport.patch​ added

comment:1 Changed 15 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from Stephen Thorne to Cory Dodt

I created a branch, irc-account-2086, and played around with your patch.

It turns out that the callback chain in .logOn is broken, and will end up calling both _loginFailed and _cb_logOn.

Please see the unit tests I've written in twisted.words.test.test_basesupport.

If you can fix .logOn so it has the expected behaviour (fix the failing unit test), I'd appreciate it. Do you svn commit access? Got Combinator set up?

Mark it back to review and give it back to me when you're done.

comment:2 Changed 15 years ago by Stephen Thorne

Type: enhancement → defect

comment:3 Changed 15 years ago by Cory Dodt

Cc: corydodt@… added

Thanks, I'll take a look.

comment:4 Changed 15 years ago by Cory Dodt

Keywords: review added
Owner: changed from Cory Dodt to Stephen Thorne

OK, committed r18119 on the branch.

comment:5 Changed 15 years ago by Cory Dodt

Owner: Stephen Thorne deleted

comment:6 Changed 15 years ago by Cory Dodt

Priority: normal → highest

comment:7 Changed 15 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Cory Dodt

Thanks for adding more tests than strictly necessary for this branch.

A few points:

  • return reason works just as well as reason.raiseException(), and better matches the docstring.
  • test_basesupport.py:61,78,79 are all > 80 columns
  • Please add a docstring for test_alreadyConnecting

Please fix all of these things, then merge.

comment:8 Changed 15 years ago by Cory Dodt

Resolution: → fixed
Status: new → closed

(In [18227]) Author: jerub, moonfallen Reviewer: jml Fixes #2086

Use the right name for the exception, and increase by infinity percent the number of tests for basesupport.

Note: See TracTickets for help on using tickets.