Ticket #2710: session-tests.diff

File session-tests.diff, 121.4 KB (added by Jonathan Lange, 13 years ago)

jml's review, attached to make replying easier

Line 
1Hi Thomas & Paul,
2
3This is a review of branches/session-2710-5.
4
5This diff is over 3000 lines long. Although sometimes work can't be broken up,
6you should make an effort. The time it takes to review a branch scales
7non-linearly with the size of the diff.
8
9Before you get into the review, I want to thank both of you for taking the
10time to do this work. Conch more than any other part of Twisted will benefit
11from better tests and being easier to test.
12
13There's going to be another review round-trip and there *are* bits of the diff
14that I have serious issues with. Still, the patch as a whole shows clear
15thinking and a strong desire to separate concerns that were previously mixed
16together. You're attention to backwards compatibility is also greatly
17appreciated.
18
19Because I'm coming to this branch cold, I'm going to be doing a full review.
20Personally, I hate it when an old branch of mine is subject to a fresh review
21by a fresh reviewer -- it means a lot of decisions get questioned again and
22feels like a lot of work little or no reason. It's also likely that the new
23reviewer will contradict the old reviewer.
24
25I'm going to try as hard as I can to focus only on code cleanliness, and to
26assume that you and the previous reviewers knew what they were doing. What
27that means is I'll flag everything that's wrong and everything that I don't
28get, and I'll try to distinguish between the two.
29
30I'd very much appreciate it if you could respond to each of my points, even if
31it is to say "no, I don't think I'll do that". The priority now is getting
32this branch landed. If you reply point-wise, my re-review can probably be done
33trivially.
34
35Also, feel free to grab me on IRC or on skype ('blackjml') if you want to
36discuss any of these points.
37
38{{{
39> Index: twisted/conch/test/test_filetransfer.py
40> ===================================================================
41> --- twisted/conch/test/test_filetransfer.py   (revision 23210)
42> +++ twisted/conch/test/test_filetransfer.py   (working copy)
43> @@ -56,11 +56,6 @@
44>          return os.path.join(os.getcwd(), self.homeDir)
45
46
47> -class ConchSessionForTestAvatar:
48> -
49> -    def __init__(self, avatar):
50> -        self.avatar = avatar
51> -
52>  if unix:
53>      if not hasattr(unix, 'SFTPServerForUnixConchUser'):
54>          # unix should either be a fully working module, or None.  I'm not sure
55> @@ -463,7 +458,7 @@
56>          self.interceptConnectionLost(sftpServer)
57
58>          # close session
59> -        testSession.closeReceived()
60> +        testSession.closed()
61>
62}}}
63
64XXX
65I don't know what this is for. Let's see if I find out.
66
67{{{
68>          self.assertSFTPConnectionLost()
69
70> Index: twisted/conch/scripts/conch.py
71> ===================================================================
72> --- twisted/conch/scripts/conch.py    (revision 23210)
73> +++ twisted/conch/scripts/conch.py    (working copy)
74> @@ -8,20 +8,21 @@
75
76>  #""" Implementation module for the `conch` command.
77>  #"""
78}}}
79
80Please either delete this comment, or uncomment the docstring and make
81it comply with Twisted conventions.
82
83{{{
84> -from twisted.conch.client import agent, connect, default, options
85> +from twisted.conch.client import connect, default, options
86>  from twisted.conch.error import ConchError
87>  from twisted.conch.ssh import connection, common
88>  from twisted.conch.ssh import session, forwarding, channel
89> -from twisted.internet import reactor, stdio, defer, task
90> +from twisted.internet import reactor, stdio, task
91>  from twisted.python import log, usage
92
93> -import os, sys, getpass, struct, tty, fcntl, base64, signal, stat, errno
94> +import os, sys, getpass, struct, tty, fcntl, signal, errno
95
96> +
97>  class ClientOptions(options.ConchOptions):
98> -   
99> +
100>      synopsis = """Usage:   conch [options] host [command]
101>  """
102> -   
103> +
104>      optParameters = [['escape', 'e', '~'],
105>                        ['localforward', 'L', None, 'listen-port:host:port   Forward local port to remote address'],
106>                        ['remoteforward', 'R', None, 'listen-port:host:port
107>      Forward remote port to local address'],
108}}}
109
110Please wrap these lines to fit in 79 columns.
111
112{{{
113> Index: twisted/conch/ssh/session.py
114> ===================================================================
115> --- twisted/conch/ssh/session.py      (revision 23210)
116> +++ twisted/conch/ssh/session.py      (working copy)
117> @@ -1,9 +1,7 @@
118> -# -*- test-case-name: twisted.conch.test.test_conch -*-
119> -# Copyright (c) 2001-2004 Twisted Matrix Laboratories.
120> +# -*- test-case-name: twisted.conch.test.test_session -*-
121> +# Copyright (c) 2001-2007 Twisted Matrix Laboratories.
122}}}
123
124It's 2008 now.
125
126{{{
127>  # See LICENSE for details.
128
129> -#
130> -
131>  """
132>  This module contains the implementation of SSHSession, which (by default)
133>  allows access to a shell and a python interpreter over SSH.
134> @@ -11,214 +9,890 @@
135>  Maintainer: U{Paul Swartz<mailto:z3p@twistedmatrix.com>}
136>  """
137
138> -import struct
139> +import struct, warnings, signal, os, sys
140
141> +from zope.interface import implements
142> +
143>  from twisted.internet import protocol
144> -from twisted.python import log
145> -from twisted.conch.interfaces import ISession
146> -from twisted.conch.ssh import common, channel
147> +from twisted.python import components, log
148> +from twisted.conch.interfaces import ISessionApplication
149> +from twisted.conch.interfaces import ISessionApplicationFactory, ISession
150> +from twisted.conch.ssh import common, channel, connection
151
152> +
153> +# supportedSignals is a list of signals that every session channel is supposed
154> +# to support.
155> +supportedSignals = ["ABRT", "ALRM", "FPE", "HUP", "ILL", "INT", "KILL", "PIPE",
156> +                    "QUIT", "SEGV", "TERM", "USR1", "USR2"]
157> +
158> +
159> +
160>  class SSHSession(channel.SSHChannel):
161> +    """
162> +    A channel implementing the server side of the 'session' channel.  This is
163> +    the channel that a client requests when it wants a subsystem, shell, or
164> +    command.
165
166> +    @ivar earlyData: data sent to this channel before a client is present.
167> +    @type earlyData: C{str}
168> +    @ivar earlyExtended: extended data sent to this channel before a client is
169> +    present.
170> +    @type earlyExtended: C{list}
171> +    @ivar applicationFactory: an object to which requests for shells and such
172> +        are dispatched.  It implements I{ISessionApplicationFactory}.
173> +    @ivar sessionApplication: an object which handles interacting with the
174> +        other side of the channel.  It implements I{ISessionApplication}.
175> +    @ivar client: a C{ProcessProtocol} to which data sent to this channel is
176> +        sent.  This variable is deprecated.
177> +    @type client: L{protocol.ProcessProtocol}
178> +    @ivar session: an object implementing L{ISession} to which requests for
179> +        shells or commands are dispatched.  This variable is deprecated.
180> +    """
181> +
182> +
183> +    # set so that this object can be used as a client channel.
184>      name = 'session'
185> +
186> +
187> +    def __setattr__(self, k, v):
188> +        """
189> +        Trap the 'client' attribute, what used to be the old name (roughly) for
190> +        'sessionApplication', and 'session', which triggers setting up
191> +        _DeprecatedSSHSession as our application factory.
192> +        """
193> +        if k == 'client':
194> +            # Someone is trying to inform us of an old-style client.  Clear the
195> +            # buffers (because this would not have previously delivered any
196> +            # data, only delivered subsequent data) and set the old-style
197> +            # "client" object up as a new-style processProtocol.
198> +            self.earlyData = ''
199> +            self.earlyExtended = []
200> +            self.setupSession(_TransportToProcessProtocol(v.transport))
201> +        if k == 'session' and v is not None:
202> +            # Someone is trying to inform us of an old-style session.  Wrap it
203> +            # in a _DeprecatedSSHSession.
204> +            self.applicationFactory = _DeprecatedSSHSession(self, v)
205> +        self.__dict__[k] = v
206> +
207}}}
208
209I saw the '__setattr__' and had a moment of quiet terror. Now I see that it's
210there for backwards compatibility -- this code will make my life easier, so
211thanks :)
212
213However, I think that you could and should implement this with properties.
214
215{{{
216> +
217> +    def applicationFactory(self):
218> +        """
219> +        Produce an applicationFactory dynamically if one does not yet exist.
220> +        """
221> +        if self.avatar is not None:
222> +            factoryCandidate = ISessionApplicationFactory(self.avatar, None)
223> +            if factoryCandidate is None:
224> +                # Maybe it implements the old version.
225> +                oldStyle = ISession(self.avatar, None)
226> +                if oldStyle is not None:
227> +                    # See __setattr__ above.
228> +                    self.session = oldStyle
229> +                else:
230> +                    # Maybe it doesn't implement either.  The test
231> +                    # SFTP server doesn't implement a session, because
232> +                    # subsystems were just looked up in the avatar.
233> +                    # Use a _SubsystemOnlyApplicationFactory.
234> +                    self.applicationFactory = _SubsystemOnlyApplicationFactory(
235> +                        self)
236> +            else:
237> +                self.applicationFactory = factoryCandidate
238> +            return self.applicationFactory
239> +    applicationFactory = property(applicationFactory)
240> +
241}}}
242
243This method doesn't do what it says it does. The check at the beginning is
244for whether 'self.avatar' is None. Calling this method multiple times will
245create an applicationFactory multiple times. Is this deliberate? If so, please
246update the docstring. If not, then I *think* you need to change the guard at
247the top of the function.
248
249Also, it's not clear to me that applicationFactory should be None if no avatar
250is set. The docstring should state that this is the case and explain why.
251
252{{{
253> +
254> +    def client(self):
255> +        """
256> +        This used to be the C{ProcessProtocol} that was connected to us.
257> +        Since we're not connected to a C{ProcessProtocol} anymore, we
258> +        fake it.
259> +        """
260> +        if isinstance(self.sessionApplication,
261> +                      _ProcessProtocolToSessionApplication):
262> +            return self.sessionApplication.processProtocol
263> +        elif isinstance(self.sessionApplication,
264> +                        SSHSessionProcessProtocolApplication):
265> +            return self.sessionApplication.processProtocol
266> +    client = property(client)
267> +
268}}}
269
270You should extract the if k == 'client' bit out of setattr and make it the
271setter for this property.
272
273Also, I think this should raise a DeprecationWarning.
274
275{{{
276> +
277> +    # This used to be set in the older SSHSession implementation.
278> +    session = None
279> +
280}}}
281
282And this needs to have a getter. I'm sorry, but there's code out there that
283uses this.
284
285{{{
286> +
287>      def __init__(self, *args, **kw):
288>          channel.SSHChannel.__init__(self, *args, **kw)
289> -        self.buf = ''
290> -        self.client = None
291> -        self.session = None
292> +        self.earlyData = ''
293> +        self.earlyExtended = []
294> +        self.sessionApplication = None
295
296> +
297> +    def setupSession(self, sessionApplication):
298> +        """
299> +        Connect us to the application.  We set ourselves as it's channel
300> +        instance variable, and the application becomes our sessionApplication.
301> +        If any data was sent early, send it now.
302> +        """
303}}}
304
305Is this supposed to be public? If not, make it _setUpSession.
306
307Is there already an API or compatibility constraint on the name? If not, make
308it setUpSession.
309
310{{{
311> +        # make sure we have an ISessionApplication
312> +        sessionApplication = ISessionApplication(sessionApplication)
313> +        sessionApplication.channel = self
314> +        sessionApplication.connectionMade()
315> +        # Maybe change this name too...
316}}}
317
318Please say why you think this name should maybe change.
319
320{{{
321> +        self.sessionApplication = sessionApplication
322> +        if self.earlyData:
323> +            b, self.earlyData = self.earlyData, None
324> +            self.dataReceived(b)
325> +        if self.earlyExtended:
326> +            b, self.earlyExtended = self.earlyExtended, None
327> +            for dataType, data in b:
328> +                self.extReceived(dataType, data)
329> +
330}}}
331
332I guess 'b' stands for 'bytes'. Could you please rename it?
333
334{{{
335> +
336>      def request_subsystem(self, data):
337> -        subsystem, ignored= common.getNS(data)
338> +        """
339> +        The remote side has requested a subsystem.  Payload::
340> +            string  subsystem name
341> +
342> +        Try to get a subsystem object by calling our adapter's lookupSubsystem
343> +        method.  If that method returns a subsystem, then connect to our
344> +        client and return True.  Otherwise, return False.
345> +        """
346}}}
347
348I'm surprised you don't return the subsystem. Why don't you?
349
350{{{
351> +        subsystem, rest = common.getNS(data)
352>          log.msg('asking for subsystem "%s"' % subsystem)
353> -        client = self.avatar.lookupSubsystem(subsystem, data)
354> +        client = self.applicationFactory.lookupSubsystem(subsystem, rest)
355>          if client:
356> -            pp = SSHSessionProcessProtocol(self)
357> -            proto = wrapProcessProtocol(pp)
358> -            client.makeConnection(proto)
359> -            pp.makeConnection(wrapProtocol(client))
360> -            self.client = pp
361> -            return 1
362> +            self.setupSession(client)
363> +            return True
364>          else:
365> -            log.msg('failed to get subsystem')
366> -            return 0
367> +            return False
368
369> +
370>      def request_shell(self, data):
371> +        """
372> +        The remote side has requested a shell.  No payload.  Call the
373> +        application factory's openShell() method; it returns something
374> +        implementing I{ISessionApplication} that will become our
375> +        application.  If there's no exception, return True.
376> +        Otherwise, return False.
377> +        """
378>          log.msg('getting shell')
379> -        if not self.session:
380> -            self.session = ISession(self.avatar)
381>          try:
382> -            pp = SSHSessionProcessProtocol(self)
383> -            self.session.openShell(pp)
384> -        except:
385> -            log.deferr()
386> -            return 0
387> +            self.setupSession(self.applicationFactory.openShell())
388> +        except Exception:
389}}}
390
391This looks wrong. Why are you catching all exceptions here and not in other
392'request_' methods?
393
394Also, a bare except is the way to go, as long as you catch and reraise
395KeyboardInterrupt.
396
397{{{
398> +            log.err()
399> +            return False
400>          else:
401> -            self.client = pp
402> -            return 1
403> +            return True
404
405> +
406>      def request_exec(self, data):
407> -        if not self.session:
408> -            self.session = ISession(self.avatar)
409> -        f,data = common.getNS(data)
410> -        log.msg('executing command "%s"' % f)
411> +        """
412> +        The remote side has requested to execute a command.  Payload::
413> +            string  command line
414> +
415> +        Call the application factory's execCommand method with the
416> +        command line.  It should return something implementing
417> +        I{ISessionApplication} that becomes our client.  If there's no
418> +        exception, return True.  Otherwise, return False.
419> +        """
420> +        command, data = common.getNS(data)
421> +        log.msg('executing command "%s"' % command)
422>          try:
423> -            pp = SSHSessionProcessProtocol(self)
424> -            self.session.execCommand(pp, f)
425> -        except:
426> -            log.deferr()
427> -            return 0
428> +            self.setupSession(self.applicationFactory.execCommand(command))
429> +        except Exception:
430> +            log.err()
431> +            return False
432}}}
433
434Same comment as before.
435
436{{{
437>          else:
438> -            self.client = pp
439> -            return 1
440> +            return True
441
442> +
443>      def request_pty_req(self, data):
444> -        if not self.session:
445> -            self.session = ISession(self.avatar)
446> -        term, windowSize, modes = parseRequest_pty_req(data)
447> -        log.msg('pty request: %s %s' % (term, windowSize))
448> +        """
449> +        The remote side has requested a psuedoterminal (PTY).  Payload::
450> +            string  terminal name
451> +            uint32  columns
452> +            uint32  rows
453> +            uint32  xpixels
454> +            uint32  ypixels
455> +            string  modes
456> +
457> +        Modes is::
458> +            0 or more of::
459> +                byte    mode number
460> +                uint32  mode value
461> +
462> +        Call the application factory's getPTY method.  If no exception
463> +        is raised, return True.  Otherwise, return False.
464> +        """
465}}}
466
467Thanks for including these docstrings, btw. They are a real help.
468
469{{{
470>          try:
471> -            self.session.getPty(term, windowSize, modes)
472> -        except:
473> +            term, windowSize, modes = parseRequest_pty_req(data)
474> +            log.msg('pty request: %s %s' % (term, windowSize))
475> +            self.applicationFactory.getPTY(term, windowSize, modes)
476> +        except Exception:
477>              log.err()
478> -            return 0
479> +            return False
480>          else:
481> -            return 1
482> +            return True
483
484> +
485>      def request_window_change(self, data):
486> -        if not self.session:
487> -            self.session = ISession(self.avatar)
488> -        winSize = parseRequest_window_change(data)
489> +        """
490> +        The remote side has changed the window size.  Payload::
491> +            uint32  columns
492> +            uint32  rows
493> +            uint32  xpixels
494> +            uint32  ypixels
495> +
496> +
497> +        Call the application factory's windowChanged method.  If no
498> +        exception is raised, return True.  Otherwise, return False.
499> +        """
500>          try:
501> -            self.session.windowChanged(winSize)
502> -        except:
503> +            winSize = parseRequest_window_change(data)
504> +            self.applicationFactory.windowChanged(winSize)
505> +        except Exception:
506>              log.msg('error changing window size')
507>              log.err()
508> -            return 0
509> +            return False
510>          else:
511> -            return 1
512> +            return True
513>
514}}}
515
516OK, I'm beginning to suspect you just missed out the except clause in
517request_subsytem. Please make sure you add it and switch to bare excepts.
518
519{{{
520> +
521>      def dataReceived(self, data):
522> -        if not self.client:
523> -            #self.conn.sendClose(self)
524> -            self.buf += data
525> +        """
526> +        We got data from the remote side.  If we have an application,
527> +        send the data to it.  Otherwise, buffer the data.
528> +
529> +        @type data: C{str}
530> +        """
531> +        if self.sessionApplication is None:
532> +            self.earlyData += data
533}}}
534
535Perhaps `StringIO` would be more appropriate than a string for `earlyData`. If
536you think so, whack an XXX comment somewhere in this file saying so -- this
537branch is already too big. Be sure to put your name and the date on the
538comment.
539
540{{{
541>              return
542> -        self.client.transport.write(data)
543> +        self.sessionApplication.dataReceived(data)
544
545> +
546>      def extReceived(self, dataType, data):
547> -        if dataType == connection.EXTENDED_DATA_STDERR:
548> -            if self.client and hasattr(self.client.transport, 'writeErr'):
549> -                self.client.transport.writeErr(data)
550> +        """
551> +        We got extended data from the remote side.  If we have an
552> +        application, send the data to it.  Otherwise, buffer the data.
553> +
554> +        @type dataType: C{int}
555> +        @type data: C{str}
556> +        """
557}}}
558
559You know, I'd always wondered what this method was for. Thanks for adding the
560docstring :)
561
562{{{
563> +        if self.sessionApplication is not None:
564> +            self.sessionApplication.extendedDataReceived(dataType, data)
565>          else:
566> -            log.msg('weird extended data: %s'%dataType)
567> +            self.earlyExtended.append((dataType, data))
568
569> +
570>      def eofReceived(self):
571> -        if self.session:
572> -            self.session.eofReceived()
573> -        elif self.client:
574> -            self.conn.sendClose(self)
575> +        """
576> +        The remote side has closed its write side.  If we have an
577> +        application factory, notify it.  If we have an application,
578> +        notify it.
579> +        """
580> +        if self.applicationFactory is not None:
581> +            self.applicationFactory.eofReceived()
582> +        if self.sessionApplication is not None:
583> +            self.sessionApplication.eofReceived()
584
585> +
586>      def closed(self):
587> -        if self.session:
588> -            self.session.closed()
589> -        elif self.client:
590> -            self.client.transport.loseConnection()
591> +        """
592> +        The channel is closed.  If we have an application factory,
593> +        notify it.  If we have an application, tell it the connection
594> +        is lost.
595> +        """
596}}}
597
598It *might* be worth mentioning here that just because the channel is closed,
599that doesn't mean that the SSH connection to the client is closed.
600
601Just a thought.
602
603{{{
604> +        if self.applicationFactory is not None:
605> +            self.applicationFactory.closed()
606> +        if self.sessionApplication is not None:
607> +            self.sessionApplication.closed()
608
609> -    #def closeReceived(self):
610> -    #    self.loseConnection() # don't know what to do with this
611
612> -    def loseConnection(self):
613> -        if self.client:
614> -            self.client.transport.loseConnection()
615> -        channel.SSHChannel.loseConnection(self)
616> +    def closeReceived(self):
617> +        """
618> +        The remote side has requested that we no longer send data.  If
619> +        we have an application factory, notify it.  If we have an
620> +        application, notify it.
621> +        """
622>
623}}}
624
625Good docstring.
626
627{{{
628> -class _ProtocolWrapper(protocol.ProcessProtocol):
629> +        if self.applicationFactory is not None:
630> +            self.applicationFactory.closeReceived()
631> +        if self.sessionApplication is not None:
632> +            self.sessionApplication.closeReceived()
633> +
634> +
635> +    # client methods
636> +
637}}}
638
639'client methods' is ambiguous. Please say what they are used for, why they are
640different to the previous methods and, if you think it's appropriate, why they
641are in this class at all.
642
643{{{
644> +
645> +    def getPty(self, term, windowSize, modes, wantReply=False):
646> +        """
647> +        Request a PTY from the other side.
648> +
649> +        @param term: the type of terminal (e.g. xterm)
650> +        @type term: C{str}
651> +        @param windowSize: the size of the window: (rows, cols, xpixels,
652> +                           ypixels)
653> +        @type windowSize: C{tuple}
654> +        @param modes: termanal modes; a list of (modeNumber, modeValue) pairs.
655}}}
656
657It's spelled 'terminal'. It might be worth putting in a 'see also', so people
658who want to know what modeNumber and modeValue mean can find out.
659
660{{{
661> +        @type modes: C{list}
662> +        @param wantReply: True if we want a reply to this request.
663> +        @type wantReply: C{bool}
664> +
665> +        @returns: if wantReply, a Deferred that will be called back when
666> +                  the request has succeeded or failed; else, None.
667> +        @rtype: C{Deferred}/C{None}
668> +        """
669> +        data = packRequest_pty_req(term, windowSize, modes)
670> +        return self.conn.sendRequest(self, 'pty-req', data,
671> +                wantReply=wantReply)
672> +
673> +
674> +    def openSubsystem(self, subsystem, data='', wantReply=False):
675> +        """
676> +        Request that a subsystem be opened on the other side.
677> +
678> +        @param subsystem: the name of the subsystem
679> +        @type subsystem: C{str}
680> +        @param data: any extra data to send with the request
681> +        @type data: C{str}
682> +        @param wantReply: True if we want a reply to this request.
683> +        @type wantReply: C{bool}
684> +
685> +        @returns: if wantReply, a Deferred that will be called back when
686> +                  the request has succeeded or failed; else, None.
687> +        @rtype: C{Deferred}/C{None}
688> +        """
689> +        return self.conn.sendRequest(self, 'subsystem', common.NS(subsystem) +
690> +                data, wantReply=wantReply)
691> +
692> +
693> +    def openShell(self, wantReply=False):
694> +        """
695> +        Request that a shell be opened on the other side.
696> +
697> +        @param wantReply: True if we want a reply to this request.
698> +        @type wantReply: C{bool}
699> +
700> +        @returns: if wantReply, a Deferred that will be called back when
701> +                  the request has succeeded or failed; else, None.
702> +        @rtype: C{Deferred}/C{None}
703> +        """
704> +        return self.conn.sendRequest(self, 'shell', '', wantReply=wantReply)
705> +
706> +
707> +    def execCommand(self, command, wantReply=False):
708> +        """
709> +        Request that a command be executed on the other side.
710> +
711> +        @param command: the command to execute
712> +        @type command: C{str}
713> +        @param wantReply: True if we want a reply to this request.
714> +        @type wantReply: C{bool}
715> +
716> +        @returns: if wantReply, a Deferred that will be called back when
717> +                  the request has succeeded or failed; else, None.
718> +        @rtype: C{Deferred}/C{None}
719> +        """
720> +        return self.conn.sendRequest(self, 'exec', common.NS(command),
721> +                wantReply=wantReply)
722> +
723> +
724> +    def changeWindowSize(self, windowSize, wantReply=False):
725> +        """
726> +        Inform the other side that the local terminal size has changed.
727> +
728> +        @param windowSize: the new size of the window: (rows, cols, xpixels,
729> +                           ypixels)
730> +        @type windowSize: C{tuple}
731> +        @param wantReply: True if we want a reply to this request.
732> +        @type wantReply: C{bool}
733> +
734> +        @returns: if wantReply, a Deferred that will be called back when
735> +                  the request has succeeded or fails; else, None.
736> +        @rtype: C{Deferred}/C{None}
737> +        """
738> +        data = packRequest_window_change(windowSize)
739> +        return self.conn.sendRequest(self, 'window-change', data,
740> +                wantReply=wantReply)
741> +
742}}}
743
744I'm guessing that this wantReply business is for backwards compat? It seems a
745little strange otherwise.
746
747{{{
748> +
749> +
750> +class _SubsystemOnlyApplicationFactory:
751>      """
752> +    An application factory which which is only good for looking up a
753> +    subsystem.  It is used when there is not an ISession adapter
754> +    defined for the avatar.  Its use is deprecated.
755> +    """
756}}}
757
758If it's deprecated then please make sure it issues a deprecation warning when
759used. See twisted.python.deprecate.
760
761{{{
762> +    implements(ISessionApplicationFactory)
763> +
764> +
765> +    def __init__(self, sessionChannel):
766> +        self.sessionChannel = sessionChannel
767> +
768> +
769> +    def lookupSubsystem(self, subsystem, data):
770> +        """
771> +        The previous ConchUser avatar had subsystems looked up through
772> +        the avatar instead of through the ISession adapter.  To be backwards
773> +        compatible, try to look up the subsystem through the avatar.
774> +        """
775> +        self.sessionChannel.earlyData = ''
776> +        self.sessionChannel.earlyExtended = []
777> +        client = self.sessionChannel.avatar.lookupSubsystem(subsystem,
778> +                                              common.NS(subsystem) + data)
779> +        return wrapProcessProtocol(client)
780> +
781> +
782> +    def eofReceived(self):
783> +        self.sessionChannel.loseConnection()
784> +
785> +
786> +    def closeReceived(self):
787> +        """
788> +        The old closeReceived method did not exist, so we dispatch to our
789> +        parent's method.
790> +        """
791> +        channel.SSHChannel.closeReceived(self.sessionChannel)
792> +
793> +
794> +    def closed(self):
795> +        """
796> +        The old implementation of this method didn't do anything, so we
797> +        don't do anything either.
798> +        """
799> +
800> +
801> +
802> +class _DeprecatedSSHSession(_SubsystemOnlyApplicationFactory):
803> +    """
804> +    This class brings the deprecated functionality of the old SSHSession
805> +    into a single place.
806> +    """
807> +
808> +
809> +    def __init__(self, sessionChannel, oldISessionProvider):
810> +        _SubsystemOnlyApplicationFactory.__init__(self, sessionChannel)
811> +        self.oldISessionProvider = oldISessionProvider
812> +
813> +
814> +    def getPTY(self, term, windowSize, modes):
815> +        """
816> +        The name of this method used to be getPty.
817> +        """
818> +        return self.oldISessionProvider.getPty(term, windowSize, modes)
819> +
820> +
821> +    def openShell(self):
822> +        """
823> +        The old openShell interface passed in a ProcessProtocol which would be
824> +        connected to the shell.
825> +        """
826> +        pp = SSHSessionProcessProtocol(self.sessionChannel)
827> +        self.oldISessionProvider.openShell(pp)
828> +        return SSHSessionProcessProtocolApplication(pp)
829> +
830> +
831> +    def execCommand(self, command):
832> +        """
833> +        The old execCommand interface passed in a ProcessProtocol which would
834> +        be connected to the command.
835> +        """
836> +        pp = SSHSessionProcessProtocol(self.sessionChannel)
837> +        self.oldISessionProvider.execCommand(pp, command)
838> +        return SSHSessionProcessProtocolApplication(pp)
839> +
840> +
841> +    def eofReceived(self):
842> +        """
843> +        The old eofReceived method closed the session connection.
844> +        """
845> +        self.oldISessionProvider.eofReceived()
846> +        self.sessionChannel.conn.sendClose(self.sessionChannel)
847> +
848> +
849> +    def closed(self):
850> +        """
851> +        The old closed method closed the client's transport.
852> +        """
853> +        if self.sessionChannel.sessionApplication is not None:
854> +            self.sessionChannel.sessionApplication.processProtocol.transport.loseConnection()
855> +        self.oldISessionProvider.closed()
856> +
857> +
858> +    def windowChanged(self, newWindowSize):
859> +        """
860> +        Just pass this method through.
861> +        """
862> +        self.oldISessionProvider.windowChanged(newWindowSize)
863> +
864> +
865> +
866> +class _ProtocolToProcessProtocol(protocol.ProcessProtocol):
867> +    """
868>      This class wraps a L{Protocol} instance in a L{ProcessProtocol} instance.
869> +
870> +    @ivar proto: the C{Protocol} we're wrapping.
871>      """
872}}}
873
874This belongs in twisted.internet, not twisted.conch. Please add a XXX comment
875saying so (with name and the date).
876
877{{{
878> +
879> +
880>      def __init__(self, proto):
881>          self.proto = proto
882
883> -    def connectionMade(self): self.proto.connectionMade()
884
885> -    def outReceived(self, data): self.proto.dataReceived(data)
886> +    def __getattr__(self, attr):
887> +        """
888> +        This class did not previously exist, so some uses expect this object
889> +        to implement the Protocol interface.  To handle this case, we pass
890> +        requests through to the wrapped object.
891> +        """
892> +        return getattr(self.proto, attr)
893>
894}}}
895
896Is there a reason for not using proxyForInterfaces?
897
898{{{
899> -    def processEnded(self, reason): self.proto.connectionLost(reason)
900
901> -class _DummyTransport:
902> +    def __setattr__(self, attr, value):
903> +        """
904> +        See the documentation for __getattr__.
905> +        """
906> +        if attr in ('proto', 'transport'):
907> +            self.__dict__[attr] = value
908> +        else:
909> +            setattr(self.proto, attr, value)
910
911> -    def __init__(self, proto):
912> -        self.proto = proto
913
914> -    def dataReceived(self, data):
915> -        self.proto.transport.write(data)
916> +    def connectionMade(self):
917> +        """
918> +        Connect our C{Protocol} to our transport.
919> +        """
920> +        self.proto.makeConnection(self.transport)
921> +        self.transport.proto = self
922
923> -    def write(self, data):
924> +
925> +    def outReceived(self, data):
926> +        """
927> +        Give the data to the C{Protocol}s dataReceived method.
928> +        """
929>          self.proto.dataReceived(data)
930
931> -    def writeSequence(self, seq):
932> -        self.write(''.join(seq))
933
934> -    def loseConnection(self):
935> -        self.proto.connectionLost(protocol.connectionDone)
936> +    def processEnded(self, reason):
937> +        """
938> +        Give the C{Failure} to the C{Protocol}s connectionLost method.
939> +        """
940> +        self.proto.connectionLost(reason)
941
942> +
943> +
944>  def wrapProcessProtocol(inst):
945> +    """
946> +    If we're passed a C{Protocol}, wrap it in a C{ProcessProtocol}.
947> +    Otherwise, just return what we were passed.
948> +    """
949>      if isinstance(inst, protocol.Protocol):
950> -        return _ProtocolWrapper(inst)
951> +        return _ProtocolToProcessProtocol(inst)
952>      else:
953>          return inst
954>
955}}}
956
957Can you use adapters for this instead?
958
959If not, add a XXX comment saying that you *should* use adapters, and ideally,
960file a bug about it.
961
962{{{
963> -def wrapProtocol(proto):
964> -    return _DummyTransport(proto)
965
966> +
967> +class _TransportToProcessProtocol(protocol.ProcessProtocol):
968> +    """
969> +    This wraps something implementing I{ITransport} in a
970> +    C{ProcessProtocol} because that's how the old interface worked.
971> +
972}}}
973
974Please say specifically which object you mean by "the old interface".
975
976Given that this is backwards compatibility code, it would be good to say in
977which version of Twisted this was added. That way, we'll know when we can
978remove it.
979
980{{{
981> +    @ivar applicationTransport: the ITransport we're wrapping.
982> +    """
983> +
984> +
985> +    def __init__(self, applicationTransport):
986> +        self.applicationTransport = applicationTransport
987> +
988> +
989> +    def outReceived(self, data):
990> +        """
991> +        When we get data, write it to our transport.
992> +        """
993> +        self.applicationTransport.write(data)
994> +
995> +
996> +    def errReceived(self, errData):
997> +        """
998> +        When we get extended data, give it to the writeErr method of
999> +        our transport.
1000> +        """
1001> +        # XXX need to test when writeErr isn't available
1002> +        self.applicationTransport.writeErr(errData)
1003> +
1004> +
1005> +    def processEnded(self, reason):
1006> +        """
1007> +        When we're told the process ended, tell the transport to drop
1008> +        the connection.  Yes, this loses data.
1009> +        """
1010> +        self.applicationTransport.loseConnection()
1011> +
1012> +
1013> +
1014> +class _ProcessProtocolToSessionApplication:
1015> +    """
1016> +    This adapts a C{ProcessProtocol} to an ISessionApplication.
1017> +
1018> +    @ivar processProtocol: the C{ProcessProtocol} we're adapting.
1019> +    """
1020> +
1021}}}
1022
1023It would be good to explain in the docstring why you need this adapter.
1024
1025Same comments on including the version apply.
1026
1027{{{
1028> +
1029> +    implements(ISessionApplication)
1030> +
1031> +
1032> +    def __init__(self, processProtocol):
1033> +        self.processProtocol = processProtocol
1034> +
1035> +
1036> +    def connectionMade(self):
1037> +        """
1038> +        Connect the C{ProcessProtocol} to the channel.
1039> +        """
1040> +        self.processProtocol.makeConnection(self.channel)
1041> +
1042> +
1043> +    def dataReceived(self, data):
1044> +        """
1045> +        Give the data to the C{ProcessProtocol}
1046> +        """
1047> +        self.processProtocol.outReceived(data)
1048> +
1049> +
1050> +    def extendedDataReceived(self, type, data):
1051> +        """
1052> +        If the extended data came from standard error, give it to the
1053> +        C{ProcessProtocol}.  Otherwise drop it on the floor.
1054> +        """
1055> +        if type == connection.EXTENDED_DATA_STDERR:
1056> +            self.processProtocol.errReceived(data)
1057> +
1058> +
1059> +    def eofReceived(self):
1060> +        """
1061> +        Tell the C{ProcessProtocol} that no more data will be sent to
1062> +        standard input.
1063> +        """
1064> +        self.processProtocol.inConnectionLost()
1065> +
1066> +
1067> +    def closeReceived(self):
1068> +        """
1069> +        Tell the C{ProcessProtocol} that it shouldn't send any more data.
1070> +        """
1071> +        self.processProtocol.outConnectionLost()
1072> +        self.processProtocol.errConnectionLost()
1073> +
1074> +
1075> +    def closed(self):
1076> +        """
1077> +        Tell the C{ProcessProtocol} that the process ended.
1078> +
1079> +        TODO: catch request_exit_status to give a better error message.
1080> +        """
1081> +        self.processProtocol.processEnded(protocol.connectionDone)
1082> +
1083> +
1084> +
1085> +components.registerAdapter(_ProcessProtocolToSessionApplication,
1086> +                           protocol.ProcessProtocol, ISessionApplication)
1087> +
1088> +
1089> +
1090>  class SSHSessionProcessProtocol(protocol.ProcessProtocol):
1091> +    """
1092> +    This is a C{ProcessProtocol} that is used to connect to old-style
1093> +    ISession implementations.  An instance of this is passed to
1094> +    openShell() and execCommand(), and then it's wrapped using
1095> +    SSHSessionProcessProtocolApplication.
1096> +    """
1097
1098> -#    __implements__ = I
1099> +
1100> +    _signalNames = None
1101> +
1102}}}
1103
1104Please add a comment explaining the structure of this variable.
1105
1106{{{
1107> +
1108>      def __init__(self, session):
1109>          self.session = session
1110
1111> -    def connectionMade(self):
1112> -        if self.session.buf:
1113> -            self.transport.write(self.session.buf)
1114> -            self.session.buf = None
1115
1116> +    def _getSignalName(self, signum):
1117> +        """
1118> +        Get a signal name given a signal number.
1119> +        """
1120> +        if self._signalNames is None:
1121> +            self.__class__._signalNames = {}
1122> +            # make sure that the POSIX ones are the defaults
1123> +            for signame in supportedSignals:
1124> +                signame = 'SIG' + signame
1125> +                sigvalue = getattr(signal, signame, None)
1126> +                if sigvalue is not None:
1127> +                    self._signalNames[sigvalue] = signame
1128> +            for k, v in signal.__dict__.items():
1129> +                if k.startswith('SIG') and not k.startswith('SIG_'):
1130> +                    if v not in self._signalNames:
1131> +                        self._signalNames[v] = k + '@' + sys.platform
1132> +        return self._signalNames[signum]
1133> +
1134}}}
1135
1136So, figuring this out, _signalNames maps from signal numbers to signal names.
1137
1138The names are either the regular names (e.g. 'SIGKILL') or they are names with
1139platforms at the end (e.g. 'SIGCHLD@linux'). They are in the latter if they
1140aren't in 'supportedSignals', which appears to be a global.
1141
1142The 'SIG_' guard is there because the Python signal module has variables that
1143start with 'SIG_' that aren't the names of signals.
1144
1145OK, I think I've got it. Here are the recommendations:
1146
1147 * Make supportedSignals 'SUPPORTED_SIGNALS'.
1148 * If 'supported' really means 'all platforms', then say so.
1149 * The self.__class__ thing smells like premature optimization. Just use self.
1150
1151{{{
1152> +
1153>      def outReceived(self, data):
1154> +        """
1155> +        Sent data from standard out over the channel.
1156> +        """
1157>          self.session.write(data)
1158
1159> +
1160>      def errReceived(self, err):
1161> +        """
1162> +        Send data from standard error as extended data of type
1163> +        EXTENDED_DATA_STDERR.
1164> +        """
1165>          self.session.writeExtended(connection.EXTENDED_DATA_STDERR, err)
1166
1167> +
1168>      def inConnectionLost(self):
1169> +        """
1170> +        If we're told the incoming connection has been lost, send an EOF
1171> +        over the channel.
1172> +        """
1173>          self.session.conn.sendEOF(self.session)
1174
1175> -    def connectionLost(self, reason = None):
1176> -        self.session.loseConnection()
1177
1178> -    def processEnded(self, reason = None):
1179> -        if reason and hasattr(reason.value, 'exitCode'):
1180> -            log.msg('exitCode: %s' % repr(reason.value.exitCode))
1181> -            self.session.conn.sendRequest(self.session, 'exit-status', struct.pack('!L', reason.value.exitCode))
1182> +    def outConnectionLost(self):
1183> +        """
1184> +        If we're running as an subsystem, close the connection.
1185> +        """
1186> +        if self.session.session is None: # we're running as an old-style
1187> +                                         # subsystem
1188> +            self.session.loseConnection()
1189> +
1190> +
1191> +    def processEnded(self, reason=None):
1192> +        """
1193> +        When we are told the process ended, try to notify the other side about
1194> +        how the process ended using the exit-signal or exit-status requests.
1195> +        Also, close the channel.
1196> +        """
1197> +        if reason is not None and hasattr(reason.value, 'exitCode'):
1198> +            err = reason.value
1199> +            if err.signal is not None:
1200> +                signame = self._getSignalName(err.signal)
1201> +                log.msg('exitSignal: %s' % (signame,))
1202> +                if hasattr(os, 'WCOREDUMP') and os.WCOREDUMP(err.status):
1203> +                    coreDumped = 1
1204> +                else:
1205> +                    coreDumped = 0
1206}}}
1207
1208If core was dumped, the log should say so. This is very useful information for
1209debugging.
1210
1211{{{
1212> +                self.session.conn.sendRequest(self.session, 'exit-signal',
1213> +                        common.NS(signame[3:]) + chr(coreDumped) +
1214> +                        common.NS('') + common.NS(''))
1215> +            elif err.exitCode is not None:
1216> +                log.msg('exitCode: %r' % (err.exitCode,))
1217> +                self.session.conn.sendRequest(self.session, 'exit-status',
1218> +                        struct.pack('>L', err.exitCode))
1219}}}
1220
1221What if 'err' has no exitCode and no signal?
1222
1223{{{
1224>          self.session.loseConnection()
1225
1226> -    # transport stuff (we are also a transport!)
1227
1228> +    # also a transport :(
1229}}}
1230
1231I share your sadness, but not your understanding. Why must this be a
1232transport? What can we do about it? Please explain these things in the
1233comment.
1234
1235{{{
1236>      def write(self, data):
1237> +        """
1238> +        If we're used like a transport, just send the data to the channel.
1239> +        """
1240>          self.session.write(data)
1241
1242> -    def writeSequence(self, seq):
1243> -        self.session.write(''.join(seq))
1244
1245>      def loseConnection(self):
1246> +        """
1247> +        If we're use like a transport, send the close message.
1248> +        """
1249}}}
1250
1251'used', not 'use'.
1252
1253{{{
1254>          self.session.loseConnection()
1255
1256> -class SSHSessionClient(protocol.Protocol):
1257
1258> +
1259> +class SSHSessionProcessProtocolApplication:
1260> +    """
1261> +    Another layer of wrapping to make the old-style ISession
1262> +    implemention work.  This adapts SSHSessionProcessProtocol to
1263> +    ISessionApplication.
1264> +
1265> +    @ivar processProtocol: the C{SSHSessionProcessProtocol} we're adapting.
1266> +    """
1267> +
1268}}}
1269
1270Gosh there are a lot of these. I think it would be worth giving a summary in
1271the module-level docstring. I'm starting to find it all a bit confusing.
1272
1273{{{
1274> +
1275> +    implements(ISessionApplication)
1276> +
1277> +
1278> +    def __init__(self, processProtocol):
1279> +        self.processProtocol = processProtocol
1280> +
1281> +
1282> +    def connectionMade(self):
1283> +        """
1284> +        Don't need to do anything because SSHSessionProcessProtocol's
1285> +        transport doesn't do anything with this.
1286> +        """
1287> +
1288> +
1289>      def dataReceived(self, data):
1290> -        if self.transport:
1291> -            self.transport.write(data)
1292> +        """
1293> +        When we get data, write it to the SSHSessionProcessProtocol's
1294> +        transport.
1295> +        """
1296> +        self.processProtocol.transport.write(data)
1297
1298> +
1299> +    def extendedDataReceived(self, dataType, data):
1300> +        """
1301> +        If we get extended data from standard error and the transport
1302> +        has a writeErr method, pass the data along.
1303> +        """
1304> +        if not hasattr(self.processProtocol.transport, 'writeErr'):
1305> +            return
1306> +        if dataType == connection.EXTENDED_DATA_STDERR:
1307> +            self.processProtocol.transport.writeErr(data)
1308> +
1309> +
1310> +    def eofReceived(self):
1311> +        """
1312> +        Don't need to implement this because
1313> +        SSHSessionProcessProtocol's transport doesn't do anything with
1314> +        this.
1315> +        """
1316> +
1317> +
1318> +    def closeReceived(self):
1319> +        """
1320> +        Don't need to implement this because
1321> +        SSHSessionProcessProtocol's transport doesn't do anything with
1322> +        this.
1323> +        """
1324> +
1325> +
1326> +    def closed(self):
1327> +        """
1328> +        Don't need to implement this because
1329> +        SSHSessionProcessProtocol's transport doesn't do anything with
1330> +        this.
1331> +        """
1332> +
1333}}}
1334
1335No adapter registration?
1336
1337{{{
1338> +
1339> +
1340> +class SSHSessionClient(protocol.Protocol):
1341> +    """
1342> +    A class the conch command-line client uses to connect the channel
1343> +    to standard output.  Deprecated.
1344> +    """
1345> +    def dataReceived(self, data):
1346> +        """
1347> +        Send data to the transport.
1348> +        """
1349> +        self.transport.write(data)
1350> +
1351}}}
1352
1353If it's deprecated, raise a warning on construction.
1354
1355{{{
1356> +
1357> +
1358>  # methods factored out to make live easier on server writers
1359>  def parseRequest_pty_req(data):
1360> -    """Parse the data from a pty-req request into usable data.
1361> +    """
1362> +    Parse the data from a pty-req request into usable data.
1363
1364>      @returns: a tuple of (terminal type, (rows, cols, xpixel, ypixel), modes)
1365>      """
1366}}}
1367
1368Please say where 'terminal type' and 'modes' come from.
1369
1370By the way, there's no harm at all in referring to the RFC number and section
1371in a docstring, especially for a protocol as complex as SSH.
1372
1373{{{
1374> @@ -226,30 +900,48 @@
1375>      cols, rows, xpixel, ypixel = struct.unpack('>4L', rest[: 16])
1376>      modes, ignored= common.getNS(rest[16:])
1377>      winSize = (rows, cols, xpixel, ypixel)
1378> -    modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in range(0, len(modes)-1, 5)]
1379> +    modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in
1380> +            range(0, len(modes)-1, 5)]
1381}}}
1382
1383There should be no space after the colon, and spaces around the '-' in
1384'len(modes)-1'.
1385
1386{{{
1387>      return term, winSize, modes
1388
1389> +
1390> +
1391>  def packRequest_pty_req(term, (rows, cols, xpixel, ypixel), modes):
1392> -    """Pack a pty-req request so that it is suitable for sending.
1393> +    """
1394> +    Pack a pty-req request so that it is suitable for sending.
1395
1396>      NOTE: modes must be packed before being sent here.
1397>      """
1398>      termPacked = common.NS(term)
1399>      winSizePacked = struct.pack('>4L', cols, rows, xpixel, ypixel)
1400> -    modesPacked = common.NS(modes) # depend on the client packing modes
1401> +    if not isinstance(modes, str):
1402> +        modes = ''.join([chr(m[0]) + struct.pack('>L', m[1]) for m in modes])
1403> +    else:
1404> +        warnings.warn("packRequest_pty_req should be packing the modes.",
1405> +                      DeprecationWarning, stacklevel=2)
1406}}}
1407
1408There's an API for this now: twisted.python.deprecate.
1409
1410{{{
1411> +    modesPacked = common.NS(modes)
1412>      return termPacked + winSizePacked + modesPacked
1413
1414> +
1415> +
1416>  def parseRequest_window_change(data):
1417> -    """Parse the data from a window-change request into usuable data.
1418> +    """
1419> +    Parse the data from a window-change request into usuable data.
1420
1421>      @returns: a tuple of (rows, cols, xpixel, ypixel)
1422>      """
1423>      cols, rows, xpixel, ypixel = struct.unpack('>4L', data)
1424>      return rows, cols, xpixel, ypixel
1425
1426> +
1427> +
1428>  def packRequest_window_change((rows, cols, xpixel, ypixel)):
1429> -    """Pack a window-change request so that it is suitable for sending.
1430>      """
1431> +    Pack a window-change request so that it is suitable for sending.
1432> +    """
1433>      return struct.pack('>4L', cols, rows, xpixel, ypixel)
1434
1435> -import connection
1436> +
1437> +
1438> +__all__ = ['SSHSession', 'packRequest_window_change', 'packRequest_pty_req',
1439> +           'parseRequest_window_change', 'parseRequest_pty_req']
1440}}}
1441
1442I think the world would be a happier place if this were at the top of the
1443file.
1444
1445{{{
1446> Index: twisted/conch/avatar.py
1447> ===================================================================
1448> --- twisted/conch/avatar.py   (revision 23210)
1449> +++ twisted/conch/avatar.py   (working copy)
1450> @@ -1,9 +1,11 @@
1451>  # -*- test-case-name: twisted.conch.test.test_conch -*-
1452> -from interfaces import IConchUser
1453> -from error import ConchError
1454> -from ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE
1455> -from twisted.python import log
1456> +# Copyright (c) 2007 Twisted Matrix Laboratories.
1457> +# See LICENSE for details.
1458> +
1459}}}
1460
14612008 :)
1462
1463{{{
1464>  from zope import interface
1465> +from twisted.conch.interfaces import IConchUser
1466> +from twisted.conch.error import ConchError
1467> +from twisted.conch.ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE
1468
1469>  class ConchUser:
1470>      interface.implements(IConchUser)
1471> @@ -17,12 +19,16 @@
1472>          if not klass:
1473>              raise ConchError(OPEN_UNKNOWN_CHANNEL_TYPE, "unknown channel")
1474>          else:
1475> -            return klass(remoteWindow = windowSize,
1476> -                         remoteMaxPacket = maxPacket,
1477> +            return klass(remoteWindow=windowSize,
1478> +                         remoteMaxPacket=maxPacket,
1479>                           data=data, avatar=self)
1480
1481>      def lookupSubsystem(self, subsystem, data):
1482> -        log.msg(repr(self.subsystemLookup))
1483> +        """
1484> +        This is deprecated in favor of ISession.lookupSubsystem.  However,
1485> +        because there is already a warning in SSHSession.request_subsystem,
1486> +        we do not warn again here.
1487> +        """
1488>          klass = self.subsystemLookup.get(subsystem, None)
1489>          if not klass:
1490>              return False
1491> Index: twisted/conch/interfaces.py
1492> ===================================================================
1493> --- twisted/conch/interfaces.py       (revision 23210)
1494> +++ twisted/conch/interfaces.py       (working copy)
1495> @@ -4,7 +4,7 @@
1496>  from zope.interface import Interface
1497
1498>  class IConchUser(Interface):
1499> -    """A user who has been authenticated to Cred through Conch.  This is
1500> +    """A user who has been authenticated to Cred through Conch.  This is
1501>      the interface between the SSH connection and the user.
1502
1503>      @ivar conn: The SSHConnection object for this user.
1504> @@ -30,26 +30,24 @@
1505>          @rtype:             subclass of L{SSHChannel}/C{tuple}
1506>          """
1507
1508> -    def lookupSubsystem(subsystem, data):
1509> -        """
1510> -        The other side requested a subsystem.
1511> -        subsystem is the name of the subsystem being requested.
1512> -        data is any other packet data (often nothing).
1513> -       
1514> -        We return a L{Protocol}.
1515> -        """
1516
1517> +
1518>      def gotGlobalRequest(requestType, data):
1519>          """
1520>          A global request was sent from the other side.
1521> -       
1522> +
1523>          By default, this dispatches to a method 'channel_channelType' with any
1524> -        non-alphanumerics in the channelType replace with _'s.  If it cannot
1525> -        find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error.
1526> +        non-alphanumerics in the channelType replace with _'s.  If it cannot
1527> +        find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error.
1528>          The method is called with arguments of windowSize, maxPacket, data.
1529>          """
1530
1531> +
1532>  class ISession(Interface):
1533> +    """
1534> +    The old, deprecated interface for implementing SSH session applications.
1535> +    Please don't use this for anything.
1536> +    """
1537>
1538}}}
1539
1540Please say which version it was deprecated in.
1541
1542{{{
1543>      def getPty(term, windowSize, modes):
1544>          """
1545> @@ -82,13 +80,143 @@
1546>          """
1547>          Called when the other side has indicated no more data will be sent.
1548>          """
1549> -       
1550> +
1551>      def closed():
1552>          """
1553>          Called when the session is closed.
1554>          """
1555
1556
1557> +
1558> +
1559> +class ISessionApplicationFactory(Interface):
1560> +    """
1561> +    The new, good interface for implementing an SSH session.  Use this to
1562> +    implement the application side of an SSH server session.
1563> +
1564> +    @ivar channel: the L{SSHChannel} object to which this application is
1565> +    connected.  Use this attribute to send data, etc.
1566> +    """
1567> +
1568> +    def getPTY(term, windowSize, modes):
1569> +        """
1570> +        Get a psuedo-terminal for use by a shell or command.
1571> +
1572> +        If a psuedo-terminal is not available, or the request otherwise
1573> +        fails, raise an exception.
1574> +        """
1575> +
1576}}}
1577
1578"pseudo", not "psuedo".
1579
1580{{{
1581> +
1582> +    def lookupSubsystem(subsystem, data):
1583> +        """
1584> +        The other side requested a subsystem.
1585}}}
1586
1587Blank line.
1588
1589{{{
1590> +        @param subsystem: the name of the subsystem being requested.
1591> +        @param data: any other packet data (often nothing).
1592> +
1593> +        @return: something that implements I{IConchApplication}
1594> +        @raises: If the subsystem is unavailable, raise an exception.
1595> +        """
1596> +
1597> +
1598> +    def openShell():
1599> +        """
1600> +        The other side requested a shell.
1601> +
1602> +        @return: something that implementes I{IConchApplication}
1603> +        @raises: If the subsystem is unavailable, raise an exception.
1604> +        """
1605> +
1606> +
1607> +    def execCommand(command):
1608> +        """
1609> +        The other side requested a shell.
1610> +
1611> +        @param command: the command the other side wants us to execute
1612> +        @type command: C{str}
1613> +
1614> +        @return: something that implementes I{IConchApplication}
1615> +        @raises: If the subsystem is unavailable, raise an exception.
1616> +        """
1617> +
1618> +
1619> +    def windowChanged(newWindowSize):
1620> +        """
1621> +        Called when the size of the remote screen has changed.
1622> +        """
1623> +
1624> +
1625> +    def eofReceived():
1626> +        """
1627> +        Called when the other side has indicated no more data will be sent.
1628> +        """
1629> +
1630> +
1631> +    def closeReceived():
1632> +        """
1633> +        Called when the other side has indicated it wants no more data.
1634> +        """
1635> +
1636> +
1637> +    def closed():
1638> +        """
1639> +        Called when the session channel is closed.
1640> +        """
1641> +
1642> +
1643}}}
1644
1645One more blank line please.
1646
1647{{{
1648> +class ISessionApplication(Interface):
1649> +    """
1650> +    Objects which are returned by an ISessionApplicationFactory implementor
1651> +    should implement this class.  It receives data from the channel and can
1652> +    send it back.
1653> +
1654> +    @ivar channel: the C{SSHSession} to which this application is connected.
1655> +    """
1656> +
1657> +
1658> +    def connectionMade():
1659> +        """
1660> +        Called when this application is connected to the channel.
1661> +        """
1662> +
1663> +
1664> +    def dataReceived(data):
1665> +        """
1666> +        Called when data is received from the other side.
1667> +
1668> +        @type data: C{str}
1669> +        """
1670> +
1671> +
1672> +    def extendedDataReceived(extendedDataType, extendedData):
1673> +        """
1674> +        Called when extended data is received from the other side.
1675> +
1676> +        @param extendedDataType: the type of extended data.  Typically, this
1677> +            is connection.EXTENDED_DATA_STDERR.
1678> +        @type extendedDataType: C{int}
1679> +        @type extendedData: C{str}
1680> +        """
1681> +
1682> +
1683> +    def eofReceived():
1684> +        """
1685> +        Called when the other side has said it will not send any more data.
1686> +        """
1687> +
1688> +
1689> +    def closeReceived():
1690> +        """
1691> +        Called when the other side has said it will not accept any more data.
1692> +        """
1693> +
1694> +
1695> +    def closed():
1696> +        """
1697> +        Called when the application is closed on both sides.
1698> +        """
1699> +
1700> +
1701> +
1702>  class ISFTPServer(Interface):
1703>      """
1704>      The only attribute of this class is "avatar".  It is the avatar
1705> @@ -350,5 +478,3 @@
1706>          @param attrs: a dictionary in the same format as the attrs argument to
1707>          L{openFile}.
1708>          """
1709> -
1710> -
1711> Index: twisted/conch/unix.py
1712> ===================================================================
1713> --- twisted/conch/unix.py     (revision 23210)
1714> +++ twisted/conch/unix.py     (working copy)
1715> @@ -5,19 +5,20 @@
1716>  from twisted.python import components, log
1717>  from twisted.internet.error import ProcessExitedAlready
1718>  from zope import interface
1719> -from ssh import session, forwarding, filetransfer
1720> -from ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND, FXF_CREAT, FXF_TRUNC, FXF_EXCL
1721> +from twisted.conch.ssh import session, forwarding, filetransfer
1722> +from twisted.conch.ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND
1723> +from twisted.conch.ssh.filetransfer import FXF_CREAT, FXF_TRUNC, FXF_EXCL
1724>  from twisted.conch.ls import lsLine
1725
1726> -from avatar import ConchUser
1727> -from error import ConchError
1728> -from interfaces import ISession, ISFTPServer, ISFTPFile
1729> +from twisted.conch.avatar import ConchUser
1730> +from twisted.conch.error import ConchError
1731> +from twisted.conch.interfaces import ISession, ISFTPServer, ISFTPFile
1732> +from twisted.conch import ttymodes
1733
1734>  import struct, os, time, socket
1735>  import fcntl, tty
1736>  import pwd, grp
1737>  import pty
1738> -import ttymodes
1739
1740>  try:
1741>      import utmp
1742> @@ -48,9 +49,6 @@
1743>                  {"session": session.SSHSession,
1744>                   "direct-tcpip": forwarding.openConnectForwardingClient})
1745
1746> -        self.subsystemLookup.update(
1747> -                {"sftp": filetransfer.FileTransferServer})
1748> -
1749>      def getUserGroupId(self):
1750>          return self.pwdData[2:4]
1751
1752> @@ -67,10 +65,10 @@
1753>          hostToBind, portToBind = forwarding.unpackGlobal_tcpip_forward(data)
1754>          from twisted.internet import reactor
1755>          try: listener = self._runAsUser(
1756> -                            reactor.listenTCP, portToBind,
1757> +                            reactor.listenTCP, portToBind,
1758>                              forwarding.SSHListenForwardingFactory(self.conn,
1759>                                  (hostToBind, portToBind),
1760> -                                forwarding.SSHListenServerForwardingChannel),
1761> +                                forwarding.SSHListenServerForwardingChannel),
1762>                              interface = hostToBind)
1763>          except:
1764>              return 0
1765> @@ -95,7 +93,8 @@
1766>          # remove all listeners
1767>          for listener in self.listeners.itervalues():
1768>              self._runAsUser(listener.stopListening)
1769> -        log.msg('avatar %s logging out (%i)' % (self.username, len(self.listeners)))
1770> +        log.msg('avatar %s logging out (%i)' % (self.username,
1771> +            len(self.listeners)))
1772
1773>      def _runAsUser(self, f, *args, **kw):
1774>          euid = os.geteuid()
1775> @@ -160,7 +159,6 @@
1776>          b = utmp.UtmpRecord(utmp.WTMP_FILE)
1777>          b.pututline(entry)
1778>          b.endutent()
1779> -                           
1780
1781>      def getPty(self, term, windowSize, modes):
1782>          self.environ['TERM'] = term
1783> @@ -168,9 +166,13 @@
1784>          self.modes = modes
1785>          master, slave = pty.openpty()
1786>          ttyname = os.ttyname(slave)
1787> -        self.environ['SSH_TTY'] = ttyname
1788> +        self.environ['SSH_TTY'] = ttyname
1789>          self.ptyTuple = (master, slave, ttyname)
1790
1791> +    def lookupSubsystem(subsystem, data):
1792> +        if subsystem == 'sftp':
1793> +            return filetransfer.FileTransferServer(data)
1794> +
1795>      def openShell(self, proto):
1796>          from twisted.internet import reactor
1797>          if not self.ptyTuple: # we didn't get a pty-req
1798> @@ -191,7 +193,7 @@
1799>                    shell, ['-%s' % shellExec], self.environ, homeDir, uid, gid,
1800>                     usePTY = self.ptyTuple)
1801>          self.addUTMPEntry()
1802> -        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ,
1803> +        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ,
1804>                      struct.pack('4H', *self.winSize))
1805>          if self.modes:
1806>              self.setModes()
1807> @@ -232,7 +234,7 @@
1808>          finally:
1809>              os.setegid(egid)
1810>              os.seteuid(euid)
1811> -       
1812> +
1813>      def setModes(self):
1814>          pty = self.pty
1815>          attr = tty.tcgetattr(pty.fileno())
1816> @@ -276,7 +278,7 @@
1817
1818>      def windowChanged(self, winSize):
1819>          self.winSize = winSize
1820> -        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ,
1821> +        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ,
1822>                          struct.pack('4H', *self.winSize))
1823
1824>      def _writeHack(self, data):
1825}}}
1826
1827Here the diff ends, what follows is the added test file.
1828
1829{{{
1830> === twisted/conch/test/test_session.py ===
1831> # Copyright (c) 2007-2008 Twisted Matrix Laboratories.
1832> # See LICENSE for details.
1833>
1834> """
1835> Tests for the 'session' channel implementation in twisted.conch.ssh.session.
1836> """
1837>
1838}}}
1839
1840This is a good place to refer to the RFC.
1841
1842{{{
1843> import os, signal, sys, struct
1844>
1845> from zope.interface import implements
1846>
1847> from twisted.conch.ssh import common, connection, session
1848> from twisted.internet import defer, protocol, error
1849> from twisted.python import components, failure
1850> from twisted.trial import unittest
1851>
1852>
1853>
1854> class SubsystemOnlyAvatar(object):
1855>     """
1856>     A stub class representing an avatar that is only useful for
1857>     getting a subsystem.
1858>     """
1859>
1860>
1861>     def lookupSubsystem(self, name, data):
1862>         """
1863>         If the other side requests the 'subsystem' subsystem, allow it by
1864>         returning a MockProcessProtocol to implement it.  Otherwise, fail.
1865>         """
1866>         if name == 'subsystem':
1867>             return MockProcessProtocol()
1868>
1869}}}
1870
1871When I read 'fail', I think 'raise some sort of error', not 'return None'. The
1872docstring would be more helpful if it explained the impact of returning None
1873in this case.
1874
1875{{{
1876>
1877>
1878> class StubOldAvatar(object):
1879>     """
1880>     A stub class representing the avatar representing the authenticated user.
1881>     """
1882>
1883}}}
1884
1885Please explain why it's 'Old'.
1886
1887{{{
1888>
1889>     def lookupSubsystem(self, name, data):
1890>         """
1891>         If the user requests the TestSubsystem subsystem, connect them
1892>         to our MockProcessProtocol.  If they request the protocol
1893>         subsystem, connect them to a MockProtocol.
1894>         """
1895>         if name == 'TestSubsystem':
1896>             self.subsystem = MockProcessProtocol()
1897>             self.subsystem.packetData = data
1898>             return self.subsystem
1899>         elif name == 'protocol':
1900>             self.subsystem = MockProtocol()
1901>             self.subsystem.packetData = data
1902>             return self.subsystem
1903>
1904}}}
1905
1906And if they request neither, what happens?
1907
1908{{{
1909>
1910>
1911> class StubSessionForStubOldAvatar(object):
1912>     """
1913>     A stub ISession implementation for our StubOldAvatar.
1914>
1915>     @ivar avatar: the C{StubOldAvatar} we are adapting.
1916>     @ivar ptyRequest: if present, the terminal, window size, and modes passed
1917>         to the getPty method.
1918>     @ivar windowChange: if present, the window size passed to the
1919>         windowChangned method.
1920>     @ivar shellProtocol: if present, the C{SSHSessionProcessProtocol} passed
1921>         to the openShell method.
1922>     @ivar shellTransport: if present, the C{EchoTransport} connected to
1923>         shellProtocol.
1924>     @ivar execProtocol: if present, the C{SSHSessionProcessProtocol} passed
1925>         to the execCommand method.
1926>     @ivar execTransport: if present, the C{EchoTransport} connected to
1927>         execProtocol.
1928>     @ivar execCommandLine: if present, the command line passed to the
1929>         execCommand method.
1930>     @ivar gotEOF: if present, an EOF message was received.
1931>     @ivar gotClosed: if present, a closed message was received.
1932>     """
1933>
1934}}}
1935
1936I guess you have all of those here so you can manipulate them from tests.
1937
1938Looking down, I guessed wrong. These are like a log of the things that have
1939been done to this session object. It's worth making a general comment about
1940this in the docstring.
1941
1942You should also mention that certain calls to certain methods will trigger
1943errors.
1944
1945Note that a more usual way of doing this is to have a 'log' or 'calls'
1946attribute that appends information about the calls to a list. No need to
1947change it though.
1948
1949{{{
1950>
1951>     implements(session.ISession)
1952>
1953>
1954>     def __init__(self, avatar):
1955>         """
1956>         Store the avatar we're adapting.
1957>         """
1958>         self.avatar = avatar
1959>         self.shellProtocol = None
1960>
1961>
1962>     def getPty(self, terminal, window, modes):
1963>         """
1964>         If the terminal is 'bad', fail.  Otherwise, store the information in
1965>         the ptyRequest variable.
1966>         """
1967>         if terminal != 'bad':
1968>             self.ptyRequest = (terminal, window, modes)
1969>         else:
1970>             raise RuntimeError('not getting a pty')
1971>
1972>
1973>     def windowChanged(self, window):
1974>         """
1975>         If all the window sizes are 0, fail.  Otherwise, store the size in the
1976>         windowChange variable.
1977>         """
1978>         if window == (0, 0, 0, 0):
1979>             raise RuntimeError('not changing the window size')
1980>         else:
1981>             self.windowChange = window
1982>
1983>
1984>     def openShell(self, pp):
1985>         """
1986>         If we have gotten a shell request before, fail.  Otherwise, store the
1987>         process protocol in the shellProtocol variable, connect it to the
1988>         EchoTransport and store that as shellTransport.
1989>         """
1990>         if self.shellProtocol is not None:
1991>             raise RuntimeError('not getting a shell this time')
1992>         else:
1993>             self.shellProtocol = pp
1994>             self.shellTransport = EchoTransport(pp)
1995>
1996>
1997>     def execCommand(self, pp, command):
1998>         """
1999>         If the command is 'true', store the command, the process protocol, and
2000>         the transport we connect to the process protocol.  Otherwise, just
2001>         store the command and raise an error.
2002>         """
2003>         self.execCommandLine = command
2004>         if command == 'true':
2005>             self.execProtocol = pp
2006>         elif command[:4] == 'echo':
2007>             self.execProtocol = pp
2008>             self.execTransport = EchoTransport(pp)
2009>             pp.outReceived(command[5:])
2010>         else:
2011>             raise RuntimeError('not getting a command')
2012>
2013>
2014>     def eofReceived(self):
2015>         """
2016>         Note that EOF has been received.
2017>         """
2018>         self.gotEOF = True
2019>
2020>
2021>     def closed(self):
2022>         """
2023>         Note that close has been received.
2024>         """
2025>         self.gotClosed = True
2026>
2027>
2028>
2029> components.registerAdapter(StubSessionForStubOldAvatar, StubOldAvatar,
2030>         session.ISession)
2031>
2032>
2033>
2034> class StubNewAvatar(object):
2035>     """
2036>     A stub avatar.  It does not need any methods, as it is only used as the
2037>     object StubSessionForStubNewAvatar adapts.
2038>     """
2039>
2040>
2041>
2042> class StubApplicationFactoryForStubNewAvatar:
2043>     """
2044>     A stub ISessionApplicationFactory implementation for our StubNewAvatar.
2045>
2046>     @ivar avatar: the C{StubOldAvatar} we are adapting.
2047>     @ivar inConnectionOpen: C{True} if the input side is open.
2048>     @ivar outConnectionOpen: C{True} if the output side is open.
2049>     @ivar ended: C{True} if the session has ended.
2050>     @ivar subsystem: if present, the C{MockApplication} that is the
2051>         current subsystem.  It has a packetData ivar which is the C{str} of
2052>         data passed in the subsystem request.
2053>     @ivar term: if present, the terminal name passed to getPty
2054>     @ivar windowSize: if present, the window size passed to getPty or
2055>         windowChanged
2056>     @ivar modes: if present, the terminal modes passed to getPty
2057>     @ivar command: if present, the C{MockApplication} that is the
2058>         current command.  It has a command ivar which is the C{str} giving
2059>         the command's name.
2060>     @ivar shell: if present, the C{MockApplication} that is the current
2061>         shell.
2062>     """
2063>
2064>
2065>     implements(session.ISessionApplicationFactory)
2066>
2067>
2068>     def __init__(self, avatar):
2069>         self.avatar = avatar
2070>         self.inConnectionOpen = True
2071>         self.outConnectionOpen = True
2072>         self.ended = False
2073>         self.subsystem = None
2074>         self.command = None
2075>         self.shell = None
2076>
2077>
2078>     def lookupSubsystem(self, subsystem, data):
2079>         """
2080>         Request a subsystem.  If one has been requested already, raise an
2081>         exception.  Otherwise, set self.subsystem to a MockApplication and
2082>         return it.
2083>         """
2084>         if self.subsystem is not None:
2085>             raise ValueError('already opened a subsystem')
2086>         if subsystem == 'echo':
2087>             self.subsystem = MockApplication()
2088>             self.subsystem.packetData = data
2089>             return self.subsystem
2090>
2091>
2092>     def getPTY(self, term, windowSize, modes):
2093>         """
2094>         Request a psuedoterminal.  Store the data passed to us.
2095>         """
2096}}}
2097
2098"pseudo", not "psuedo".
2099
2100{{{
2101>         self.term = term
2102>         self.windowSize = windowSize
2103>         self.modes = modes
2104>
2105>
2106>     def windowChanged(self, newSize):
2107>         """
2108>         The window size has changed.  Store the data.
2109>         """
2110>         self.windowSize = newSize
2111>
2112>
2113>     def execCommand(self, command):
2114>         """
2115>         Request a command.  If one has been requested already, raise an
2116>         exception.  Otherwise, set self.command to a MockApplication and
2117>         return it.
2118>         """
2119>
2120>         if self.command is not None:
2121>             raise RuntimeError('already executed a command')
2122>         self.command = MockApplication()
2123>         self.command.command = command
2124>         return self.command
2125>
2126>
2127>     def openShell(self):
2128>         """
2129>         Request a shell.  If one has been requested already, raise an
2130>         exception.  Otherwise, set self.shell to a MockApplication and
2131>         return it.
2132>         """
2133>
2134>         if self.shell is not None:
2135>             raise RuntimeError('already opened a shell')
2136>         self.shell = MockApplication()
2137>         return self.shell
2138>
2139>
2140>     def eofReceived(self):
2141>         """
2142>         Close the input side.
2143>         """
2144>         self.inConnectionOpen = False
2145>
2146>
2147>     def closeReceived(self):
2148>         """
2149>         Close the output side.
2150>         """
2151>         self.outConnectionOpen = False
2152>
2153>
2154>     def closed(self):
2155>         """
2156>         End the session.
2157>         """
2158>         self.ended = True
2159>
2160}}}
2161
2162I see you've taken the same approach here. The same comments apply.
2163
2164{{{
2165>
2166>
2167> components.registerAdapter(StubApplicationFactoryForStubNewAvatar,
2168>                            StubNewAvatar,
2169>                            session.ISessionApplicationFactory)
2170>
2171>
2172>
2173> class MockApplication:
2174>     """
2175>     A mock ISessionApplication.  This is the new interface that
2176>     clients of SSHSession should implement.
2177>
2178>     @ivar data: a C{list} of C{str} passed to our dataReceived.
2179>     @ivar extendedData: a C{list} of C{tuple} of (C{int}, C{str})
2180>         passed to our extendedDataReceived.
2181>     @ivar hasClosed: a C{bool} indicating whether the application is closed.
2182>     @ivar gotEOF: if present, we have received an EOF from the other side.
2183>     @ivar gotClose: if present, we have received a close message from the other
2184>        side
2185>     """
2186>
2187>
2188>     implements(session.ISessionApplication)
2189>
2190>
2191>     def connectionMade(self):
2192>         """
2193>         Called when the application is connected to the other side.
2194>         Initialize our instance variables.
2195>         """
2196>         self.data = []
2197>         self.extendedData = []
2198>         self.hasClosed = False
2199>
2200>
2201>     def dataReceived(self, data):
2202>         """
2203>         We got some data.  Store it, and echo it back with a tilde appended.
2204>         """
2205}}}
2206
2207Why?
2208
2209{{{
2210>         self.data.append(data)
2211>         self.channel.write(data + '~')
2212>
2213>
2214>     def extendedDataReceived(self, type, data):
2215>         """
2216>         We got some extended data.  Store it, and echo it back with an
2217>         incremented data type and with a tilde appended to the data.
2218>         """
2219}}}
2220
2221Again, please say _why_ you are appending a tilde.
2222
2223{{{
2224>         self.extendedData.append((type, data))
2225>         self.channel.writeExtended(type + 1, data + '~')
2226>
2227>
2228>     def eofReceived(self):
2229>         """
2230>         Note that we received an EOF.
2231>         """
2232>         self.gotEOF = True
2233>
2234>
2235>     def closeReceived(self):
2236>         """
2237>         Note that we received a close message.
2238>         """
2239>         self.gotClose = True
2240>
2241>
2242>     def closed(self):
2243>         """
2244>         Note that this application is closed.
2245>         """
2246>         self.hasClosed = True
2247>
2248>
2249>
2250> class MockApplicationSendingOnConnection(MockApplication):
2251>     """
2252>     This is a a simple subclass of MockApplication which sends some
2253>     data when it is connected.
2254>     """
2255>
2256>
2257>     def connectionMade(self):
2258>         """
2259>         Write an introduction when we are connected.
2260>         """
2261>         MockApplication.connectionMade(self)
2262>         self.channel.write('intro')
2263>
2264>
2265>
2266> class MockProcessProtocol(protocol.ProcessProtocol):
2267>     """
2268>     A mock ProcessProtocol which echoes back data sent to it and
2269>     appends a tilde.
2270>
2271}}}
2272
2273I'm beginning to suspect a tilde fetish. Those sleek curves, that wavy form,
2274the way it reminds me of home...
2275
2276Say why you are adding a tilde. What's the intent?
2277
2278{{{
2279>     @ivar data: a C{str} of data received.
2280>     @ivar err: a C{str} of error data received.
2281>     @ivar inConnectionOpen: True if the input side is open.
2282>     @ivar outConnectionOpen: True if the output side is open.
2283>     @ivar errConnectionOpen: True if the error side is open.
2284>     @ivar ended: False if the protocol has not ended, a C{Failure} if the
2285>         process has ended.
2286>     """
2287>     packetData = ''
2288>
2289>
2290>     def connectionMade(self):
2291>         """
2292>         Set up variables.
2293>         """
2294>         self.data = ''
2295>         self.err = ''
2296>         self.inConnectionOpen = True
2297>         self.outConnectionOpen = True
2298>         self.errConnectionOpen = True
2299>         self.ended = False
2300>         if self.packetData:
2301>             self.outReceived(self.packetData)
2302>
2303>
2304>     def outReceived(self, data):
2305>         """
2306>         Data was received.  Store it and echo it back with a tilde.
2307>         """
2308>         self.data += data
2309>         self.transport.write(data + '~')
2310>
2311>
2312>     def errReceived(self, data):
2313>         """
2314>         Error data was received.  Store it and echo it back backwards.
2315>         """
2316>         self.err += data
2317>         self.transport.write(data[::-1])
2318>
2319>
2320>     def inConnectionLost(self):
2321>         """
2322>         Close the input side.
2323>         """
2324>         self.inConnectionOpen = False
2325>
2326>
2327>     def outConnectionLost(self):
2328>         """
2329>         close the output side.
2330>         """
2331}}}
2332
2333'Close', not 'close'.
2334
2335{{{
2336>         self.outConnectionOpen = False
2337>
2338>
2339>     def errConnectionLost(self):
2340>         """
2341>         Close the error side.
2342>         """
2343>         self.errConnectionOpen = False
2344>
2345>
2346>     def processEnded(self, reason):
2347>         """
2348>         End the process and store the reason.
2349>         """
2350>         self.ended = reason
2351>
2352>
2353>
2354> class EchoTransport:
2355>     """
2356>     A transport for a ProcessProtocol which echos data that is sent to it with
2357>     a Window newline (\\r\\n) appended to it.  If a null byte is in the data,
2358}}}
2359
2360The normal way to say this is 'CR LF'.
2361
2362{{{
2363>     disconnect.  When we are asked to disconnect, disconnect the C{ProcessProtocol}
2364>     with a 0 exit code.
2365>
2366>     @ivar proto: the C{ProcessProtocol} connected to us.
2367>     @ivar data: a C{str} of data written to us.
2368>     """
2369>
2370>
2371>     def __init__(self, p):
2372>         """
2373>         Initialize our instance variables.
2374>
2375>         @param p: a C{ProcessProtocol} to connect to ourself.
2376>         """
2377>         self.proto = p
2378>         p.makeConnection(self)
2379>         self.closed = False
2380>         self.data = ''
2381>
2382}}}
2383
2384If you feel like it, rename 'p' to 'processProtocol'.
2385
2386{{{
2387>
2388>     def write(self, data):
2389>         """
2390>         We got some data.  Give it back to our C{ProcessProtocol} with
2391>         a newline attached.  Disconnect if there's a null byte.
2392>         """
2393>         self.data += data
2394>         self.proto.outReceived(data)
2395>         self.proto.outReceived('\r\n')
2396>         if '\x00' in data: # mimic 'exit' for the shell test
2397>             self.loseConnection()
2398>
2399>
2400>     def loseConnection(self):
2401>         """
2402>         If we're asked to disconnect (and we haven't already) shut down
2403>         the C{ProcessProtocol} with a 0 exit code.
2404>         """
2405>         if self.closed: return
2406}}}
2407
2408Put a newline after the colon.
2409
2410{{{
2411>         self.closed = 1
2412>         self.proto.inConnectionLost()
2413>         self.proto.outConnectionLost()
2414>         self.proto.errConnectionLost()
2415>         self.proto.processEnded(failure.Failure(
2416>                 error.ProcessTerminated(0, None, None)))
2417>
2418>
2419>
2420> class MockProtocol(protocol.Protocol):
2421>     """
2422>     A sample Process which stores the data passed to it.
2423>
2424}}}
2425
2426This isn't a 'Process'. Perhaps you mean 'protocol'?
2427
2428{{{
2429>     @ivar data: a C{str} of the data passed to us.
2430>     @ivar open: True if the channel is open.
2431>     @ivar reason: if not None, the reason the protocol was closed.
2432>     """
2433>     packetData = ''
2434>
2435}}}
2436
2437packetData lacks documentation. What is it? Why is it there?
2438
2439{{{
2440>
2441>     def connectionMade(self):
2442>         """
2443>         Set up the instance variables.
2444>         """
2445}}}
2446
2447Not the most helpful docstring in the world. Nothing obviously better springs
2448to mind. I see you use this in other places too.
2449
2450{{{
2451>         self.data = ''
2452>         self.open = True
2453>         self.reason = None
2454>         if self.packetData:
2455>             self.dataReceived(self.packetData)
2456>
2457>
2458>     def dataReceived(self, data):
2459>         """
2460>         Store the received data.
2461>         """
2462>         self.data += data
2463>         self.transport.write(data + '~')
2464>
2465}}}
2466
2467Hey look! It's that tilde again!
2468
2469{{{
2470>
2471>     def connectionLost(self, reason):
2472>         """
2473>         Close the protocol and store the reason.
2474>         """
2475>         self.open = False
2476>         self.reason = reason
2477>
2478>
2479}}}
2480
2481At this point I'm really starting to look forward to reading the tests.
2482
2483{{{
2484>
2485> class StubConnection:
2486>     """
2487>     A stub for twisted.conch.ssh.connection.SSHConnection.  Record the data
2488>     that channels send, and when they try to close the connection.
2489>
2490>     @ivar data: a C{dict} mapping C{SSHChannel}s to a C{list} of C{str} of data
2491>        they sent.
2492>     @ivar extData: a C{dict} mapping C{SSHChannel}s to a C{list} of C{tuple} of
2493>        (C{int}, C{str}) of extended data they sent.
2494>     @ivar requests: a C{dict} mapping C{SSHChannel}s to a C{list} of C{tuple} of
2495>        (C{str}, C{str}) of channel requests they made.
2496>     @ivar eofs: a C{dict} mapping C{SSHChannel}s to C{true} if they have sent an
2497>        EOF.
2498>     @ivar closes: a C{dict} mapping C{SSHChannel}s to C{true} if they have sent a
2499>        close.
2500>     """
2501>
2502>
2503>     def __init__(self):
2504>         """
2505>         Initialize our instance variables.
2506>         """
2507>         self.data = {}
2508>         self.extData = {}
2509>         self.requests = {}
2510>         self.eofs = {}
2511>         self.closes = {}
2512>
2513>
2514>     def logPrefix(self):
2515>         """
2516>         Return our logging prefix.
2517>         """
2518>         return "MockConnection"
2519>
2520>
2521>     def sendData(self, channel, data):
2522>         """
2523>         Record the sent data.
2524>         """
2525>         self.data.setdefault(channel, []).append(data)
2526>
2527>
2528>     def sendExtendedData(self, channel, type, data):
2529>         """
2530>         Record the sent extended data.
2531>         """
2532>         self.extData.setdefault(channel, []).append((type, data))
2533>
2534>
2535>     def sendRequest(self, channel, request, data, wantReply=False):
2536>         """
2537>         Record the sent channel request.
2538>         """
2539>         self.requests.setdefault(channel, []).append((request, data,
2540>             wantReply))
2541>         if wantReply:
2542>             return defer.succeed(None)
2543>
2544>
2545>     def sendEOF(self, channel):
2546>         """
2547>         Record the sent EOF.
2548>         """
2549>         self.eofs[channel] = True
2550>
2551>
2552>     def sendClose(self, channel):
2553>         """
2554>         Record the sent close.
2555>         """
2556>         self.closes[channel] = True
2557>
2558>
2559>
2560> class StubTransport:
2561>     """
2562>     A stub transport which records the data written.
2563>
2564>     @ivar buf: the data sent to the transport.
2565>     @type buf: C{str}
2566>
2567>     @ivar err: the extended data sent to the transport.
2568>     @type err: C{str}
2569>
2570>     @ivar close: flags indicating if the transport has been closed.
2571>     @type close: C{bool}
2572>     """
2573>
2574>     buf = ''
2575>     err = ''
2576>     close = False
2577>
2578>
2579>     def write(self, data):
2580>         """
2581>         Record data in the buffer.
2582>         """
2583>         self.buf += data
2584>
2585>
2586>     def writeErr(self, data):
2587>         """
2588>         Record the extended data in the buffer.
2589>         """
2590>         self.err += data
2591>
2592}}}
2593
2594It records extended data and it's called 'writeErr'? That seems weird. Can you
2595please explain the discrepancy in the docstring.
2596
2597{{{
2598>
2599>     def loseConnection(self):
2600>         """
2601>         Note that the connection was closed.
2602>         """
2603>         self.close = True
2604>
2605>
2606>
2607> class StubClient(object):
2608>     """
2609>     A stub class representing the client to a SSHSession.
2610>
2611>     @ivar transport: A C{StubTransport} object which keeps track of the data
2612>         passed to it.
2613>     """
2614>
2615>
2616>     def __init__(self):
2617>         self.transport = StubTransport()
2618>
2619>
2620>
2621> class OldSessionInterfaceTestCase(unittest.TestCase):
2622>     """
2623>     Tests for the old SSHSession class interface.  This interface is not ideal,
2624>     but it is tested in order to maintain backwards compatibility.
2625>     """
2626>
2627}}}
2628
2629Thanks for the docstring, it helps.
2630
2631{{{
2632>
2633>     def setUp(self):
2634>         """
2635>         Make an SSHSession object to test.  Give the chanel some window
2636>         so that it's allowed to send packets.
2637>         """
2638}}}
2639
2640"channel", not "chanel".
2641
2642If the numbers are arbitrary, say so. If they aren't, say why they were
2643chosen.
2644
2645{{{
2646>         self.session = session.SSHSession(remoteWindow=500,
2647>                 remoteMaxPacket=100, conn=StubConnection(),
2648>                 avatar=StubOldAvatar())
2649>
2650>
2651>     def test_init(self):
2652>         """
2653>         Test that SSHSession initializes its buffer (buf), client, and
2654>         ISession adapter.  The avatar should not need to be adaptable to an
2655>         ISession immediately.
2656>         """
2657>         s = session.SSHSession(avatar=object) # use object because it doesn't
2658>                                               # have an adapter
2659}}}
2660
2661I don't like this style of comment, and it doesn't appear much in the rest of
2662Twisted. It's no big deal though.
2663
2664A comment here that applies to all of your other tests:
2665
2666Don't say "Test that" in the docstring of a test. We know it's a test. Saying
2667"test that" or "check that" or "foo should bar" encourages one to write
2668docstrings that speak in terms of implemention.
2669
2670Instead, describe the behaviour. "SSHSession initializes its buffer, ...".
2671
2672{{{
2673>         self.assertEquals(s.buf, '')
2674>         self.assertIdentical(s.client, None)
2675>         self.assertIdentical(s.session, None)
2676>
2677>
2678>     def _testGetsSession(self):
2679>         """
2680>         Check that a function will cause SSHSession to generate a session.
2681>
2682>         We return a C{tuple} of a C{SSHSession} and aC{Deferred} which expects
2683}}}
2684
2685Add a space after 'a'.
2686
2687{{{
2688>         to be called back with a function and arguments to be called.  After
2689>         the function is called, the C{SSHSession} should have a session
2690>         attribute which provides I{ISession}.
2691>         """
2692}}}
2693
2694This would be a good spot to say *why* you need this helper.
2695
2696{{{
2697>         s = session.SSHSession()
2698>         d = defer.Deferred()
2699>         def check(fargs):
2700}}}
2701
2702Say "functionAndArgs", please. For me.
2703
2704Actually, maybe it's better to assume it's a nullary function and then use
2705lambdas when you need to invoke it.
2706
2707{{{
2708>             f = fargs[0]
2709>             args = fargs[1:]
2710>             s.avatar = StubOldAvatar()
2711>             self.assertIdentical(s.session, None)
2712>             f(*args)
2713}}}
2714
2715At this point I'm curious, what sort of function do we expect here?
2716
2717{{{
2718>             self.assertTrue(session.ISession.providedBy(s.session))
2719>         d.addCallback(check)
2720>         return s, d
2721>
2722}}}
2723
2724This is one weird function.
2725
2726{{{
2727>
2728>     def test_requestShellGetsSession(self):
2729>         """
2730>         If an ISession adapter isn't already present, request_shell should get
2731>         one.
2732>         """
2733>         s, d = self._testGetsSession()
2734>         return d.callback((s.requestReceived, 'shell', ''))
2735>
2736>
2737>     def test_requestExecGetsSession(self):
2738>         """
2739>         If an ISession adapter isn't already present, request_exec should get
2740>         one.
2741>         """
2742>         s, d = self._testGetsSession()
2743>         return d.callback((s.requestReceived, 'exec', common.NS('true')))
2744>
2745>
2746>     def test_requestPtyReqGetsSession(self):
2747>         """
2748>         If an ISession adapter isn't already present, request_pty_req should
2749>         get one.
2750>         """
2751>         s, d = self._testGetsSession()
2752>         return d.callback((s.requestReceived, 'pty_req',
2753>             session.packRequest_pty_req(
2754>             'term', (0, 0, 0, 0), [])))
2755>
2756>
2757>     def test_requestWindowChangeGetsSession(self):
2758>         """
2759>         If an ISession adapter isn't already present, request_window_change
2760>         should get one.
2761>         """
2762>         s, d = self._testGetsSession()
2763>         return d.callback((s.requestReceived, 'window_change',
2764>             session.packRequest_window_change((1, 1, 1, 1))))
2765>
2766}}}
2767
2768This is a long, long way from idiomatic Twisted code.
2769
2770http://www.osnews.com/images/comics/wtfm.jpg
2771
2772Looking back at _testGetsSession, everything up to and including the
2773assertIdentical line could easily go into setUp. Then, the tests could simply
2774call the methods they are testing and then do the assertion manually.
2775
2776From all I can see, the Deferred is a red herring.
2777
2778{{{
2779>
2780>     def test_client(self):
2781>         """
2782>         Test that SSHSession passes data and extended data along to a client.
2783>         """
2784>         self.session.dataReceived('1')
2785>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '3')
2786>         # these don't get sent as part of the client interface
2787>         self.session.client = StubClient()
2788>         self.session.dataReceived('2')
2789>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '4')
2790>         self.assertEquals(self.session.client.transport.buf, '2')
2791>         self.assertEquals(self.session.client.transport.err, '4')
2792>         self.session.eofReceived()
2793>         self.assertTrue(self.session.conn.closes[self.session])
2794>         self.session.closed()
2795>         self.assertTrue(self.session.client.transport.close)
2796>         self.session.client.transport.close = False
2797>
2798}}}
2799
2800Please split this into three tests, one for each assert block.
2801
2802{{{
2803>
2804>     def test_lookupSubsystem(self):
2805>         """
2806>         When a client requests a subsystem, the SSHSession object should get
2807>         the subsystem by calling avatar.lookupSubsystem, and attach it as
2808>         the client.
2809>         """
2810>         self.session.session = None # old SSHSession didn't have this attribute
2811>         self.session.dataReceived('1')
2812>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2')
2813>         self.assertFalse(self.session.requestReceived(
2814>                 'subsystem', common.NS('BadSubsystem')))
2815>         self.assertIdentical(self.session.client, None)
2816}}}
2817
2818Please add a comment explaining why you have this assertion.
2819
2820{{{
2821>         self.assertTrue(self.session.requestReceived(
2822>                 'subsystem', common.NS('TestSubsystem') + 'data'))
2823>         self.assertIsInstance(self.session.client, protocol.ProcessProtocol)
2824>         self.assertIdentical(self.session.sessionApplication.processProtocol,
2825>                              self.session.avatar.subsystem)
2826>
2827>         self.assertEquals(self.session.conn.data[self.session],
2828>                 ['\x00\x00\x00\x0dTestSubsystemdata~'])
2829>         self.session.dataReceived('more data')
2830>         self.assertEquals(self.session.conn.data[self.session][-1],
2831>                 'more data~')
2832>         self.session.closeReceived()
2833>         self.assertTrue(self.session.conn.closes[self.session])
2834>
2835}}}
2836
2837This test is too big. Please split it up.
2838
2839{{{
2840>
2841>     def test_lookupSubsystemProtocol(self):
2842>         """
2843>         Test that lookupSubsystem correctly handles being returned a
2844>         Protocol.
2845>         """
2846}}}
2847
2848Please define "correctly" in the docstring.
2849
2850{{{
2851>         self.session.session = None # old SSHSession didn't have this attribute
2852>         self.session.dataReceived('1')
2853>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2')
2854>         self.assertIdentical(self.session.client, None)
2855>         self.assertTrue(self.session.requestReceived(
2856>                 'subsystem', common.NS('protocol') + 'data'))
2857>         self.assertIsInstance(self.session.client,
2858>                               protocol.ProcessProtocol)
2859>         self.assertIdentical(
2860>             self.session.sessionApplication.processProtocol.proto,
2861>                              self.session.avatar.subsystem)
2862>         self.assertEquals(self.session.sessionApplication.processProtocol.connectionLost,
2863>                              self.session.sessionApplication.processProtocol.proto.connectionLost)
2864>         self.session.sessionApplication.processProtocol.foo = 'foo'
2865>         self.assertEquals(self.session.sessionApplication.processProtocol.proto.foo,
2866>                           'foo')
2867>
2868>         self.assertIdentical(
2869>             self.session.sessionApplication.processProtocol.transport.proto,
2870>             self.session.sessionApplication.processProtocol)
2871>
2872>         self.assertEquals(self.session.conn.data[self.session],
2873>                 ['\x00\x00\x00\x08protocoldata~'])
2874>         self.session.dataReceived('more data')
2875>         self.assertEquals(self.session.conn.data[self.session][-1],
2876>                 'more data~')
2877>         self.session.closeReceived()
2878>         self.assertTrue(self.session.conn.closes[self.session])
2879>         self.session.closed()
2880>         self.assertEquals(self.session.sessionApplication.processProtocol.proto.reason,
2881>                           protocol.connectionDone)
2882>
2883}}}
2884
2885Please split this test up.
2886
2887{{{
2888>
2889>     def test_lookupSubsystemDoesNotNeedISession(self):
2890>         """
2891>         Previously, if one only wanted to implement a subsystem, an
2892>         ISession adapter wasn't needed.
2893>         """
2894>         s = session.SSHSession(avatar=SubsystemOnlyAvatar(),
2895>                                conn=StubConnection())
2896>         self.assertTrue(s.request_subsystem(common.NS('subsystem') + 'data'))
2897>         self.assertNotIdentical(s.applicationFactory, None)
2898>         self.assertIdentical(s.conn.closes.get(s), None)
2899>         s.eofReceived()
2900>         self.assertTrue(s.conn.closes.get(s))
2901>         # these should not raise errors
2902>         s.closeReceived()
2903>         s.closed()
2904>
2905}}}
2906
2907Testing negatives is always tricky. I think you could explain better exactly
2908how you *aren't* using ISession here, and highlight the bits that you think
2909should blow up.
2910
2911681 lines to go. Time for breakfast.
2912
2913{{{
2914>
2915>     def test_requestShell(self):
2916>         """
2917>         When a client requests a shell, the SSHSession object should get
2918>         the shell by getting an ISession adapter for the avatar, then
2919>         calling openShell() with a ProcessProtocol to attach.
2920>         """
2921>         # gets a shell the first time
2922>         self.assertTrue(self.session.requestReceived('shell', ''))
2923>         self.assertIsInstance(self.session.session,
2924>                               StubSessionForStubOldAvatar)
2925>         self.assertIsInstance(self.session.client,
2926>                               session.SSHSessionProcessProtocol)
2927>         self.assertIdentical(self.session.session.shellProtocol,
2928>                 self.session.client)
2929>         # doesn't get a shell the second time
2930>         self.assertFalse(self.session.requestReceived('shell', ''))
2931>         errors = self.flushLoggedErrors()
2932}}}
2933
2934You should specify the type of error that you are flushing here, so that you
2935don't accidentally flush a real error.
2936
2937{{{
2938>         self.assertEquals(len(errors), 1)
2939>         errors[0].trap(RuntimeError)
2940>
2941}}}
2942
2943Part of the difficulty in reviewing this is that there are so many assertions
2944in these tests, which means they are testing many things.
2945
2946If you can think of one, please extract custom assert* methods that can
2947express your intent at a higher level.
2948
2949{{{
2950>
2951>     def test_requestShellWithData(self):
2952>         """
2953>         When a client executes a shell, it should be able to give pass data back
2954>         and forth.
2955>         """
2956}}}
2957
2958"back and forth" is usually followed by "between X and Y". Please say where
2959the data is going to and coming from.
2960
2961{{{
2962>         self.assertTrue(self.session.requestReceived('shell', ''))
2963>         self.assertIsInstance(self.session.session,
2964>                               StubSessionForStubOldAvatar)
2965>         self.session.dataReceived('some data\x00')
2966>         self.assertEquals(self.session.session.shellTransport.data,
2967>                           'some data\x00')
2968>         self.assertEquals(self.session.conn.data[self.session],
2969>                           ['some data\x00', '\r\n'])
2970>         self.assertTrue(self.session.session.shellTransport.closed)
2971>         self.assertEquals(self.session.conn.requests[self.session],
2972>                           [('exit-status', '\x00\x00\x00\x00', False)])
2973>
2974>
2975>     def test_requestExec(self):
2976>         """
2977>         When a client requests a command, the SSHSession object should get
2978>         the command by getting an ISession adapter for the avatar, then
2979>         calling execCommand with a ProcessProtocol to attach and the
2980>         command line.
2981>         """
2982>         self.assertFalse(self.session.requestReceived('exec', common.NS('false')))
2983>         errors = self.flushLoggedErrors()
2984}}}
2985
2986errors = self.flushLoggedErrors(RuntimeError)
2987
2988{{{
2989>         self.assertEquals(len(errors), 1)
2990>         errors[0].trap(RuntimeError)
2991>         self.assertIdentical(self.session.client, None)
2992>
2993>         self.assertTrue(self.session.requestReceived('exec', common.NS('true')))
2994>         self.assertIsInstance(self.session.session,
2995>                               StubSessionForStubOldAvatar)
2996>         self.assertIsInstance(self.session.client,
2997>                               session.SSHSessionProcessProtocol)
2998>         self.assertIdentical(self.session.session.execProtocol,
2999>                 self.session.client)
3000>         self.assertEquals(self.session.session.execCommandLine,
3001>                 'true')
3002>
3003}}}
3004
3005Uhhh, that doesn't *really* execute 'true' on the command line, does it?
3006
3007If it does, please replace it with something that will work on all platforms
3008(e.g. '%s -c pass' % sys.executable). If it doesn't please replace it with
3009something obviously bogus.
3010
3011{{{
3012>
3013>     def test_requestExecWithData(self):
3014>         """
3015>         When a client executes a command, it should be able to give pass data back
3016>         and forth.  Setting the remoteWindowLeft to 4 tests that the C{SSHChannel}
3017>         buf instance variable isn't overriden by SSHSession.
3018>         """
3019}}}
3020
3021Why is 4 special? What is it about setting remoteWindowLeft that affects
3022SSHChannel.buf?
3023
3024{{{
3025>         self.session.remoteWindowLeft = 4
3026>         self.assertTrue(self.session.requestReceived('exec', common.NS('echo hello')))
3027>         self.assertIsInstance(self.session.session,
3028>                               StubSessionForStubOldAvatar)
3029>         self.session.dataReceived('some data')
3030>         self.assertEquals(self.session.session.execTransport.data, 'some data')
3031>         self.session.addWindowBytes(20)
3032}}}
3033
3034Why 20?
3035
3036You know, it's entirely OK to do something like:
3037    arbitraryInteger = 20
3038    self.session.addWindowBytes(arbitraryInteger)
3039
3040{{{
3041>         self.assertEquals(self.session.conn.data[self.session],
3042>                           ['hell', 'osome data\r\n'])
3043>         self.session.eofReceived()
3044>         self.session.closeReceived()
3045>         self.session.closed()
3046>         self.assertTrue(self.session.session.execTransport.closed)
3047>         self.assertEquals(self.session.conn.requests[self.session],
3048>                           [('exit-status', '\x00\x00\x00\x00', False)])
3049>
3050>
3051>     def test_requestPty(self):
3052>         """
3053>         When a client requests a PTY, the SSHSession object should make
3054>         the request by getting an ISession adapter for the avatar, then
3055>         calling getPty with the terminal type, the window size, and any modes
3056>         the client gave us.
3057>         """
3058>         self.assertFalse(
3059>                 self.session.requestReceived('pty_req',
3060>             session.packRequest_pty_req('bad', (1, 2, 3, 4), [])))
3061>         self.assertIsInstance(self.session.session,
3062>                               StubSessionForStubOldAvatar)
3063>         errors = self.flushLoggedErrors()
3064}}}
3065
3066errors = self.flushLoggedErrors(RuntimeError)
3067
3068{{{
3069>         self.assertEquals(len(errors), 1)
3070>         errors[0].trap(RuntimeError)
3071}}}
3072
3073This is begging for a custom assertion. It's perfectly OK to add one here, and
3074then later we can move it into trial.
3075
3076{{{
3077>         self.assertTrue(self.session.requestReceived('pty_req',
3078>             session.packRequest_pty_req('good', (1, 2, 3, 4), [])))
3079>         self.assertEquals(self.session.session.ptyRequest,
3080>                 ('good', (1, 2, 3, 4), []))
3081>
3082}}}
3083
3084I can't remember, do 'good' and 'bad' activate specific behaviours on the stub
3085objects? If they do, add a comment saying so.
3086
3087{{{
3088>
3089>     def test_requestWindowChange(self):
3090>         """
3091>         When the client requests to change the window size, the SSHSession
3092>         object should make the request by getting an ISession adapter for the
3093>         avatar, then calling windowChanged with the new window size.
3094>         """
3095>         self.assertFalse(self.session.requestReceived('window_change',
3096>             session.packRequest_window_change((0, 0, 0, 0))))
3097>         errors = self.flushLoggedErrors()
3098>         self.assertEquals(len(errors), 1)
3099>         errors[0].trap(RuntimeError)
3100>         self.assertIsInstance(self.session.session,
3101>                               StubSessionForStubOldAvatar)
3102}}}
3103
3104You do this a lot. I'm guessing it's because requestReceived should set
3105session. Why not test that in one place and then remove the assertion from all
3106your other tests?
3107
3108{{{
3109>         self.assertTrue(self.session.requestReceived('window_change',
3110>             session.packRequest_window_change((1, 2, 3, 4))))
3111>         self.assertEquals(self.session.session.windowChange,
3112>                 (1, 2, 3, 4))
3113>
3114>
3115>     def test_eofReceived(self):
3116>         """
3117>         When an EOF is received and a ISession adapter is present, it should
3118>         be notified of the EOF message.
3119>         """
3120>         self.session.session = session.ISession(self.session.avatar)
3121>         self.session.eofReceived()
3122>         self.assertTrue(self.session.session.gotEOF)
3123>
3124>
3125>     def test_closeReceived(self):
3126>         """
3127>         When a close is received, the session should send a close message.
3128>         """
3129>         self.assertIdentical(self.session.closeReceived(), None)
3130>         self.assertTrue(self.session.conn.closes[self.session])
3131>
3132>
3133>     def test_closed(self):
3134>         """
3135>         When a close is received and a ISession adapter is present, it should
3136>         be notified of the close message.
3137>         """
3138>         self.session.session = session.ISession(self.session.avatar)
3139>         self.session.closed()
3140>         self.assertTrue(self.session.session.gotClosed)
3141>
3142>
3143>
3144> class TestHelpers(unittest.TestCase):
3145>     """
3146>     Tests for the 4 helper functions: parseRequest_* and packRequest_*.
3147>     """
3148>
3149>
3150>     def test_parseRequest_pty_req(self):
3151>         """
3152>         The payload of a pty-req message is::
3153>             string  terminal
3154>             uint32  columns
3155>             uint32  rows
3156>             uint32  x pixels
3157>             uint32  y pixels
3158>             string  modes
3159>
3160>         Modes are::
3161>             byte    mode number
3162>             uint32  mode value
3163>         """
3164>         self.assertEquals(session.parseRequest_pty_req(common.NS('xterm') +
3165>                 '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00'
3166>                 '\x04\x00\x00\x00\x05\x05\x00\x00\x00\x06'),
3167>                 ('xterm', (2, 1, 3, 4), [(5, 6)]))
3168>
3169}}}
3170
3171If you are happy to use NS to convert 'xterm', you should be OK with using a
3172similar helper to convert the Python integers into their wire format. Please
3173do this.
3174
3175Alternatively, format the byte string differently:
3176
3177{{{
3178#!python
3179request = (common.NS('xterm') +
3180           '\x00\x00\x00\x01' +
3181           '\x00\x00\x00\x02' +
3182           '\x00\x00\x00\x03' +
3183           '\x00\x00\x00\x04' +
3184           '\x00\x00\x00\x05' +
3185           '\x00\x00\x00\x06')
3186self.assertEquals(session.parseRequest_pty_req(request),
3187                  ('xterm', (2, 1, 3, 4), [(5, 6)]))
3188}}}
3189
3190{{{
3191>
3192>     def test_packRequest_pty_req(self):
3193>         """
3194>         See test_parseRequest_pty_req for the payload format.
3195>         """
3196>         self.assertEquals(self.assertWarns(DeprecationWarning,
3197>             "packRequest_pty_req should be packing the modes.",
3198>             unittest.__file__, session.packRequest_pty_req, 'xterm',
3199>             (2, 1, 3, 4), '\x05\x00\x00\x00\x06'),
3200>             common.NS('xterm') + '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00'
3201>             '\x00\x03\x00\x00\x00\x04\x00\x00\x00\x05\x05\x00\x00\x00\x06')
3202>
3203}}}
3204
3205There's no reason to nest these calls so much. Local variables are as cheap as
3206chips.
3207
3208Use `TestCase.callDeprecated` instead of `assertWarns`.
3209
3210{{{
3211>
3212>     def test_parseRequest_window_change(self):
3213>         """
3214>         The payload of a window_change request is::
3215>             uint32  columns
3216>             uint32  rows
3217>             uint32  x pixels
3218>             uint32  y pixels
3219>         """
3220>         self.assertEquals(session.parseRequest_window_change('\x00\x00\x00\x01'
3221>             '\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x04'), (2, 1, 3, 4))
3222>
3223}}}
3224
3225Same comments as for the first test.
3226
3227{{{
3228>
3229>     def test_packRequest_window_change(self):
3230>         """
3231>         See test_parseRequest_window_change for the payload format.
3232>         """
3233>         self.assertEquals(session.packRequest_window_change((2, 1, 3, 4)),
3234>             '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x04')
3235>
3236}}}
3237
3238Same comments as for the first test.
3239
3240{{{
3241>
3242>
3243> class SSHSessionProcessProtocolTestCase(unittest.TestCase):
3244>     """
3245>     SSHSessionProcessProtocol is an obsolete class used as a ProcessProtocol
3246>     for executed programs.  It has has a couple transport methods, namely
3247>     write() and loseConnection()
3248>     """
3249>
3250}}}
3251
3252'couple of transport methods'.
3253
3254{{{
3255>
3256>     def setUp(self):
3257>         self.session = session.SSHSession(conn=StubConnection(),
3258>                 remoteWindow=500, remoteMaxPacket=100)
3259>         self.transport = StubTransport()
3260>         self.pp = session.SSHSessionProcessProtocol(self.session)
3261>         self.pp.makeConnection(self.transport)
3262>
3263>
3264>     def test_init(self):
3265>         """
3266>         SSHSessionProcessProtocol should set self.session to the session passed
3267>         to the __init__ method.
3268>         """
3269>         self.assertEquals(self.pp.session, self.session)
3270>
3271>
3272>     def test_getSignalName(self):
3273>         """
3274>         _getSignalName should return the name of a signal when given the
3275>         signal number.  Also, it should only generate the signal dictionary
3276>         once.
3277>         """
3278>         signal.SIGTwistedTest = signal.NSIG + 1 # value can't exist normally
3279>         # Force reinitialization of signals
3280>         self.pp.__class__._signalNames = None
3281>         try:
3282>             for signame in session.supportedSignals:
3283>                 signame = 'SIG' + signame
3284>                 value = getattr(signal, signame)
3285>                 self.assertEquals(self.pp._getSignalName(value),
3286>                         signame, "%i: %s != %s" % (value,
3287>                             self.pp._getSignalName(value), signame))
3288>             self.assertEquals(self.pp._getSignalName(signal.SIGTwistedTest),
3289>                 'SIGTwistedTest@' + sys.platform)
3290>             newPP = session.SSHSessionProcessProtocol(self.session)
3291>             self.assertNotIdentical(newPP._signalNames, None)
3292>             self.assertIdentical(newPP._signalNames, self.pp._signalNames)
3293}}}
3294
3295This tests three things, and should be three tests.
3296
3297{{{
3298>         finally:
3299>             del signal.SIGTwistedTest
3300>             # Force reinitialization of signals
3301>             self.pp.__class__._signalNames = None
3302>
3303}}}
3304
3305See, that just goes to show that storing it on the class is a bad idea.
3306
3307{{{
3308>     if getattr(signal, 'SIGALRM', None) is None:
3309>         test_getSignalName.skip = "Not all signals available"
3310>
3311>
3312>     def test_outReceived(self):
3313>         """
3314>         When data is passed to the outReceived method, it should be sent to
3315>         the session's write method.
3316>         """
3317>         self.pp.outReceived('test data')
3318>         self.assertEquals(self.session.conn.data[self.session],
3319>                 ['test data'])
3320>
3321>
3322>     def test_write(self):
3323>         """
3324>         When data is passed to the write method, it should be sent to the
3325>         session channel's write method.
3326>         """
3327>         self.pp.write('test data')
3328>         self.assertEquals(self.session.conn.data[self.session],
3329>                 ['test data'])
3330>
3331>
3332>     def test_errReceived(self):
3333>         """
3334>         When data is passed to the errReceived method, it should be sent to
3335>         the session's writeExtended method.
3336>         """
3337>         self.pp.errReceived('test data')
3338>         self.assertEquals(self.session.conn.extData[self.session],
3339>                 [(1, 'test data')])
3340>
3341>
3342>     def test_inConnectionLost(self):
3343>         """
3344>         When inConnectionLost is called, it should send an EOF message,
3345>         """
3346>         self.pp.inConnectionLost()
3347>         self.assertTrue(self.session.conn.eofs[self.session])
3348>
3349>
3350>     def test_outConnectionLost(self):
3351>         """
3352>         When outConnectionLost is called and there is no ISession adapter,
3353>         the connection should be closed,
3354>         """
3355>         self.pp.outConnectionLost()
3356>         self.assertTrue(self.session.conn.closes[self.session])
3357>
3358>
3359>     def test_loseConnection(self):
3360>         """
3361>         When loseConnection() is called, it should call loseConnection
3362>         on the session channel.
3363>         """
3364>         self.pp.loseConnection()
3365>         self.assertTrue(self.session.conn.closes[self.session])
3366>
3367>
3368>     def test_processEndedWithExitCode(self):
3369>         """
3370>         When processEnded is called, if there is an exit code in the reason
3371>         it should be sent in an exit-status method.  The connection should be
3372>         closed.
3373>         """
3374>         self.pp.processEnded(failure.Failure(error.ProcessDone(None)))
3375>         self.assertEquals(self.session.conn.requests[self.session],
3376>                 [('exit-status', '\x00' * 4, False)])
3377}}}
3378
3379Explain where the magic number comes from.
3380
3381{{{
3382>         self.assertTrue(self.session.conn.closes[self.session])
3383>
3384>
3385>     def test_processEndedWithExitSignalCoreDump(self):
3386>         """
3387>         When processEnded is called, if there is an exit signal in the reason
3388>         it should be sent in an exit-signal message.  The connection should be
3389>         closed.
3390>         """
3391>         self.pp.processEnded(failure.Failure(error.ProcessTerminated(None,
3392>             signal.SIGTERM, 128)))
3393>         self.assertEqual(self.session.conn.requests[self.session],
3394>                 [('exit-signal', common.NS('TERM') + '\x01' + common.NS('') +
3395>                     common.NS(''), False)])
3396}}}
3397
3398Two magic numbers here. Please explain where they come from.
3399
3400(Hint: coreDumped = '\x01' would go a long way)
3401
3402{{{
3403>         self.assertTrue(self.session.conn.closes[self.session])
3404>
3405>
3406>     def test_processEndedWithExitSignalNoCoreDump(self):
3407>         """
3408>         When processEnded is called, if there is an exit signal in the
3409>         reason it should be sent in an exit-signal message.  If no
3410>         core was dumped, don't set the core-dump bit.
3411>         """
3412>         self.pp.processEnded(
3413>             failure.Failure(error.ProcessTerminated(None, signal.SIGTERM, 0)))
3414>         self.assertEqual(self.session.conn.requests[self.session],
3415>                          [('exit-signal', common.NS('TERM') + '\x00' +
3416>                            common.NS('') + common.NS(''), False)])
3417>         self.assertTrue(self.session.conn.closes[self.session])
3418>
3419}}}
3420
3421Magic number again.
3422
3423{{{
3424>     if not hasattr(os, 'WCOREDUMP'):
3425>         skipMsg = "can't run this w/o os.WCOREDUMP"
3426>         test_processEndedWithExitSignalCoreDump.skip = skipMsg
3427>         test_processEndedWithExitSignalNoCoreDump.skip = skipMsg
3428>
3429>
3430>
3431> class SSHSessionProcessProtocolApplicationTestCase(unittest.TestCase):
3432>     """
3433>     SSHSessionProcessProtocolApplicationTestCase is an class used to
3434>     connect to a SSHSessionProcessProtocol as an application.
3435>     """
3436>
3437>
3438>     def test_dataReceived(self):
3439>         """
3440>         When data is received, it should be sent to the transport.
3441>         """
3442>         client = StubClient()
3443>         app = session.SSHSessionProcessProtocolApplication(client)
3444>         app.dataReceived('test data')
3445>         self.assertEquals(client.transport.buf, 'test data')
3446>
3447}}}
3448
3449This is a beautiful test. More like this please.
3450
3451{{{
3452>
3453>     def test_extendedDataReceived(self):
3454>         """
3455>         When extended data of the type EXTENDED_DATA_STDERR is
3456>         received, it should be passed along to SSHSessionProcessProtocol's
3457>         transport.writeErr.
3458>         """
3459>         transport = StubTransport()
3460>         pp = MockProcessProtocol()
3461>         pp.makeConnection(transport)
3462>         app = session.SSHSessionProcessProtocolApplication(pp)
3463>         app.extendedDataReceived(255, "ignore this")
3464}}}
3465
3466What does 255 mean?
3467
3468What does this line test?
3469
3470{{{
3471>         app.extendedDataReceived(connection.EXTENDED_DATA_STDERR, "data")
3472>         self.assertEquals(transport.err, "data")
3473>         # should not raise an error if writeErr doesn't exist
3474>         app.extendedDataReceived(connection.EXTENDED_DATA_STDERR, "more")
3475>
3476}}}
3477
3478Huh what? You haven't done anything to writeErr. And if you did, it should go
3479into another test.
3480
3481Also, "should not raise an error" is a bad thing to test for. It's much easier
3482and clearer to test for what a thing should do, rather than what it should not
3483do.
3484
3485{{{
3486>
3487>
3488> class SSHSessionClientTestCase(unittest.TestCase):
3489>     """
3490>     SSHSessionClient is an obsolete class used to connect standard IO to
3491>     an SSHSession.
3492>     """
3493>
3494>
3495>     def test_dataReceived(self):
3496>         """
3497>         When data is received, it should be sent to the transport.
3498>         """
3499>         client = session.SSHSessionClient()
3500>         client.transport = StubTransport()
3501>         client.dataReceived('test data')
3502>         self.assertEquals(client.transport.buf, 'test data')
3503>
3504>
3505>
3506> class ServerSessionTestCase(unittest.TestCase):
3507>     """
3508>     Tests that verify server functionality of SSHSession.
3509>     """
3510>
3511>
3512>     def setUp(self):
3513>         self.conn = StubConnection()
3514>         self.avatar = StubNewAvatar()
3515>         self.session = session.SSHSession(remoteWindow=131072,
3516>                 remoteMaxPacket=32768, conn=self.conn,
3517>                 avatar=self.avatar)
3518>
3519>
3520>     def test_init(self):
3521>         """
3522>         Test that the session is initialized correctly,
3523>         """
3524}}}
3525
3526'Correctly' is a weasel word in tests. Of course you are testing that something
3527happens correctly. Tell us what 'correctly' means.
3528
3529{{{
3530>         self.assertEquals(self.session.client, None)
3531>         self.assertTrue(session.ISessionApplicationFactory.providedBy(
3532>                 self.session.applicationFactory),
3533>                         str(self.session.applicationFactory))
3534>
3535>
3536>     def test_client(self):
3537>         """
3538>         Test that the session performs correctly when it has a client
3539>         application.
3540>         """
3541}}}
3542
3543This docstring is unclear. "correctly", as mentioned before, adds no meaning.
3544
3545Looking at the test below, it would be easier to write a docstring if you were
3546testing fewer things.
3547
3548{{{
3549>         client = MockApplication()
3550>         self.session.dataReceived('1')
3551>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2')
3552>         self.session.extReceived(2, '3')
3553>         self.session.setupSession(client)
3554>         self.session.dataReceived('out')
3555>         self.assertEquals(client.data, ['1', 'out'])
3556>         self.assertEquals(self.session.conn.data[self.session],
3557>                           ['1~', 'out~'])
3558>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, 'err')
3559>         self.assertEquals(client.extendedData,
3560>                           [(connection.EXTENDED_DATA_STDERR, '2'),
3561>                            (2, '3'),
3562>                            (connection.EXTENDED_DATA_STDERR, 'err')])
3563>         self.assertEquals(self.session.conn.extData[self.session],
3564>                           [(connection.EXTENDED_DATA_STDERR + 1, '2~'),
3565>                            (2 + 1, '3~'),
3566>                            (connection.EXTENDED_DATA_STDERR + 1, 'err~')])
3567>         self.session.eofReceived()
3568>         self.assertTrue(client.gotEOF)
3569>         self.assertFalse(client.hasClosed, 'closed() called during EOF')
3570>         self.session.closeReceived()
3571>         self.assertTrue(client.gotClose)
3572>         self.assertFalse(client.hasClosed,
3573>                          'closed() called during closeReceived')
3574>         self.session.closed()
3575>         self.assertTrue(client.hasClosed, 'closed() not called')
3576>
3577}}}
3578
3579This test tests so many things. Please split it up as much as possible.
3580
3581{{{
3582>
3583>     def test_applicationFactory(self):
3584>         """
3585>         Test that SSHSession handles dispatching to applicationFactory correctly.
3586>         """
3587>         self.session.eofReceived()
3588>         self.assertFalse(self.session.applicationFactory.inConnectionOpen)
3589>         self.session.closeReceived()
3590>         self.assertFalse(self.session.applicationFactory.outConnectionOpen)
3591>         self.session.closed()
3592>         self.assertTrue(self.session.applicationFactory.ended)
3593>
3594}}}
3595
3596What does correctly mean?
3597
3598I think this test would be clearer if you used the more traditional 'call
3599logging' style of mock object, rather than one with many custom booleans.
3600
3601{{{
3602>
3603>     def test_subsystem(self):
3604>         """
3605>         Test that SSHSession handles subsystem requests by dispatching to the
3606>         application factory's requestSubsytem() method and connecting the
3607>         ISessionApplication to itself.
3608>         """
3609}}}
3610
3611'itself' is ambiguous here. I *think* you mean "the session", but you could
3612mean "the session application".
3613
3614{{{
3615>         ret = self.session.requestReceived('subsystem', common.NS('bad'))
3616>         self.assertFalse(ret)
3617>         self.assertEquals(self.session.client, None)
3618>         ret = self.session.requestReceived('subsystem',
3619>                 common.NS('echo') + 'abc')
3620}}}
3621
3622The 'echo' subsystem was defined some few hundred lines back. It's worth
3623reminding the reader that it's there.
3624
3625{{{
3626>         self.assertTrue(ret)
3627>         self.assertTrue(session.ISessionApplication.providedBy(
3628>                 self.session.sessionApplication))
3629}}}
3630
3631I really dislike the way this combines an assertion with code that exercises
3632the system-under-test. It makes it harder to understand the expected
3633behaviour. You do it a lot in this file, and I'd really appreciate it if you
3634could change the tests to not do it.
3635
3636{{{
3637>         self.assertIdentical(self.session.sessionApplication,
3638>                 self.session.applicationFactory.subsystem)
3639>         self.assertFalse(self.session.sessionApplication.hasClosed)
3640>         self.assertEquals(self.session.sessionApplication.packetData, 'abc')
3641>         self.assertEquals(self.session.sessionApplication.channel,
3642>                 self.session)
3643>
3644>
3645>     def test_shell(self):
3646>         """
3647>         Test that SSHSession handles shell requests by dispatching to the
3648>         application factory's openShell() method and connecting the
3649>         ISessionApplication to itself.
3650>         """
3651}}}
3652
3653Same comments on 'itself'.
3654
3655{{{
3656>         self.assertTrue(self.session.requestReceived('shell', ''))
3657>         self.assertNotEquals(self.session.sessionApplication, None)
3658>         self.assertIdentical(self.session.sessionApplication,
3659>                 self.session.applicationFactory.shell)
3660>         self.assertEquals(self.session.sessionApplication.channel,
3661>                 self.session)
3662>         self.assertFalse(self.session.sessionApplication.hasClosed)
3663>         self.assertFalse(self.session.requestReceived('shell', ''))
3664>         # fail if we have a shell already
3665>         errors = self.flushLoggedErrors()
3666}}}
3667
3668`errors = self.flushLoggedErrors(RuntimeError)`
3669
3670{{{
3671>         self.assertEquals(len(errors), 1)
3672>         errors[0].trap(RuntimeError)
3673>
3674>
3675>     def test_exec(self):
3676>         """
3677>         Test that SSHSession handles command requests by dispatching to the
3678>         the application factory's execCommand method and connecting the
3679>         ISessionApplication to itself.
3680>         """
3681}}}
3682
3683Ditto 'itself'.
3684
3685{{{
3686>         self.assertTrue(self.session.requestReceived('exec',
3687>             common.NS('good')))
3688>         self.assertNotEquals(self.session.sessionApplication, None)
3689>         self.assertIdentical(self.session.sessionApplication,
3690>                              self.session.applicationFactory.command)
3691>         self.assertEquals(self.session.sessionApplication.channel,
3692>                 self.session)
3693>         self.assertEquals(self.session.sessionApplication.command, 'good')
3694>         self.assertFalse(self.session.sessionApplication.hasClosed)
3695>         self.assertFalse(self.session.requestReceived('exec',
3696>             common.NS('bad')))
3697>         errors = self.flushLoggedErrors()
3698}}}
3699
3700`errors = self.flushLoggedErrors(RuntimeError)`
3701
3702{{{
3703>         self.assertEquals(len(errors), 1)
3704>         errors[0].trap(RuntimeError)
3705>
3706>
3707>     def test_ptyRequest(self):
3708>         """
3709>         Test that SSHSession handles PTY requests by dispatching to the
3710>         application factory's getPTY method.
3711>         """
3712>         term = 'conch'
3713>         windowSize = (80, 25, 0, 0)
3714>         modes = [(0, 1), (2, 3)]
3715>         ret = self.session.requestReceived('pty_req',
3716>                 common.NS(term) +
3717>                 '\x00\x00\x00\x19' + '\x00\x00\x00\x50' +
3718>                 '\x00\x00\x00\x00' * 2 + common.NS('\x00\x00\x00\x00\x01'
3719>                     '\x02\x00\x00\x00\x03'))
3720}}}
3721
3722Formatting this with many more line breaks would make it a lot clearer.
3723
3724Also, if you really cared about the reader, you'd make it obvious that '25' is
3725inextricably bound up with '\x00\x00\x00\x19'.
3726
3727{{{
3728>         self.assertTrue(ret)
3729>         self.assertEquals(self.session.applicationFactory.term, term)
3730>         self.assertEquals(self.session.applicationFactory.windowSize, windowSize)
3731}}}
3732
3733Line too long, please wrap.
3734
3735{{{
3736>         self.assertEquals(self.session.applicationFactory.modes, modes)
3737>         self.assertFalse(self.session.requestReceived('pty_req', ''))
3738>         errors = self.flushLoggedErrors()
3739}}}
3740
3741errors = self.flushLoggedErrors(RuntimeError)
3742
3743{{{
3744>         self.assertEquals(len(errors), 1)
3745>         errors[0].trap(struct.error)
3746>
3747>
3748>     def test_windowChange(self):
3749>         """
3750>         Test that SSHSession handles window size change requests by dispatching
3751>         to the application factory's windowChanged method.
3752>         """
3753>         windowSize = (1, 1, 1, 1)
3754>         ret = self.session.requestReceived('window_change',
3755>                 '\x00\x00\x00\x01' * 4)
3756>         self.assertTrue(ret)
3757>         self.assertEquals(self.session.applicationFactory.windowSize, windowSize)
3758>         self.assertFalse(self.session.requestReceived('window_change', ''))
3759>         errors = self.flushLoggedErrors()
3760}}}
3761
3762errors = self.flushLoggedErrors(RuntimeError)
3763
3764{{{
3765>         self.assertEquals(len(errors), 1)
3766>         errors[0].trap(struct.error)
3767>
3768>
3769>
3770> class ClientSessionTestCase(unittest.TestCase):
3771>     """
3772>     Test methods that use SSHSession as a client.
3773>     """
3774>
3775>
3776>     def setUp(self):
3777>         self.conn = StubConnection()
3778>         self.session = session.SSHSession(remoteWindow=131072,
3779>                 remoteMaxPacket=32768, conn=self.conn)
3780>
3781>
3782>     def test_getPty(self):
3783>         """
3784>         Test that getPty sends the correct request.
3785>         """
3786>         d = self.session.getPty('term', (80, 25, 0, 0), [(0, 1), (2, 3)],
3787>                 True)
3788>         def check(value):
3789>             self.assertEquals(self.conn.requests[self.session],
3790>                     [('pty-req', common.NS('term') + '\x00\x00\x00\x19' +
3791>                         '\x00\x00\x00\x50' + '\x00\x00\x00\x00' * 2 +
3792>                         common.NS('\x00\x00\x00\x00\x01\x02\x00\x00\x00\x03'),
3793>                         True)])
3794}}}
3795
3796Formatting this with many more line breaks would make it a lot clearer.
3797
3798Also, if you really cared about the reader, you'd make it obvious that '25' is
3799inextricably bound up with '\x00\x00\x00\x19'.
3800
3801{{{
3802>             self.assertEquals(self.session.getPty('term', (80, 25, 0, 0), []),
3803>                     None)
3804>         d.addCallback(check)
3805>         return d
3806>
3807>
3808>     def test_changeWindowSize(self):
3809>         """
3810>         Test that windowChange sends the correct request.
3811>         """
3812>         d = self.session.changeWindowSize((80, 25, 0, 0), True)
3813>         def check(value):
3814>             self.assertEquals(self.conn.requests[self.session],
3815>                     [('window-change', '\x00\x00\x00\x19\x00\x00\x00\x50' +
3816>                         '\x00\x00\x00\x00' * 2, True)])
3817}}}
3818
3819Newlines and hex.
3820
3821{{{
3822>             self.assertEquals(self.session.changeWindowSize((80, 25, 0, 0)),
3823>                     None)
3824>         d.addCallback(check)
3825>         return d
3826>
3827>
3828>     def test_openSubsystem(self):
3829>         """
3830>         Test that openSubsystem sends the correct request.
3831>         """
3832}}}
3833
3834What's correct? I don't know.
3835
3836{{{
3837>         d = self.session.openSubsystem('echo', 'data', True)
3838>         def check(value):
3839>             self.assertEquals(self.conn.requests[self.session],
3840>                     [('subsystem', common.NS('echo') + 'data', True)])
3841>             self.assertEquals(self.session.openSubsystem('echo', 'data'),
3842>                     None)
3843>         d.addCallback(check)
3844>         return d
3845>
3846>
3847>     def test_openShell(self):
3848>         """
3849>         Test that openShell sends the correct request.
3850>         """
3851}}}
3852
3853What's correct? I don't know.
3854
3855{{{
3856>         d = self.session.openShell(True)
3857>         def check(value):
3858>             self.assertEquals(self.conn.requests[self.session],
3859>                     [('shell', '', True)])
3860>             self.assertEquals(self.session.openShell(), None)
3861>         d.addCallback(check)
3862>         return d
3863>
3864>
3865>     def test_execCommand(self):
3866>         """
3867>         Test that execCommand sentds the correct request.
3868>         """
3869}}}
3870
3871What's correct? I don't know.
3872
3873Also, 'sends' not 'sentds'.
3874
3875{{{
3876>         d = self.session.execCommand('true', True)
3877>         def check(value):
3878>             self.assertEquals(self.conn.requests[self.session],
3879>                     [('exec', common.NS('true'), True)])
3880>             self.assertEquals(self.session.execCommand('true'), None)
3881>         d.addCallback(check)
3882>         return d
3883}}}
3884
3885OK, that was epic.
3886
3887It'd be great if you could send a diff containing the changes that you made in
3888response to this review.
3889
3890Thanks again,
3891jml