code review 5385041: exp/ssh: add simple tcpip.go functional tests

exp/ssh: add simple tcpip.go functional tests

[dave_cheney.net, 2011-11-20 08:51:39]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-11-20 16:53:25]

Go style doesn't mix case of abbreviations, as in TcpipHttps.

On Sun, Nov 20, 2011 at 3:51 AM, <dave@cheney.net> wrote:

> Reviewers: rsc, agl1, niemeyer,
>
> Message:
> Hello rsc@golang.org, agl@golang.org, n13m3y3r@gmail.com (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> exp/ssh: add simple tcpip.go functional tests
>
> Please review this at
http://codereview.appspot.com/**5385041/<http://codereview.appspot.com/5385041/>
>
> Affected files:
>  A src/pkg/exp/ssh/tcpip_func_**test.go
>
>
> Index: src/pkg/exp/ssh/tcpip_func_**test.go
> ==============================**==============================**=======
> new file mode 100644
> --- /dev/null
> +++ b/src/pkg/exp/ssh/tcpip_func_**test.go
> @@ -0,0 +1,79 @@
> +// Copyright 2011 The Go Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +package ssh
> +
> +// direct-tcpip functional tests
> +
> +import (
> +       "net"
> +       "net/http"
> +       "testing"
> +)
> +
> +func TestTcpipHttp(t *testing.T) {
> +       if *sshuser == "" {
> +               t.Log("ssh.user not defined, skipping test")
> +               return
> +       }
> +       config := &ClientConfig{
> +               User: *sshuser,
> +               Auth: []ClientAuth{
> +                       ClientAuthPassword(password(***sshpass)),
> +               },
> +       }
> +       conn, err := Dial("tcp", "localhost:22", config)
> +       if err != nil {
> +               t.Fatalf("Unable to connect: %s", err)
> +       }
> +       defer conn.Close()
> +       tr := &http.Transport{
> +               Dial: func(n, addr string) (net.Conn, error) {
> +                       return conn.Dial(n, addr)
> +               },
> +       }
> +       client := &http.Client{
> +               Transport: tr,
> +       }
> +       // google.com will generate at least one redirect, possibly three
> +       // depending on your location.
> +       resp, err := client.Get("http://google.com/**")
> +       if err != nil {
> +               t.Fatalf("unable to proxy: %s", err)
> +       }
> +       // got a body without error
> +       t.Log(resp)
> +}
> +
> +func TestTcpipHttps(t *testing.T) {
> +       if *sshuser == "" {
> +               t.Log("ssh.user not defined, skipping test")
> +               return
> +       }
> +       config := &ClientConfig{
> +               User: *sshuser,
> +               Auth: []ClientAuth{
> +                       ClientAuthPassword(password(***sshpass)),
> +               },
> +       }
> +       conn, err := Dial("tcp", "localhost:22", config)
> +       if err != nil {
> +               t.Fatalf("Unable to connect: %s", err)
> +       }
> +       defer conn.Close()
> +       tr := &http.Transport{
> +               Dial: func(n, addr string) (net.Conn, error) {
> +                       return conn.Dial(n, addr)
> +               },
> +       }
> +       client := &http.Client{
> +               Transport: tr,
> +       }
> +       resp, err :=
client.Get("https://encrypted.**google.com/<https://encrypted.google.com/>
> ")
> +       if err != nil {
> +               t.Fatalf("unable to proxy: %s", err)
> +       }
> +       // got a body without error
> +       t.Log(resp)
> +}
>
>
>

[dave_cheney.net, 2011-11-21 07:47:43]

Fixed. I hope that is what you meant.

[rsc, 2011-11-21 15:36:22]

looks fine to me.  leaving for brad.

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
File src/pkg/exp/ssh/tcpip_func_test.go (right):

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:7: // direct-tcpip functional tests
// Test TCP connections tunnelled over SSH.

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:15: func TestTCPIPHTTP(t *testing.T) {
How about just TestDialHTTP and TestDialHTTPS?

[bradfitz, 2011-11-21 15:44:26]

LGTM

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
File src/pkg/exp/ssh/tcpip_func_test.go (right):

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:32: Dial: func(n, addr string) (net.Conn,
error) {
nice to see the Dial hook being useful here.

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:41: resp, err :=
client.Get("http://google.com/")
http://ip.appspot.com/ might be a smaller, simpler page to hit, with a
well-known response body format.  *shrug*

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:73: resp, err :=
client.Get("https://encrypted.google.com/")
lot of repetitive code here for what's effectively one line difference.

move this all to a function?

func TestDialHTTP(t *testing.T) {
    doTest(t, "http://ip.appspot.com/")
}

func TestDialHTTPS(t *testing.T) {
    doTest(t, "https://ip.appspot.com/")
}

Your call.

[dave_cheney.net, 2011-11-22 08:53:37]

Hello rsc@golang.org, agl@golang.org, n13m3y3r@gmail.com, bradfitz@golang.org
(cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2011-11-22 08:56:43]

Thanks for your comments Brad.

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
File src/pkg/exp/ssh/tcpip_func_test.go (right):

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:32: Dial: func(n, addr string) (net.Conn,
error) {
Yeah, it's awesome. 

On 2011/11/21 15:44:27, bradfitz wrote:
> nice to see the Dial hook being useful here.

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:41: resp, err :=
client.Get("http://google.com/")
I'm calling google.com so it generates a few redirects so we get a few more
calls to client.Dial for free.

On 2011/11/21 15:44:27, bradfitz wrote:
> http://ip.appspot.com/ might be a smaller, simpler page to hit, with a
> well-known response body format.  *shrug*

http://codereview.appspot.com/5385041/diff/6002/src/pkg/exp/ssh/tcpip_func_te...
src/pkg/exp/ssh/tcpip_func_test.go:73: resp, err :=
client.Get("https://encrypted.google.com/")
Yeah, I can make that much less verbose.

On 2011/11/21 15:44:27, bradfitz wrote:
> lot of repetitive code here for what's effectively one line difference.
> 
> move this all to a function?
> 
> func TestDialHTTP(t *testing.T) {
>     doTest(t, "http://ip.appspot.com/")
> }
> 
> func TestDialHTTPS(t *testing.T) {
>     doTest(t, "https://ip.appspot.com/")
> }
> 
> Your call.

[agl1, 2011-11-23 14:38:15]

*** Submitted as http://code.google.com/p/go/source/detail?r=a5185f942ad9 ***

exp/ssh: add simple tcpip.go functional tests

R=rsc, agl, n13m3y3r, bradfitz
CC=golang-dev
http://codereview.appspot.com/5385041

Committer: Adam Langley <agl@golang.org>