code review 6487043: go.crypto/ssh: fix test failure on windows (issue 6487043)

[da...@cheney.net]

Reviewers: agl1, minux, kardia,

Message:
Hello a...@golang.org, minu...@gmail.com, kard...@gmail.com (cc:
golan...@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto


Description:
go.crypto/ssh: fix test failure on windows

Use a handler that does not attempt to send as status message
as the failing test closes the connection abruptly.

Also, check the err response on all shell.ReadLine operations.

Please review this at http://codereview.appspot.com/6487043/

Affected files:
M ssh/session_test.go


Index: ssh/session_test.go
===================================================================
--- a/ssh/session_test.go
+++ b/ssh/session_test.go
@@ -380,8 +380,11 @@
}
}

-// TODO(dfc) currently writes succeed after Close()
-func testClientCannotSendAfterEOF(t *testing.T) {
+func TestClientCannotSendAfterEOF(t *testing.T) {
+ // TODO(dfc) currently writes succeed after Close()
+ t.Logf("test skipped")
+ return
+
conn := dial(shellHandler, t)
defer conn.Close()
session, err := conn.NewSession()
@@ -405,7 +408,7 @@
}

func TestClientCannotSendAfterClose(t *testing.T) {
- conn := dial(shellHandler, t)
+ conn := dial(exitWithoutSignalOrStatus, t)
defer conn.Close()
session, err := conn.NewSession()
if err != nil {
@@ -457,21 +460,21 @@
defer ch.Close()
// this string is returned to stdout
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
sendStatus(0, ch, t)
}

func exitStatusNonZeroHandler(ch *serverChan, t *testing.T) {
defer ch.Close()
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
sendStatus(15, ch, t)
}

func exitSignalAndStatusHandler(ch *serverChan, t *testing.T) {
defer ch.Close()
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
sendStatus(15, ch, t)
sendSignal("TERM", ch, t)
}
@@ -479,31 +482,37 @@
func exitSignalHandler(ch *serverChan, t *testing.T) {
defer ch.Close()
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
sendSignal("TERM", ch, t)
}

func exitSignalUnknownHandler(ch *serverChan, t *testing.T) {
defer ch.Close()
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
sendSignal("SYS", ch, t)
}

func exitWithoutSignalOrStatus(ch *serverChan, t *testing.T) {
defer ch.Close()
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
}

func shellHandler(ch *serverChan, t *testing.T) {
defer ch.Close()
// this string is returned to stdout
shell := newServerShell(ch, "golang")
- shell.ReadLine()
+ readLine(shell, t)
sendStatus(0, ch, t)
}

+func readLine(shell *ServerTerminal, t *testing.T) {
+ if _, err := shell.ReadLine(); err != nil && err != io.EOF {
+ t.Fatalf("unable to read line: %v", err)
+ }
+}
+
func sendStatus(status uint32, ch *serverChan, t *testing.T) {
msg := exitStatusMsg{
PeersId: ch.remoteId,
@@ -549,7 +558,7 @@
// send a bogus zero sized window update
ch.sendWindowAdj(0)
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
sendStatus(0, ch, t)
}

@@ -559,7 +568,7 @@
// the initial 1 << 14 window.
ch.sendWindowAdj(1024 * 1024)
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
io.Copy(ioutil.Discard, ch.serverConn)
}

@@ -569,7 +578,7 @@
// the initial 1 << 14 window.
ch.sendWindowAdj(1024 * 1024)
shell := newServerShell(ch, "> ")
- shell.ReadLine()
+ readLine(shell, t)
// try to send more than the 32k window
// will allow
if err := ch.writePacket(make([]byte, 128*1024)); err == nil {

[kard...@gmail.com]

I can confirm this makes tests pass on Windows. Changes look benign.

http://codereview.appspot.com/6487043/

[da...@cheney.net]

On 2012/08/25 06:29:43, kardia wrote:
> I can confirm this makes tests pass on Windows. Changes look benign.

Thank you. Can you confirm before this CL the test was failing on your
windows host ?

http://codereview.appspot.com/6487043/

[Daniel Theophanes]

Yes. Before it was failing, with the patch it passed.
Windows 7, x64

-Daniel

[minu...@gmail.com]

w/o this CL test fails on windows/386 with a probability of about 1/2.
after applying this CL, i ran the test for 20 times without seeing any
failure,
so i'm reasonably sure this fixed the test.

http://codereview.appspot.com/6487043/

[Dave Cheney]

Thank you. I will submit soon.

[da...@cheney.net]

*** Submitted as
http://code.google.com/p/go/source/detail?r=87f3ce85c32a&repo=crypto ***

go.crypto/ssh: fix test failure on windows

Use a handler that does not attempt to send a status message

as the failing test closes the connection abruptly.

Also, check the err response on all shell.ReadLine operations.

R=agl, minux.ma, kardianos
CC=golang-dev
http://codereview.appspot.com/6487043


http://codereview.appspot.com/6487043/