code review 6075046: go.crypt/ssh: Add additional test for server.

go.crypt/ssh: Add additional test for server.

[kardia, 2012-04-19 17:20:58]

Hello golang-dev@googlegroups.com (cc: agl@golang.org,
golang-dev@googlegroups.com),

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

[agl1, 2012-04-19 17:41:30]

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go
File ssh/server_test.go (right):

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode1
ssh/server_test.go:1: package ssh
Needs copyright header.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode13
ssh/server_test.go:13: ServerNetwork = "127.0.0.1:23000"
Listening on a specific point is too error prone for tests. Tests should let the
kernel assign a port.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode15
ssh/server_test.go:15: DataLenMult = 200
I think this can be removed, see next.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode18
ssh/server_test.go:18: DataLen = 16000 * DataLenMult
// DataLen is the number of bytes that we'll send to the SSH server.
DataLen = 16000 * 200

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode23
ssh/server_test.go:23: func CopyN(dst io.Writer, src io.Reader, n int64)
(written int64, err error) {
CopyNRandomly copies n bytes from src to dst. It uses a variable, and random,
buffer size to exercise more code paths.

(and rename the function accordingly.)

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode24
ssh/server_test.go:24: buf := make([]byte, (rand.Intn(30)+1)*1024) //32*1024)
remove "//32*1024)"

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode24
ssh/server_test.go:24: buf := make([]byte, (rand.Intn(30)+1)*1024) //32*1024)
I think the test would exercise things better if buf was always 32KB, but |l :=
len(buf)| was replaced with code that randomly set l to a fraction of len(buf).

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode52
ssh/server_test.go:52: func Copy(dst io.Writer, src io.Reader) (written int64,
err error) {
I think this can be removed because the one use can be replaced with
CopyNRandomly.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode89
ssh/server_test.go:89: // First write date to the server in an easily verifiable
data pattern.
runSSHClient writes random data to the server. The server is expected to echo
the same data back, which is compared against the original.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode92
ssh/server_test.go:92: func runSshClient(t *testing.T) {
s/Ssh/SSH/

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode105
ssh/server_test.go:105: for i, _ := range dumpTo {
crypto.rand.Reader can be used to fill dumpTo with a better pattern.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode110
ssh/server_test.go:110: // To wait for reader to exit.
I think a channel would be best.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode116
ssh/server_test.go:116: var dumpFromBuffer = bytes.NewBuffer(make([]byte,
len(dumpTo)))
Rather than calling Reset, the argument to NewBuffer can be made zero length:
make([]byte, 0, len(dumpTo))

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode124
ssh/server_test.go:124: t.Fatalf("Client/Server communication hung in:: Server:
%t, Write: %t, Read: %t", inServerCopy, inWriterCopy, inReaderCopy)
I'm not sure that this will actually stop the test, although I've not tried it.
I think this can be removed since the test will time out after a while anyway.
If it is removed, in{Reader|Writer}Copy can also be removed.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode140
ssh/server_test.go:140: if err != nil {
can be merged with &&

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode186
ssh/server_test.go:186: func startSshServer(t *testing.T) {
s/Ssh/SSH.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode242
ssh/server_test.go:242: if err != nil {
these two ifs can also be combined.

[kardia, 2012-04-19 18:19:27]

Hello golang-dev@googlegroups.com, agl@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[kardia, 2012-04-19 18:19:34]

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go
File ssh/server_test.go (right):

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode1
ssh/server_test.go:1: package ssh
On 2012/04/19 17:41:30, agl1 wrote:
> Needs copyright header.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode13
ssh/server_test.go:13: ServerNetwork = "127.0.0.1:23000"
On 2012/04/19 17:41:30, agl1 wrote:
> Listening on a specific point is too error prone for tests. Tests should let
the
> kernel assign a port.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode15
ssh/server_test.go:15: DataLenMult = 200
On 2012/04/19 17:41:30, agl1 wrote:
> I think this can be removed, see next.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode18
ssh/server_test.go:18: DataLen = 16000 * DataLenMult
On 2012/04/19 17:41:30, agl1 wrote:
> // DataLen is the number of bytes that we'll send to the SSH server.
> DataLen = 16000 * 200

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode23
ssh/server_test.go:23: func CopyN(dst io.Writer, src io.Reader, n int64)
(written int64, err error) {
On 2012/04/19 17:41:30, agl1 wrote:
> CopyNRandomly copies n bytes from src to dst. It uses a variable, and random,
> buffer size to exercise more code paths.
> 
> (and rename the function accordingly.)

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode24
ssh/server_test.go:24: buf := make([]byte, (rand.Intn(30)+1)*1024) //32*1024)
On 2012/04/19 17:41:30, agl1 wrote:
> remove "//32*1024)"

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode24
ssh/server_test.go:24: buf := make([]byte, (rand.Intn(30)+1)*1024) //32*1024)
On 2012/04/19 17:41:30, agl1 wrote:
> I think the test would exercise things better if buf was always 32KB, but |l
:=
> len(buf)| was replaced with code that randomly set l to a fraction of
len(buf).

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode24
ssh/server_test.go:24: buf := make([]byte, (rand.Intn(30)+1)*1024) //32*1024)
On 2012/04/19 17:41:30, agl1 wrote:
> remove "//32*1024)"

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode52
ssh/server_test.go:52: func Copy(dst io.Writer, src io.Reader) (written int64,
err error) {
On 2012/04/19 17:41:30, agl1 wrote:
> I think this can be removed because the one use can be replaced with
> CopyNRandomly.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode89
ssh/server_test.go:89: // First write date to the server in an easily verifiable
data pattern.
On 2012/04/19 17:41:30, agl1 wrote:
> runSSHClient writes random data to the server. The server is expected to echo
> the same data back, which is compared against the original.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode89
ssh/server_test.go:89: // First write date to the server in an easily verifiable
data pattern.
On 2012/04/19 17:41:30, agl1 wrote:
> runSSHClient writes random data to the server. The server is expected to echo
> the same data back, which is compared against the original.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode92
ssh/server_test.go:92: func runSshClient(t *testing.T) {
On 2012/04/19 17:41:30, agl1 wrote:
> s/Ssh/SSH/

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode105
ssh/server_test.go:105: for i, _ := range dumpTo {
On 2012/04/19 17:41:30, agl1 wrote:
> crypto.rand.Reader can be used to fill dumpTo with a better pattern.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode110
ssh/server_test.go:110: // To wait for reader to exit.
On 2012/04/19 17:41:30, agl1 wrote:
> I think a channel would be best.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode116
ssh/server_test.go:116: var dumpFromBuffer = bytes.NewBuffer(make([]byte,
len(dumpTo)))
On 2012/04/19 17:41:30, agl1 wrote:
> Rather than calling Reset, the argument to NewBuffer can be made zero length:
> make([]byte, 0, len(dumpTo))

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode124
ssh/server_test.go:124: t.Fatalf("Client/Server communication hung in:: Server:
%t, Write: %t, Read: %t", inServerCopy, inWriterCopy, inReaderCopy)
On 2012/04/19 17:41:30, agl1 wrote:
> I'm not sure that this will actually stop the test, although I've not tried
it.
> I think this can be removed since the test will time out after a while anyway.
> If it is removed, in{Reader|Writer}Copy can also be removed.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode140
ssh/server_test.go:140: if err != nil {
On 2012/04/19 17:41:30, agl1 wrote:
> can be merged with &&

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode186
ssh/server_test.go:186: func startSshServer(t *testing.T) {
On 2012/04/19 17:41:30, agl1 wrote:
> s/Ssh/SSH.

Done.

http://codereview.appspot.com/6075046/diff/4001/ssh/server_test.go#newcode242
ssh/server_test.go:242: if err != nil {
On 2012/04/19 17:41:30, agl1 wrote:
> these two ifs can also be combined.

Done.

[agl1, 2012-04-19 20:47:00]

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

go.crypt/ssh: Add additional test for server.

R=golang-dev, agl
CC=golang-dev
http://codereview.appspot.com/6075046

Committer: Adam Langley <agl@golang.org>

http://codereview.appspot.com/6075046/diff/9001/ssh/server_test.go
File ssh/server_test.go (right):

http://codereview.appspot.com/6075046/diff/9001/ssh/server_test.go#newcode1
ssh/server_test.go:1: // Copyright 2011 The Go Authors. All rights reserved.
(Tided up a few things before landing, including the year here.)

[kardia, 2012-05-19 02:56:57]

*** Abandoned ***