Opened 8 years ago

Last modified 8 years ago

#3995 enhancement new

need unit test that ssh_CHANNEL_OPEN sends a packet rather than raising a TypeError

Reported by: jesstess Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess, Thijs Triemstra, Jean-Paul Calderone Branch:
Author:

Description

Prior to the changes in #3657, ConchUser.lookupChannel raised a ConchError with the arguments reversed. twisted.conch.ssh.connection.ssh_CHANNEL_OPEN uses the attributes of ConchError to send an SSH message so this reversal caused the traceback described at the top of #3657.

Once #3657 is closed, we need a unit test that the behavior is now correct and ssh_CHANNEL_OPEN sends a packet rather than raising a TypeError.

Attachments (1)

conch-SSH_CHANNEL_OPEN-test.patch (1.7 KB) - added by jesstess 8 years ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 8 years ago by jesstess

Keywords: review added
Owner: jesstess deleted

The patch tests that a ConchError coming from lookupChannel is logged and doesn't cause an exception when trying to stuff a packet with the textual information in the ConchError's 'data' field.

comment:2 Changed 8 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to jesstess

The patch should also update the copyright date.

Changed 8 years ago by jesstess

comment:3 Changed 8 years ago by jesstess

Keywords: review added
Owner: jesstess deleted

Copyright updated.

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

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: set to jesstess

The test looks good, as far as it goes. It needs to go a bit futher, though (and its docstring suggests this): ssh_CHANNEL_OPEN ends with a call to sendPacket; the test should verify that that call is doing the right thing. I'm not quite sure what the right thing is, or whether the implementation is currently correct or not, partly because I'm not sure how this patch is intended to interact with the patch from #3657 (and partly because this is a pretty low-level part of Conch which I haven't looked at before :). Can you elaborate on that, or re-submit this after #3657 is resolved?

comment:5 Changed 7 years ago by <automation>

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