code review 6601043: go.crypto/ssh: new package: sshtest

go.crypto/ssh: new test subpackage

This proposal is an attempt to improve the state of functional testing in the ssh package. The previous functional tests required the user to give away some personal details, like their password and private key to run the tests, and so were probably not run as frequently as they should.

[dave_cheney.net, 2012-10-03 10:52:48]

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

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

[dave_cheney.net, 2012-10-03 11:22:36]

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

Please take another look.

[dave_cheney.net, 2012-10-03 12:47:09]

On 2012/10/03 11:22:36, dfc wrote:
> Hello mailto:agl@golang.org, mailto:gustav.paul@gmail.com,
mailto:kardianos@gmail.com,
> mailto:fullung@gmail.com (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Just a note about the choice of a sub package for these tests. 

It was my intention to separate these integration tests from the unit-esq tests
in the main ssh package for a number of reasons

1. they are not self hosting, these are not unit tests
2. sadly, I expect them to break more often, and having an individual test
package, I feel, will make it easier to spot this earlier
3. for those that do not want/cant run the integration tests, moving them to a
separate package makes that easier, without having to put up with a bunch of
'test skipped ...' messages in the log.

Having said all this I have had feedback that this arrangement is unorthodox, so
will be happy to entertain moving these back into the main ssh package if that
is the majority view.

[agl1, 2012-10-03 20:14:22]

https://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/auth_test.go
File ssh/sshtest/auth_test.go (right):

https://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/auth_test.go#ne...
ssh/sshtest/auth_test.go:86: func (k *keychain) loadPEM(file string) error {
It's not obvious where this is used, but I might have missed it.

https://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/session_test.go
File ssh/sshtest/session_test.go (right):

https://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/session_test.go...
ssh/sshtest/session_test.go:19: cleanup := startSshd(t)
s/Sshd/SSHd/ perhaps. ("Sshd" looks really weird to me.)

https://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/sshtest_test.go
File ssh/sshtest/sshtest_test.go (right):

https://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/sshtest_test.go...
ssh/sshtest/sshtest_test.go:142: <-time.After(100 * time.Millisecond)
This seems unfortunate. Can we have the sshd log to stderr, or a file, and have
a goroutine watch for a log line that indicates that it has opened the socket?

Or perhaps pass -i to sshd and mimic inetd mode?

[dave_cheney.net, 2012-10-04 04:56:57]

Thank you for your comments. I'm attempting a version of the harness that uses
sshd -i now.

http://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/auth_test.go
File ssh/sshtest/auth_test.go (right):

http://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/auth_test.go#new...
ssh/sshtest/auth_test.go:86: func (k *keychain) loadPEM(file string) error {
On 2012/10/03 20:14:22, agl1 wrote:
> It's not obvious where this is used, but I might have missed it.

I've merged this file back into sshtest_test.go, it wasn't carrying its weight.

http://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/session_test.go
File ssh/sshtest/session_test.go (right):

http://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/session_test.go#...
ssh/sshtest/session_test.go:19: cleanup := startSshd(t)
On 2012/10/03 20:14:22, agl1 wrote:
> s/Sshd/SSHd/ perhaps. ("Sshd" looks really weird to me.)

Done.

http://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/sshtest_test.go
File ssh/sshtest/sshtest_test.go (right):

http://codereview.appspot.com/6601043/diff/10001/ssh/sshtest/sshtest_test.go#...
ssh/sshtest/sshtest_test.go:142: <-time.After(100 * time.Millisecond)
On 2012/10/03 20:14:22, agl1 wrote:
> This seems unfortunate. Can we have the sshd log to stderr, or a file, and
have a goroutine watch for a log line that indicates that it has opened the
socket? Or perhaps pass -i to sshd and mimic inetd mode?

logging to stdout is easy, cmd = os.Stdout, but rather noisy.

I've never tried fd passing in Go, but it would be the best way, i'll try to
make that work with sshd -i

[dave_cheney.net, 2012-10-04 06:06:15]

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

Please take another look.

[kardia, 2012-10-04 06:28:35]

On windows only build tcpip_test.go and not the other tests. Otherwise looks
fine as far as I can tell.

[dave_cheney.net, 2012-10-04 06:30:31]

None of these will work on windows (no sshd). Maybe I should use //
+build tags ?

On Thu, Oct 4, 2012 at 4:28 PM,  <kardianos@gmail.com> wrote:
> On windows only build tcpip_test.go and not the other tests. Otherwise
> looks fine as far as I can tell.
>
> https://codereview.appspot.com/6601043/

[dave_cheney.net, 2012-10-04 07:47:18]

PTAL. I have reworked the file layout to run these tests on unix platforms only.
Refactoring to support windows server processes should be straight forward.

[kardia, 2012-10-04 15:04:46]

On 2012/10/04 07:47:18, dfc wrote:
> PTAL. I have reworked the file layout to run these tests on unix platforms
only.
> Refactoring to support windows server processes should be straight forward.

LGTM.

[agl1, 2012-10-04 16:18:57]

LGTM.

Is go.crypto/ssh/sshtest a stutter? Should it be go.crypto/ssh/test? Up to you.

[dave_cheney.net, 2012-10-05 01:09:51]

On 2012/10/04 16:18:57, agl1 wrote:
> LGTM.
> 
> Is go.crypto/ssh/sshtest a stutter? Should it be go.crypto/ssh/test? Up to
you.

Done. That does mean we end up with test_unix_test.go, which could probably be
improved in the future.

I'm going to submit this today and see what breaks.

[dave_cheney.net, 2012-10-05 01:11:16]

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

go.crypto/ssh: new test subpackage

This proposal is an attempt to improve the state of functional testing in the
ssh package. The previous functional tests required the user to give away some
personal details, like their password and private key to run the tests, and so
were probably not run as frequently as they should.

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