code review 6475063: go.crypto/ssh: improve test reliability

go.crypto/ssh: improve test reliability

Fixes issue 3989.

Tested for several hours on an 8 core ec2 instance with
random GOMAXPROC values.

Also, rolls server_test.go into session_test using the
existing dial() framework.

[dave_cheney.net, 2012-08-26 06:38:47]

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

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

[kardia, 2012-08-27 04:50:27]

Could you point out what changed? I don't have an issue with moving the test,
but I don't see what was fixed. Did the issue end up being a bad test, and not
anything to do with not checking the proper length?

On 2012/08/26 06:38:47, dfc wrote:
> Hello mailto:fullung@gmail.com, mailto:agl@golang.org,
mailto:kardianos@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go.crypto

[dave_cheney.net, 2012-08-27 04:56:23]

> Could you point out what changed? I don't have an issue with moving the
> test, but I don't see what was fixed. Did the issue end up being a bad
> test, and not anything to do with not checking the proper length?

Here are the changes

1. server_test was merged into session_test as it ended up duplicating
a lot of the test ssh server logic.

2. All the Fatal{,f}s that occurred on other goroutines were changed
to Error{,f}s, this was a programming error.

3. The actual cause of the TestServerWindow test failing occasionally
with a short read was the CopyNRandom function should have handled EOF
as io.Copy does.

[agl1, 2012-08-27 14:30:28]

LGTM

https://codereview.appspot.com/6475063/diff/6002/ssh/session_test.go
File ssh/session_test.go (right):

https://codereview.appspot.com/6475063/diff/6002/ssh/session_test.go#newcode668
ssh/session_test.go:668: l := (rand.Intn(30) + 1) * 1024
Isn't the probability of l being > n rather high? It also never tests small or
odd numbers.

[dave_cheney.net, 2012-08-27 21:21:09]

https://codereview.appspot.com/6475063/diff/6002/ssh/session_test.go
File ssh/session_test.go (right):

https://codereview.appspot.com/6475063/diff/6002/ssh/session_test.go#newcode668
ssh/session_test.go:668: l := (rand.Intn(30) + 1) * 1024
On 2012/08/27 14:30:28, agl1 wrote:
> Isn't the probability of l being > n rather high? It also never tests small or
> odd numbers.

In this test case n is 3.2mb, so l will only be bigger than n in the final 10%
of the test. You make a good point about the 1024 multiplier. I'll change it to
be l := rand.Intn(1 << 15)

[dave_cheney.net, 2012-08-27 22:06:21]

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

go.crypto/ssh: improve test reliability

Fixes issue 3989.

Tested for several hours on an 8 core ec2 instance with
random GOMAXPROC values.

Also, rolls server_test.go into session_test using the
existing dial() framework.

R=fullung, agl, kardianos
CC=golang-dev
http://codereview.appspot.com/6475063