Opened 11 years ago

Closed 10 years ago

#2709 defect closed fixed (fixed)

twisted.conch.ssh.channel has poor tests

Reported by: z3p Owned by:
Priority: highest Milestone:
Component: conch Keywords: soc2007
Cc: therve Branch: branches/channel-2709
branch-diff, diff-cov, branch-cov, buildbot
Author: z3p

Description

Change History (15)

comment:1 Changed 11 years ago by z3p

Keywords: review added
Owner: z3p deleted
Priority: highhighest

Ready for review in channel-2709.

comment:2 Changed 11 years ago by Glyph

Keywords: review removed
Owner: set to z3p

Most of the docstrings for the new tests don't really say what they're testing. "works correctly", "returns an appopriate value", "handles data appropriately", and so on, aren't telling me what the expected results are. For example, in the case of "str", you should say something about what is expected to show up in the str's results. The docstring can then be something like "str(SSHChannel) should return a string describing the channel's foo, bar, and baz attributes at a glance."

Missing docstrings entirely:

  • test_loseConnection
  • test_getPeer
  • test_getHost
  • setUp (why is remoteMaxPacket 10? is that important to the tests? why?)

Docstrings are always required, but they are particularly important in this code because it has historically proven particularly difficult for other maintainers to understand. Test and module docstrings are a good place to provide an overview of the intended functionality of the system rather than the snapshots

The tests' implementations doesn't help me understand the vague docstrings, either. For example, test_init: 131072? 32768? 10? 1, 2, 3, 4, 5, 6, 7? What do any of these things mean? Use some named constants if the values are meaningful, and if not, then comment to that effect.

Since it's really verifying the behavior of some stuff that's going on in setUp, you might want to put test_init specifically on a separate class so that it can set up its own channel inline with the test.

In ...ssh.channel itself:

  • don't ever comment out code. Just delete it. Version control can tell us about it if it's interesting.
  • I'm a bit confused as to the purpose of the changes, but hopefully that will be clearer when I can grok what's going on in the tests.

comment:3 Changed 11 years ago by z3p

Keywords: review added
Owner: z3p deleted

I've tried to add more information to the docstring to answer your questions, and explain the code better in t.c.s.channel. I can't judge how I've done because the code makes sense to me. I'm putting it back in the review category.

comment:4 Changed 11 years ago by therve

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

channel.py:

  • Unused import context
  • update its copyright
  • SSHChannel could declare its implementation of ITransport.
  • there are one -= and one += which need some spaces around

test_channel.py:

  • don't use callbacks named '_', a small meaningful name is better
  • in test_addWindowBytes, you probably want to do self.assertTrue(cb[0]) not self.assertTrue(_)
  • test_init should definitely be in another TestCase

Both files would benefit from a few blank lines.

comment:5 Changed 11 years ago by z3p

I've fixed all but test_init. I'm not sure it deserves its own TestCase; after all, it is testing a function of SSHChannel.

comment:6 Changed 11 years ago by z3p

Keywords: review added

comment:7 Changed 11 years ago by z3p

Owner: z3p deleted

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

Keywords: review removed
Owner: set to z3p
  • in the tests:
    • Make new classes new-style
    • Document the ivars of MockConnection and its sendClose method
  • in the implementation:
    • SSHChannel could use a class docstring with @ivars

The test method docstrings look good to me.

comment:9 Changed 11 years ago by z3p

Keywords: review added
Owner: z3p deleted

Okay, back up for review (channel-2709).

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

Keywords: review removed
Owner: set to z3p

MockTransport's docstring should be more specific about what role the class is intended to fill. Is it a mock for twisted.conch.ssh.transport.SSH{Client,Server}Transport? If so, say that. "Transport" could mean a lot of things.

Everything else looks good.

comment:11 Changed 10 years ago by z3p

(In [22472]) More documentation for MockTransport

Refs #2709

comment:12 Changed 10 years ago by z3p

author: z3p
Branch: branches/channel-2709
Keywords: review added
Owner: z3p deleted

comment:13 Changed 10 years ago by therve

Keywords: review removed
Owner: set to z3p

Great. Please update copyrights of both files and merge.

comment:14 Changed 10 years ago by z3p

Resolution: fixed
Status: newclosed

(In [22626]) merge channel-2709

Author: z3p Reviewer: exarkun, glyph, therve Fixes #2709

Adds explicit tests and some documentation for the code in twisted.conch.ssh.channel. This work was done as part of Google's Summer of Code 2007.

comment:15 Changed 7 years ago by <automation>

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