fix(sshd): re-enable TestConcurrentPushSameRepo (#361)

Fixes deis/builder#250

Also fixes intermittent races in tests for wrapInLock

Author: arschles

pkg/sshd/lock_test.go

@@ -14,7 +14,7 @@ const (
 )
 
 var (
-	gitError = errors.New("git receive error")
+	errGitReceive = errors.New("git receive error")
 )
 
 func TestMultipleSameRepoLocks(t *testing.T) {
@@ -92,15 +92,20 @@ func TestDoubleLockUnlock(t *testing.T) {
 }
 
 func TestWrapInLock(t *testing.T) {
-	lck := NewInMemoryRepositoryLock(0)
-	assert.NoErr(t, wrapInLock(lck, "repo", func() error {
+	const repoName = "repo"
+	lck := NewInMemoryRepositoryLock(100 * time.Second)
+	assert.NoErr(t, wrapInLock(lck, repoName, func() error {
 		return nil
 	}))
-	assert.Err(t, gitError, wrapInLock(lck, "repo", func() error {
-		return gitError
+	assert.NoErr(t, lck.Lock(repoName))
+	assert.Err(t, errAlreadyLocked, wrapInLock(lck, repoName, func() error {
+		return errGitReceive
+	}))
+	assert.Err(t, errAlreadyLocked, wrapInLock(lck, repoName, func() error {
+		return nil
 	}))
-	assert.NoErr(t, lck.Lock("repo"))
-	assert.Err(t, errAlreadyLocked, wrapInLock(lck, "repo", func() error {
+	assert.NoErr(t, lck.Unlock(repoName))
+	assert.NoErr(t, wrapInLock(lck, repoName, func() error {
 		return nil
 	}))
 }

pkg/sshd/server_test.go

@@ -142,8 +142,6 @@ func TestPushInvalidArgsLength(t *testing.T) {
 
 // TestConcurrentPushSameRepo tests many concurrent pushes, each to the same repo
 func TestConcurrentPushSameRepo(t *testing.T) {
-	t.Skip("skipping because the global lock prevents testing the repository lock, for multiple concurrent pushes to the same repo")
-	t.SkipNow()
 	const testingServerAddr = "127.0.0.1:2245"
 	key, err := sshTestingHostKey()
 	assert.NoErr(t, err)
@@ -153,7 +151,7 @@ func TestConcurrentPushSameRepo(t *testing.T) {
 	cfg.AddHostKey(key)
 
 	c := NewCircuit()
-	pushLock := NewInMemoryRepositoryLock(0)
+	pushLock := NewInMemoryRepositoryLock(500 * time.Millisecond)
 	runServer(cfg, c, pushLock, testingServerAddr, 2*time.Second, t)
 
 	// Give server time to initialize.
@@ -165,45 +163,32 @@ func TestConcurrentPushSameRepo(t *testing.T) {
 	client, err := ssh.Dial("tcp", testingServerAddr, clientConfig())
 	assert.NoErr(t, err)
 
-	const numPushers = 10
+	const numPushers = 4
 	outCh := make(chan *sshSessionOutput, numPushers)
 	for i := 0; i < numPushers; i++ {
 		go func() {
-			sess, err := client.NewSession()
-			assert.NoErr(t, err)
+			sess, newSessErr := client.NewSession()
+			assert.NoErr(t, newSessErr)
 			defer sess.Close()
-			out, err := sess.Output("git-upload-pack /demo.git")
-			outCh <- &sshSessionOutput{outStr: string(out), err: err}
+			out, outErr := sess.Output("git-upload-pack /demo.git")
+			outCh <- &sshSessionOutput{outStr: string(out), err: outErr}
 		}()
 	}
 
+	// ensure at least 1 output was successful
 	foundOK := false
 	to := 1 * time.Second
-	multiPushLine, err := gitPktLineStr(multiplePush)
-	assert.NoErr(t, err)
 	for i := 0; i < numPushers; i++ {
 		select {
 		case sessOut := <-outCh:
-			output := sessOut.outStr
-			err := sessOut.err
-			if output != multiPushLine && output != "OK" {
-				t.Fatalf("expected 'OK' or '%s', but got '%s' (error '%s')", multiPushLine, output, err)
-			}
-
-			if sessOut.err != nil {
-				t.Fatalf("found '%s' output with an error '%s'", output, err)
-			}
-
-			if !foundOK && output == "OK" {
+			if sessOut.outStr == "OK" {
 				foundOK = true
-			} else if output == "OK" {
-				t.Fatalf("found second 'OK' when shouldn't have")
 			}
-
 		case <-time.After(to):
 			t.Fatalf("didn't receive an output within %s", to)
 		}
 	}
+	assert.True(t, foundOK, "no SSH requests were successful")
 }
 
 // TestConcurrentPushDifferentRepo tests many concurrent pushes, each to a different repo