clients/ssh: test fix

3 files changed, 72 insertions, 150 deletions

diff --git a/clients/ssh/auth_method.go b/clients/ssh/auth_method.go
index 4fe68ca..e55283e 100644
--- a/clients/ssh/auth_method.go
+++ b/clients/ssh/auth_method.go
@@ -2,8 +2,11 @@ package ssh
 
 import (
 	"fmt"
+	"net"
+	"os"
 
 	"golang.org/x/crypto/ssh"
+	"golang.org/x/crypto/ssh/agent"
 	"gopkg.in/src-d/go-git.v4/clients/common"
 )
 
@@ -134,3 +137,17 @@ func (a *PublicKeysCallback) clientConfig() *ssh.ClientConfig {
 		Auth: []ssh.AuthMethod{ssh.PublicKeysCallback(a.Callback)},
 	}
 }
+
+// Opens a pipe with the ssh agent and uses the pipe
+// as the implementer of the public key callback function.
+func NewSSHAgentAuth() (*PublicKeysCallback, error) {
+	pipe, err := net.Dial("unix", os.Getenv("SSH_AUTH_SOCK"))
+	if err != nil {
+		return nil, err
+	}
+
+	return &PublicKeysCallback{
+		User:     "git",
+		Callback: agent.NewClient(pipe).Signers,
+	}, nil
+}
diff --git a/clients/ssh/git_upload_pack.go b/clients/ssh/git_upload_pack.go
index 551ab9c..03b289a 100644
--- a/clients/ssh/git_upload_pack.go
+++ b/clients/ssh/git_upload_pack.go
@@ -42,15 +42,19 @@ type GitUploadPackService struct {
 }
 
 // NewGitUploadPackService initialises a GitUploadPackService.
-// TODO: remove this, as the struct is zero-value safe.
 func NewGitUploadPackService(endpoint common.Endpoint) common.GitUploadPackService {
 	return &GitUploadPackService{endpoint: endpoint}
 }
 
 // Connect cannot be used with SSH clients and always return
 // ErrAuthRequired. Use ConnectWithAuth instead.
-func (s *GitUploadPackService) Connect() (err error) {
-	return ErrAuthRequired
+func (s *GitUploadPackService) Connect() error {
+	auth, err := NewSSHAgentAuth()
+	if err != nil {
+		return err
+	}
+
+	return s.ConnectWithAuth(auth)
 }
 
 // ConnectWithAuth connects to ep using SSH. Authentication is handled
diff --git a/clients/ssh/git_upload_pack_test.go b/clients/ssh/git_upload_pack_test.go
index 3a0a983..b26276d 100644
--- a/clients/ssh/git_upload_pack_test.go
+++ b/clients/ssh/git_upload_pack_test.go
@@ -1,97 +1,30 @@
-// +build ssh
-
 package ssh
 
 import (
-	"fmt"
 	"io/ioutil"
-	"net"
 	"os"
 
-	"golang.org/x/crypto/ssh/agent"
-
 	. "gopkg.in/check.v1"
 	"gopkg.in/src-d/go-git.v4/clients/common"
 	"gopkg.in/src-d/go-git.v4/core"
 )
 
-type SuiteRemote struct{}
-
-var _ = Suite(&SuiteRemote{})
-
-const (
-	fixRepo             = "git@github.com:tyba/git-fixture.git"
-	fixRepoBadVcs       = "www.example.com"
-	fixRepoNonGit       = "https://code.google.com/p/go"
-	fixGitRepoNonGithub = "https://bitbucket.org/user/repo.git"
-)
-
-func (s *SuiteRemote) TestConnect(c *C) {
-	r := NewGitUploadPackService()
-	c.Assert(r.Connect(fixRepo), Equals, ErrAuthRequired)
-}
-
-// We will use a running ssh agent for testing
-// ssh authentication.
-type sshAgentConn struct {
-	pipe net.Conn
-	auth *PublicKeysCallback
+type RemoteSuite struct {
+	Endpoint common.Endpoint
 }
 
-// Opens a pipe with the ssh agent and uses the pipe
-// as the implementer of the public key callback function.
-func newSSHAgentConn() (*sshAgentConn, error) {
-	pipe, err := net.Dial("unix", os.Getenv("SSH_AUTH_SOCK"))
-	if err != nil {
-		return nil, err
-	}
-	return &sshAgentConn{
-		pipe: pipe,
-		auth: &PublicKeysCallback{
-			User:     "git",
-			Callback: agent.NewClient(pipe).Signers,
-		},
-	}, nil
-}
+var _ = Suite(&RemoteSuite{})
 
-// Closes the pipe with the ssh agent
-func (c *sshAgentConn) close() error {
-	return c.pipe.Close()
-}
+func (s *RemoteSuite) SetUpSuite(c *C) {
+	var err error
+	s.Endpoint, err = common.NewEndpoint("git@github.com:git-fixtures/basic.git")
+	c.Assert(err, IsNil)
 
-func (s *SuiteRemote) SetUpSuite(c *C) {
 	if os.Getenv("SSH_AUTH_SOCK") == "" {
 		c.Skip("SSH_AUTH_SOCK is not set")
 	}
 }
 
-func (s *SuiteRemote) TestConnectWithPublicKeysCallback(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
-
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
-	defer func() { c.Assert(r.Disconnect(), IsNil) }()
-	c.Assert(r.connected, Equals, true)
-	c.Assert(r.auth, Equals, agent.auth)
-}
-
-func (s *SuiteRemote) TestConnectBadVcs(c *C) {
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepoBadVcs, nil), ErrorMatches, fmt.Sprintf(".*%s.*", fixRepoBadVcs))
-}
-
-func (s *SuiteRemote) TestConnectNonGit(c *C) {
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepoNonGit, nil), Equals, ErrUnsupportedVCS)
-}
-
-func (s *SuiteRemote) TestConnectNonGithub(c *C) {
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixGitRepoNonGithub, nil), Equals, ErrUnsupportedRepo)
-}
-
 // A mock implementation of client.common.AuthMethod
 // to test non ssh auth method detection.
 type mockAuth struct{}
@@ -99,85 +32,60 @@ type mockAuth struct{}
 func (*mockAuth) Name() string   { return "" }
 func (*mockAuth) String() string { return "" }
 
-func (s *SuiteRemote) TestConnectWithAuthWrongType(c *C) {
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, &mockAuth{}), Equals, ErrInvalidAuthMethod)
-	c.Assert(r.connected, Equals, false)
+func (s *RemoteSuite) TestConnectWithAuthWrongType(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.ConnectWithAuth(&mockAuth{}), Equals, ErrInvalidAuthMethod)
 }
 
-func (s *SuiteRemote) TestAlreadyConnected(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
+func (s *RemoteSuite) TestAlreadyConnected(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
+	defer func() {
+		c.Assert(r.Disconnect(), IsNil)
+	}()
 
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
-	defer func() { c.Assert(r.Disconnect(), IsNil) }()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), Equals, ErrAlreadyConnected)
-	c.Assert(r.connected, Equals, true)
+	c.Assert(r.Connect(), Equals, ErrAlreadyConnected)
 }
 
-func (s *SuiteRemote) TestDisconnect(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
-
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
+func (s *RemoteSuite) TestDisconnect(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
 	c.Assert(r.Disconnect(), IsNil)
-	c.Assert(r.connected, Equals, false)
 }
 
-func (s *SuiteRemote) TestDisconnectedWhenNonConnected(c *C) {
-	r := NewGitUploadPackService()
+func (s *RemoteSuite) TestDisconnectedWhenNonConnected(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
 	c.Assert(r.Disconnect(), Equals, ErrNotConnected)
 }
 
-func (s *SuiteRemote) TestAlreadyDisconnected(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
-
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
+func (s *RemoteSuite) TestAlreadyDisconnected(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
 	c.Assert(r.Disconnect(), IsNil)
 	c.Assert(r.Disconnect(), Equals, ErrNotConnected)
-	c.Assert(r.connected, Equals, false)
 }
 
-func (s *SuiteRemote) TestServeralConnections(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
-
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
+func (s *RemoteSuite) TestServeralConnections(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
 	c.Assert(r.Disconnect(), IsNil)
 
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
-	c.Assert(r.connected, Equals, true)
+	c.Assert(r.Connect(), IsNil)
 	c.Assert(r.Disconnect(), IsNil)
-	c.Assert(r.connected, Equals, false)
 
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
-	c.Assert(r.connected, Equals, true)
+	c.Assert(r.Connect(), IsNil)
 	c.Assert(r.Disconnect(), IsNil)
-	c.Assert(r.connected, Equals, false)
 }
 
-func (s *SuiteRemote) TestInfoNotConnected(c *C) {
-	r := NewGitUploadPackService()
+func (s *RemoteSuite) TestInfoNotConnected(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
 	_, err := r.Info()
 	c.Assert(err, Equals, ErrNotConnected)
 }
 
-func (s *SuiteRemote) TestDefaultBranch(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
-
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
+func (s *RemoteSuite) TestDefaultBranch(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
 	defer func() { c.Assert(r.Disconnect(), IsNil) }()
 
 	info, err := r.Info()
@@ -185,13 +93,9 @@ func (s *SuiteRemote) TestDefaultBranch(c *C) {
 	c.Assert(info.Capabilities.SymbolicReference("HEAD"), Equals, "refs/heads/master")
 }
 
-func (s *SuiteRemote) TestCapabilities(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
-
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
+func (s *RemoteSuite) TestCapabilities(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
 	defer func() { c.Assert(r.Disconnect(), IsNil) }()
 
 	info, err := r.Info()
@@ -199,29 +103,26 @@ func (s *SuiteRemote) TestCapabilities(c *C) {
 	c.Assert(info.Capabilities.Get("agent").Values, HasLen, 1)
 }
 
-func (s *SuiteRemote) TestFetchNotConnected(c *C) {
-	r := NewGitUploadPackService()
+func (s *RemoteSuite) TestFetchNotConnected(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
 	pr := &common.GitUploadPackRequest{}
 	pr.Want(core.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
 	_, err := r.Fetch(pr)
 	c.Assert(err, Equals, ErrNotConnected)
 }
 
-func (s *SuiteRemote) TestFetch(c *C) {
-	agent, err := newSSHAgentConn()
-	c.Assert(err, IsNil)
-	defer func() { c.Assert(agent.close(), IsNil) }()
-
-	r := NewGitUploadPackService()
-	c.Assert(r.ConnectWithAuth(fixRepo, agent.auth), IsNil)
+func (s *RemoteSuite) TestFetch(c *C) {
+	r := NewGitUploadPackService(s.Endpoint)
+	c.Assert(r.Connect(), IsNil)
 	defer func() { c.Assert(r.Disconnect(), IsNil) }()
 
-	pr := &common.GitUploadPackRequest{}
-	pr.Want(core.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
-	reader, err := r.Fetch(pr)
+	req := &common.GitUploadPackRequest{}
+	req.Want(core.NewHash("6ecf0ef2c2dffb796033e5a02219af86ec6584e5"))
+	req.Want(core.NewHash("e8d3ffab552895c19b9fcf7aa264d277cde33881"))
+	reader, err := r.Fetch(req)
 	c.Assert(err, IsNil)
 
 	b, err := ioutil.ReadAll(reader)
 	c.Assert(err, IsNil)
-	c.Assert(b, HasLen, 85374)
+	c.Assert(len(b), Equals, 85585)
 }