Opened 11 years ago

Closed 11 years ago

#2707 defect closed fixed (fixed)

twisted.conch.ssh.connection has poor tests

Reported by: z3p Owned by:
Priority: highest Milestone:
Component: conch Keywords: soc2007
Cc: therve Branch:
Author:

Description

The title says it all.

Change History (12)

comment:1 Changed 11 years ago by z3p

Keywords: review added
Owner: z3p deleted

It doesn't work ATM if PyCrypto isn't installed because #2682 isn't in, but otherwise it's ready for review.

comment:2 Changed 11 years ago by z3p

Priority: highhighest

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

Owner: set to Jean-Paul Calderone
Status: newassigned

Reviewing connection-2707

comment:4 Changed 11 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to z3p
Status: assignednew

Lots of ssh_* methods have no docstrings, and SSHConnection has no class docstring. SSHConnection.ssh_GLOBAL_REQUEST should use isinstance, not type() in ....

In the test module, TestChannel should have docs for its attributes and methods. Likewise for the methods of TestAvatar and TestConnection. Test methods mostly look pretty good, but the docstrings should be adjusted to be more specific instead of just saying "appropriately".

comment:5 Changed 11 years ago by z3p

Keywords: review added
Owner: z3p deleted

Ready for review again in branches/connection-2707.

comment:6 Changed 11 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to z3p

A few comments:

  • pydoctor error:
    L8: Error: Improper paragraph indentation.
    these 1 objects' docstrings are not proper epytext:
    ssh.connection.SSHConnection.ssh_CHANNEL_EXTENDED_DATA
    
  • pyflakes errors:
    twisted/conch/test/test_connection.py:10: 'log' imported but unused
    twisted/conch/ssh/connection.py:17: 'protocol' imported but unused
    twisted/conch/ssh/connection.py:17: 'reactor' imported but unused
    twisted/conch/ssh/connection.py:20: 'session' imported but unused
    twisted/conch/ssh/connection.py:20: 'forwarding' imported but unused
    
  • remove relative imports in connections.py
  • update copyright of connection.py, change copyright of test_connection.py to only mention 2007.
  • the self.connection attribute of ConnectionTestCase doesn't seem to be used.
  • there are a few trailing whitespaces in connection.py
  • it would be nice to remove the types import in connection.py and use list/tuple instead

That's it for now!

comment:7 Changed 11 years ago by z3p

Keywords: review added
Owner: z3p deleted

I've addressed these in branches/connection-2707 and putting it back in the review category.

comment:8 Changed 11 years ago by therve

Cool. The last point I'm seeing is at the end of the file, with import and dir usage. This probably could be replaced by locals() or something.

comment:9 Changed 11 years ago by z3p

Owner: set to therve

Okay, I switched it to use locals.

comment:10 Changed 11 years ago by therve

Keywords: review removed
Owner: changed from therve to z3p

That's fine for me. Both files would benefit from some blank lines around (we really need to update the coding standard, I don't event know what's the best for this...), but otherwise it looks great.

comment:11 Changed 11 years ago by z3p

Resolution: fixed
Status: newclosed

Fixed in r20700. (I typed the message wrong).

comment:12 Changed 7 years ago by <automation>

Owner: z3p deleted
Note: See TracTickets for help on using tickets.