From f87c2b11c2a73bc5d5c0ac3df591f89fdcd225a5 Mon Sep 17 00:00:00 2001 From: Russell Jones Date: Fri, 8 Jul 2022 20:27:42 -0700 Subject: [PATCH] Refactor tests under client package. Refactored all tests under "lib/client" to use testify instead of gocheck. --- lib/client/api_test.go | 102 ++++++++++++++-------------- lib/client/client_test.go | 110 +++++++++++++------------------ lib/client/escape/reader_test.go | 85 +++++++++++------------- lib/client/keyagent_test.go | 5 +- 4 files changed, 137 insertions(+), 165 deletions(-) diff --git a/lib/client/api_test.go b/lib/client/api_test.go index f1036508736..e38eb9fa316 100644 --- a/lib/client/api_test.go +++ b/lib/client/api_test.go @@ -22,7 +22,6 @@ import ( "os" "testing" - "github.com/gravitational/trace" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" @@ -31,9 +30,10 @@ import ( "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/observability/tracing" "github.com/gravitational/teleport/lib/utils" + "github.com/gravitational/trace" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" - "gopkg.in/check.v1" ) func TestMain(m *testing.M) { @@ -41,14 +41,6 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } -// register test suite -type APITestSuite struct{} - -// bootstrap check -func TestClientAPI(t *testing.T) { check.TestingT(t) } - -var _ = check.Suite(&APITestSuite{}) - func TestParseProxyHostString(t *testing.T) { t.Parallel() @@ -186,7 +178,7 @@ func TestParseProxyHostString(t *testing.T) { } } -func (s *APITestSuite) TestNew(c *check.C) { +func TestNew(t *testing.T) { conf := Config{ Host: "localhost", HostLogin: "vincent", @@ -197,68 +189,69 @@ func (s *APITestSuite) TestNew(c *check.C) { Tracer: tracing.NoopProvider().Tracer("test"), } err := conf.ParseProxyHost("proxy") - c.Assert(err, check.IsNil) + require.NoError(t, err) tc, err := NewClient(&conf) - c.Assert(err, check.IsNil) - c.Assert(tc, check.NotNil) + require.NoError(t, err) + require.NotNil(t, tc) la := tc.LocalAgent() - c.Assert(la, check.NotNil) + require.NotNil(t, la) } -func (s *APITestSuite) TestParseLabels(c *check.C) { +func TestParseLabels(t *testing.T) { // simplest case: m, err := ParseLabelSpec("key=value") - c.Assert(m, check.NotNil) - c.Assert(err, check.IsNil) - c.Assert(m, check.DeepEquals, map[string]string{ + require.NotNil(t, m) + require.NoError(t, err) + require.Empty(t, cmp.Diff(m, map[string]string{ "key": "value", - }) + })) + // multiple values: m, err = ParseLabelSpec(`type="database";" role"=master,ver="mongoDB v1,2"`) - c.Assert(m, check.NotNil) - c.Assert(err, check.IsNil) - c.Assert(m, check.HasLen, 3) - c.Assert(m["role"], check.Equals, "master") - c.Assert(m["type"], check.Equals, "database") - c.Assert(m["ver"], check.Equals, "mongoDB v1,2") + require.NotNil(t, m) + require.NoError(t, err) + require.Len(t, m, 3) + require.Equal(t, m["role"], "master") + require.Equal(t, m["type"], "database") + require.Equal(t, m["ver"], "mongoDB v1,2") // multiple and unicode: m, err = ParseLabelSpec(`服务器环境=测试,操作系统类别=Linux,机房=华北`) - c.Assert(err, check.IsNil) - c.Assert(m, check.NotNil) - c.Assert(m, check.HasLen, 3) - c.Assert(m["服务器环境"], check.Equals, "测试") - c.Assert(m["操作系统类别"], check.Equals, "Linux") - c.Assert(m["机房"], check.Equals, "华北") + require.NoError(t, err) + require.NotNil(t, m) + require.Len(t, m, 3) + require.Equal(t, m["服务器环境"], "测试") + require.Equal(t, m["操作系统类别"], "Linux") + require.Equal(t, m["机房"], "华北") // invalid specs m, err = ParseLabelSpec(`type="database,"role"=master,ver="mongoDB v1,2"`) - c.Assert(m, check.IsNil) - c.Assert(err, check.NotNil) + require.Nil(t, m) + require.NotNil(t, err) m, err = ParseLabelSpec(`type="database",role,master`) - c.Assert(m, check.IsNil) - c.Assert(err, check.NotNil) + require.Nil(t, m) + require.NotNil(t, err) } -func (s *APITestSuite) TestPortsParsing(c *check.C) { +func TestPortsParsing(t *testing.T) { // empty: ports, err := ParsePortForwardSpec(nil) - c.Assert(ports, check.IsNil) - c.Assert(err, check.IsNil) + require.Nil(t, ports) + require.NoError(t, err) ports, err = ParsePortForwardSpec([]string{}) - c.Assert(ports, check.IsNil) - c.Assert(err, check.IsNil) + require.Nil(t, ports) + require.NoError(t, err) // not empty (but valid) spec := []string{ "80:remote.host:180", "10.0.10.1:443:deep.host:1443", } ports, err = ParsePortForwardSpec(spec) - c.Assert(err, check.IsNil) - c.Assert(ports, check.HasLen, 2) - c.Assert(ports, check.DeepEquals, ForwardedPorts{ + require.NoError(t, err) + require.Len(t, ports, 2) + require.Empty(t, cmp.Diff(ports, ForwardedPorts{ { SrcIP: "127.0.0.1", SrcPort: 80, @@ -271,20 +264,21 @@ func (s *APITestSuite) TestPortsParsing(c *check.C) { DestHost: "deep.host", DestPort: 1443, }, - }) + })) + // back to strings: clone := ports.String() - c.Assert(spec[0], check.Equals, clone[0]) - c.Assert(spec[1], check.Equals, clone[1]) + require.Equal(t, spec[0], clone[0]) + require.Equal(t, spec[1], clone[1]) // parse invalid spec: spec = []string{"foo", "bar"} ports, err = ParsePortForwardSpec(spec) - c.Assert(ports, check.IsNil) - c.Assert(err, check.ErrorMatches, "^Invalid port forwarding spec: .foo.*") + require.Nil(t, ports) + require.ErrorContains(t, err, "Invalid port forwarding spec:") } -func (s *APITestSuite) TestDynamicPortsParsing(c *check.C) { +func TestDynamicPortsParsing(t *testing.T) { tests := []struct { spec []string isError bool @@ -374,13 +368,13 @@ func (s *APITestSuite) TestDynamicPortsParsing(c *check.C) { for _, tt := range tests { specs, err := ParseDynamicPortForwardSpec(tt.spec) if tt.isError { - c.Assert(err, check.NotNil) + require.NotNil(t, err) continue } else { - c.Assert(err, check.IsNil) + require.NoError(t, err) } - c.Assert(specs, check.DeepEquals, tt.output) + require.Empty(t, cmp.Diff(specs, tt.output)) } } @@ -526,7 +520,7 @@ func (s *mockSigner) PublicKey() ssh.PublicKey { } func (s *mockSigner) Sign(rand io.Reader, b []byte) (*ssh.Signature, error) { - return nil, trace.Errorf("mockSigner does not implement Sign") + return nil, fmt.Errorf("mockSigner does not implement Sign") } // Signers implements agent.Agent.Signers. diff --git a/lib/client/client_test.go b/lib/client/client_test.go index 7c535c077cd..3ac216d3132 100644 --- a/lib/client/client_test.go +++ b/lib/client/client_test.go @@ -23,43 +23,27 @@ import ( "net" "os" "strings" + "testing" "time" + "golang.org/x/crypto/ssh" + tracessh "github.com/gravitational/teleport/api/observability/tracing/ssh" "github.com/gravitational/teleport/lib/observability/tracing" "github.com/gravitational/teleport/lib/sshutils" + "github.com/stretchr/testify/require" "github.com/gravitational/trace" - "golang.org/x/crypto/ssh" - "gopkg.in/check.v1" + + "github.com/google/go-cmp/cmp" ) -type ClientTestSuite struct { - client *TeleportClient +func TestHelperFunctions(t *testing.T) { + require.Equal(t, nodeName("one"), "one") + require.Equal(t, nodeName("one:22"), "one") } -var _ = check.Suite(&ClientTestSuite{}) - -func (s *ClientTestSuite) TestHelperFunctions(c *check.C) { - c.Assert(nodeName("one"), check.Equals, "one") - c.Assert(nodeName("one:22"), check.Equals, "one") -} - -func (s *ClientTestSuite) SetUpSuite(c *check.C) { - // create the client: - config := &Config{ - KeysDir: c.MkDir(), - Tracer: tracing.NoopProvider().Tracer("test"), - } - err := config.ParseProxyHost("localhost") - c.Assert(err, check.IsNil) - client, err := NewClient(config) - c.Assert(err, check.IsNil) - c.Assert(client, check.NotNil) - s.client = client -} - -func (s *ClientTestSuite) TestNewSession(c *check.C) { +func TestNewSession(t *testing.T) { nc := &NodeClient{ Namespace: "blue", Tracer: tracing.NoopProvider().Tracer("test"), @@ -68,35 +52,35 @@ func (s *ClientTestSuite) TestNewSession(c *check.C) { ctx := context.Background() // defaults: ses, err := newSession(ctx, nc, nil, nil, nil, nil, nil, true) - c.Assert(err, check.IsNil) - c.Assert(ses, check.NotNil) - c.Assert(ses.NodeClient(), check.Equals, nc) - c.Assert(ses.namespace, check.Equals, nc.Namespace) - c.Assert(ses.env, check.NotNil) - c.Assert(ses.terminal.Stderr(), check.Equals, os.Stderr) - c.Assert(ses.terminal.Stdout(), check.Equals, os.Stdout) - c.Assert(ses.terminal.Stdin(), check.Equals, os.Stdin) + require.NoError(t, err) + require.NotNil(t, ses) + require.Equal(t, ses.NodeClient(), nc) + require.Equal(t, ses.namespace, nc.Namespace) + require.NotNil(t, ses.env) + require.Equal(t, ses.terminal.Stderr(), os.Stderr) + require.Equal(t, ses.terminal.Stdout(), os.Stdout) + require.Equal(t, ses.terminal.Stdin(), os.Stdin) // pass environ map env := map[string]string{ sshutils.SessionEnvVar: "session-id", } ses, err = newSession(ctx, nc, nil, env, nil, nil, nil, true) - c.Assert(err, check.IsNil) - c.Assert(ses, check.NotNil) - c.Assert(ses.env, check.DeepEquals, env) + require.NoError(t, err) + require.NotNil(t, ses) + require.Empty(t, cmp.Diff(ses.env, env)) // the session ID must be taken from tne environ map, if passed: - c.Assert(string(ses.id), check.Equals, "session-id") + require.Equal(t, string(ses.id), "session-id") } // TestProxyConnection verifies that client or server-side disconnect // propagates all the way to the opposite side. -func (s *ClientTestSuite) TestProxyConnection(c *check.C) { +func TestProxyConnection(t *testing.T) { // remoteSrv mocks a remote listener, accepting port-forwarded connections // over SSH. remoteConCh := make(chan net.Conn) remoteErrCh := make(chan error, 3) - remoteSrv := newTestListener(c, func(con net.Conn) { + remoteSrv := newTestListener(t, func(con net.Conn) { defer con.Close() remoteConCh <- con @@ -113,7 +97,7 @@ func (s *ClientTestSuite) TestProxyConnection(c *check.C) { // localSrv mocks a local tsh listener, accepting local connections for // port-forwarding to remote SSH node. proxyErrCh := make(chan error, 3) - localSrv := newTestListener(c, func(con net.Conn) { + localSrv := newTestListener(t, func(con net.Conn) { defer con.Close() proxyErrCh <- proxyConnection(context.Background(), con, remoteSrv.Addr().String(), new(net.Dialer)) @@ -123,7 +107,7 @@ func (s *ClientTestSuite) TestProxyConnection(c *check.C) { // Dial localSrv. This should trigger proxyConnection and a dial to // remoteSrv. localCon, err := net.Dial("tcp", localSrv.Addr().String()) - c.Assert(err, check.IsNil) + require.NoError(t, err) clientErrCh := make(chan error, 3) go func(con net.Conn) { _, err := io.Copy(io.Discard, con) @@ -138,27 +122,27 @@ func (s *ClientTestSuite) TestProxyConnection(c *check.C) { // Simulate a client-side disconnect. All other parties (tsh proxy and // remove listener) should disconnect as well. - c.Log("simulate client-side disconnect") + t.Log("simulate client-side disconnect") err = localCon.Close() - c.Assert(err, check.IsNil) + require.NoError(t, err) for i := 0; i < 3; i++ { select { case err := <-proxyErrCh: - c.Assert(err, check.IsNil) + require.NoError(t, err) case err := <-remoteErrCh: - c.Assert(err, check.IsNil) + require.NoError(t, err) case err := <-clientErrCh: - c.Assert(err, check.IsNil) + require.NoError(t, err) case <-time.After(5 * time.Second): - c.Fatal("proxyConnection, client and server didn't disconnect within 5s after client connection was closed") + t.Fatal("proxyConnection, client and server didn't disconnect within 5s after client connection was closed") } } // Dial localSrv again. This should trigger proxyConnection and a dial to // remoteSrv. localCon, err = net.Dial("tcp", localSrv.Addr().String()) - c.Assert(err, check.IsNil) + require.NoError(t, err) go func(con net.Conn) { _, err := io.Copy(io.Discard, con) if err != nil && strings.Contains(err.Error(), "use of closed network connection") { @@ -169,26 +153,26 @@ func (s *ClientTestSuite) TestProxyConnection(c *check.C) { // Simulate a server-side disconnect. All other parties (tsh proxy and // local client) should disconnect as well. - c.Log("simulate server-side disconnect") + t.Log("simulate server-side disconnect") remoteCon := <-remoteConCh err = remoteCon.Close() - c.Assert(err, check.IsNil) + require.NoError(t, err) for i := 0; i < 3; i++ { select { case err := <-proxyErrCh: - c.Assert(err, check.IsNil) + require.NoError(t, err) case err := <-remoteErrCh: - c.Assert(err, check.IsNil) + require.NoError(t, err) case err := <-clientErrCh: - c.Assert(err, check.IsNil) + require.NoError(t, err) case <-time.After(5 * time.Second): - c.Fatal("proxyConnection, client and server didn't disconnect within 5s after remote connection was closed") + t.Fatal("proxyConnection, client and server didn't disconnect within 5s after remote connection was closed") } } } -func (s *ClientTestSuite) TestListenAndForwardCancel(c *check.C) { +func TestListenAndForwardCancel(t *testing.T) { client := &NodeClient{ Client: &tracessh.Client{ Client: &ssh.Client{ @@ -207,7 +191,7 @@ func (s *ClientTestSuite) TestListenAndForwardCancel(c *check.C) { // Create a new cancelable listener. ctx, cancel := context.WithCancel(context.Background()) ln, err := newWrappedListener(acceptCh) - c.Assert(err, check.IsNil) + require.NoError(t, err) // Start listenAndForward and close the unblock channel once "Accept" has // unblocked. @@ -221,13 +205,13 @@ func (s *ClientTestSuite) TestListenAndForwardCancel(c *check.C) { select { case <-acceptCh: case <-time.After(1 * time.Minute): - c.Fatal("Timed out waiting for Accept to be called.") + t.Fatal("Timed out waiting for Accept to be called.") } // At this point, "Accept" should still be blocking. select { case <-unblockCh: - c.Fatalf("Failed because Accept was unblocked.") + t.Fatalf("Failed because Accept was unblocked.") default: } @@ -238,19 +222,19 @@ func (s *ClientTestSuite) TestListenAndForwardCancel(c *check.C) { select { case <-unblockCh: case <-time.After(1 * time.Minute): - c.Fatal("Timed out waiting for Accept to unblock.") + t.Fatal("Timed out waiting for Accept to unblock.") } } -func newTestListener(c *check.C, handle func(net.Conn)) net.Listener { +func newTestListener(t *testing.T, handle func(net.Conn)) net.Listener { l, err := net.Listen("tcp", "localhost:0") - c.Assert(err, check.IsNil) + require.NoError(t, err) go func() { for { con, err := l.Accept() if err != nil { - c.Logf("listener error: %v", err) + t.Logf("listener error: %v", err) return } go handle(con) diff --git a/lib/client/escape/reader_test.go b/lib/client/escape/reader_test.go index 69ba732f99f..a77937a77e3 100644 --- a/lib/client/escape/reader_test.go +++ b/lib/client/escape/reader_test.go @@ -20,16 +20,9 @@ import ( "io" "testing" - "gopkg.in/check.v1" + "github.com/stretchr/testify/require" ) -func Test(t *testing.T) { check.TestingT(t) } - -type ReaderSuite struct { -} - -var _ = check.Suite(&ReaderSuite{}) - type readerTestCase struct { inChunks [][]byte inErr error @@ -40,8 +33,8 @@ type readerTestCase struct { wantHelp string } -func (*ReaderSuite) runCase(c *check.C, t readerTestCase) { - in := &mockReader{chunks: t.inChunks, finalErr: t.inErr} +func runCase(t *testing.T, tc readerTestCase) { + in := &mockReader{chunks: tc.inChunks, finalErr: tc.inErr} helpOut := new(bytes.Buffer) out := new(bytes.Buffer) var disconnectErr error @@ -51,48 +44,48 @@ func (*ReaderSuite) runCase(c *check.C, t readerTestCase) { }) _, err := io.Copy(out, r) - c.Assert(err, check.Equals, t.wantReadErr) - c.Assert(disconnectErr, check.Equals, t.wantDisconnectErr) - c.Assert(out.String(), check.Equals, t.wantOut) - c.Assert(helpOut.String(), check.Equals, t.wantHelp) + require.Equal(t, err, tc.wantReadErr) + require.Equal(t, disconnectErr, tc.wantDisconnectErr) + require.Equal(t, out.String(), tc.wantOut) + require.Equal(t, helpOut.String(), tc.wantHelp) } -func (s *ReaderSuite) TestNormalReads(c *check.C) { - c.Log("normal read") - s.runCase(c, readerTestCase{ +func TestNormalReads(t *testing.T) { + t.Log("normal read") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello world")}, wantOut: "hello world", }) - c.Log("incomplete sequence") - s.runCase(c, readerTestCase{ + t.Log("incomplete sequence") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello\r~world")}, wantOut: "hello\r~world", }) - c.Log("escaped tilde character") - s.runCase(c, readerTestCase{ + t.Log("escaped tilde character") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello\r~~world")}, wantOut: "hello\r~world", }) - c.Log("other character between newline and tilde") - s.runCase(c, readerTestCase{ + t.Log("other character between newline and tilde") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello\rw~orld")}, wantOut: "hello\rw~orld", }) - c.Log("other character between newline and disconnect sequence") - s.runCase(c, readerTestCase{ + t.Log("other character between newline and disconnect sequence") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello\rw~.orld")}, wantOut: "hello\rw~.orld", }) } -func (s *ReaderSuite) TestReadError(c *check.C) { +func TestReadError(t *testing.T) { customErr := errors.New("oh no") - s.runCase(c, readerTestCase{ + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello world")}, inErr: customErr, wantOut: "hello world", @@ -101,30 +94,30 @@ func (s *ReaderSuite) TestReadError(c *check.C) { }) } -func (s *ReaderSuite) TestEscapeHelp(c *check.C) { - c.Log("single help sequence between reads") - s.runCase(c, readerTestCase{ +func TestEscapeHelp(t *testing.T) { + t.Log("single help sequence between reads") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello\r~?world")}, wantOut: "hello\rworld", wantHelp: helpText, }) - c.Log("single help sequence before any data") - s.runCase(c, readerTestCase{ + t.Log("single help sequence before any data") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("~?hello world")}, wantOut: "hello world", wantHelp: helpText, }) - c.Log("repeated help sequences") - s.runCase(c, readerTestCase{ + t.Log("repeated help sequences") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("hello\r~?world\n~?")}, wantOut: "hello\rworld\n", wantHelp: helpText + helpText, }) - c.Log("help sequence split across reads") - s.runCase(c, readerTestCase{ + t.Log("help sequence split across reads") + runCase(t, readerTestCase{ inChunks: [][]byte{ []byte("hello\r"), []byte("~"), @@ -136,9 +129,9 @@ func (s *ReaderSuite) TestEscapeHelp(c *check.C) { }) } -func (s *ReaderSuite) TestEscapeDisconnect(c *check.C) { - c.Log("single disconnect sequence between reads") - s.runCase(c, readerTestCase{ +func TestEscapeDisconnect(t *testing.T) { + t.Log("single disconnect sequence between reads") + runCase(t, readerTestCase{ inChunks: [][]byte{ []byte("hello"), []byte("\r~."), @@ -149,15 +142,15 @@ func (s *ReaderSuite) TestEscapeDisconnect(c *check.C) { wantDisconnectErr: ErrDisconnect, }) - c.Log("disconnect sequence before any data") - s.runCase(c, readerTestCase{ + t.Log("disconnect sequence before any data") + runCase(t, readerTestCase{ inChunks: [][]byte{[]byte("~.hello world")}, wantReadErr: ErrDisconnect, wantDisconnectErr: ErrDisconnect, }) - c.Log("disconnect sequence split across reads") - s.runCase(c, readerTestCase{ + t.Log("disconnect sequence split across reads") + runCase(t, readerTestCase{ inChunks: [][]byte{ []byte("hello\r"), []byte("~"), @@ -170,7 +163,7 @@ func (s *ReaderSuite) TestEscapeDisconnect(c *check.C) { }) } -func (*ReaderSuite) TestBufferOverflow(c *check.C) { +func TestBufferOverflow(t *testing.T) { in := &mockReader{chunks: [][]byte{make([]byte, 100)}} helpOut := new(bytes.Buffer) out := new(bytes.Buffer) @@ -183,8 +176,8 @@ func (*ReaderSuite) TestBufferOverflow(c *check.C) { go r.runReads() _, err := io.Copy(out, r) - c.Assert(err, check.Equals, ErrTooMuchBufferedData) - c.Assert(disconnectErr, check.Equals, ErrTooMuchBufferedData) + require.Equal(t, err, ErrTooMuchBufferedData) + require.Equal(t, disconnectErr, ErrTooMuchBufferedData) } type mockReader struct { diff --git a/lib/client/keyagent_test.go b/lib/client/keyagent_test.go index eb75c631658..926b7170d27 100644 --- a/lib/client/keyagent_test.go +++ b/lib/client/keyagent_test.go @@ -18,6 +18,7 @@ package client import ( "bytes" + "fmt" "io" "net" "os" @@ -356,7 +357,7 @@ func TestHostKeyVerification(t *testing.T) { require.NoError(t, err) // test user refusing connection: - fakeErr := trace.Errorf("luna cannot be trusted!") + fakeErr := fmt.Errorf("luna cannot be trusted") lka.hostPromptFunc = func(host string, k ssh.PublicKey) error { require.Equal(t, "luna", host) require.Equal(t, pk, k) @@ -365,7 +366,7 @@ func TestHostKeyVerification(t *testing.T) { var a net.TCPAddr err = lka.CheckHostSignature("luna", &a, pk) require.Error(t, err) - require.Equal(t, "luna cannot be trusted!", err.Error()) + require.Equal(t, "luna cannot be trusted", err.Error()) require.True(t, lka.UserRefusedHosts()) // clean user answer: