From 0f9fa9507e145e579e7205710fc6b2561b4f27ff Mon Sep 17 00:00:00 2001
From: Jakob Borg <jakob@nym.se>
Date: Tue, 20 Oct 2015 08:51:14 +0200
Subject: [PATCH] Tests must use locking to avoid race (fixes #2394)

---
 lib/model/progressemitter.go |  6 ++++++
 lib/model/queue.go           | 12 ++++++++++++
 lib/model/rwfolder_test.go   | 36 ++++++++++++------------------------
 3 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/lib/model/progressemitter.go b/lib/model/progressemitter.go
index eba5d2fa8..70b7ba9df 100755
--- a/lib/model/progressemitter.go
+++ b/lib/model/progressemitter.go
@@ -136,3 +136,9 @@ func (t *ProgressEmitter) BytesCompleted(folder string) (bytes int64) {
 func (t *ProgressEmitter) String() string {
 	return fmt.Sprintf("ProgressEmitter@%p", t)
 }
+
+func (t *ProgressEmitter) lenRegistry() int {
+	t.mut.Lock()
+	defer t.mut.Unlock()
+	return len(t.registry)
+}
diff --git a/lib/model/queue.go b/lib/model/queue.go
index 8b247495c..c1e81ce9f 100644
--- a/lib/model/queue.go
+++ b/lib/model/queue.go
@@ -109,6 +109,18 @@ func (q *jobQueue) Shuffle() {
 	}
 }
 
+func (q *jobQueue) lenQueued() int {
+	q.mut.Lock()
+	defer q.mut.Unlock()
+	return len(q.queued)
+}
+
+func (q *jobQueue) lenProgress() int {
+	q.mut.Lock()
+	defer q.mut.Unlock()
+	return len(q.progress)
+}
+
 func (q *jobQueue) SortSmallestFirst() {
 	q.mut.Lock()
 	defer q.mut.Unlock()
diff --git a/lib/model/rwfolder_test.go b/lib/model/rwfolder_test.go
index be4da0048..7fe0385cb 100644
--- a/lib/model/rwfolder_test.go
+++ b/lib/model/rwfolder_test.go
@@ -409,7 +409,7 @@ func TestDeregisterOnFailInCopy(t *testing.T) {
 	p.queue.Push("filex", 0, 0)
 	p.queue.Pop()
 
-	if len(p.queue.progress) != 1 {
+	if p.queue.lenProgress() != 1 {
 		t.Fatal("Expected file in progress")
 	}
 
@@ -437,7 +437,7 @@ func TestDeregisterOnFailInCopy(t *testing.T) {
 	case state := <-finisherBufferChan:
 		// At this point the file should still be registered with both the job
 		// queue, and the progress emitter. Verify this.
-		if len(p.progressEmitter.registry) != 1 || len(p.queue.progress) != 1 || len(p.queue.queued) != 0 {
+		if p.progressEmitter.lenRegistry() != 1 || p.queue.lenProgress() != 1 || p.queue.lenQueued() != 0 {
 			t.Fatal("Could not find file")
 		}
 
@@ -452,24 +452,16 @@ func TestDeregisterOnFailInCopy(t *testing.T) {
 			t.Fatal("File not closed?")
 		}
 
-		p.queue.mut.Lock()
-		lenQProgr := len(p.queue.progress)
-		lenQQueued := len(p.queue.queued)
-		p.queue.mut.Unlock()
-		if len(p.progressEmitter.registry) != 0 || lenQProgr != 0 || lenQQueued != 0 {
-			t.Fatal("Still registered", len(p.progressEmitter.registry), len(p.queue.progress), len(p.queue.queued))
+		if p.progressEmitter.lenRegistry() != 0 || p.queue.lenProgress() != 0 || p.queue.lenQueued() != 0 {
+			t.Fatal("Still registered", p.progressEmitter.lenRegistry(), p.queue.lenProgress(), p.queue.lenQueued())
 		}
 
 		// Doing it again should have no effect
 		finisherChan <- state
 		time.Sleep(100 * time.Millisecond)
 
-		p.queue.mut.Lock()
-		lenQProgr = len(p.queue.progress)
-		lenQQueued = len(p.queue.queued)
-		p.queue.mut.Unlock()
-		if len(p.progressEmitter.registry) != 0 || lenQProgr != 0 || lenQQueued != 0 {
-			t.Fatal("Still registered")
+		if p.progressEmitter.lenRegistry() != 0 || p.queue.lenProgress() != 0 || p.queue.lenQueued() != 0 {
+			t.Fatal("Still registered", p.progressEmitter.lenRegistry(), p.queue.lenProgress(), p.queue.lenQueued())
 		}
 	case <-time.After(time.Second):
 		t.Fatal("Didn't get anything to the finisher")
@@ -509,7 +501,7 @@ func TestDeregisterOnFailInPull(t *testing.T) {
 	p.queue.Push("filex", 0, 0)
 	p.queue.Pop()
 
-	if len(p.queue.progress) != 1 {
+	if p.queue.lenProgress() != 1 {
 		t.Fatal("Expected file in progress")
 	}
 
@@ -530,7 +522,7 @@ func TestDeregisterOnFailInPull(t *testing.T) {
 	case state := <-finisherBufferChan:
 		// At this point the file should still be registered with both the job
 		// queue, and the progress emitter. Verify this.
-		if len(p.progressEmitter.registry) != 1 || len(p.queue.progress) != 1 || len(p.queue.queued) != 0 {
+		if p.progressEmitter.lenRegistry() != 1 || p.queue.lenProgress() != 1 || p.queue.lenQueued() != 0 {
 			t.Fatal("Could not find file")
 		}
 
@@ -545,20 +537,16 @@ func TestDeregisterOnFailInPull(t *testing.T) {
 			t.Fatal("File not closed?")
 		}
 
-		p.queue.mut.Lock()
-		lenQProgr := len(p.queue.progress)
-		lenQQueued := len(p.queue.queued)
-		p.queue.mut.Unlock()
-		if len(p.progressEmitter.registry) != 0 || lenQProgr != 0 || lenQQueued != 0 {
-			t.Fatal("Still registered", len(p.progressEmitter.registry), lenQProgr, lenQQueued)
+		if p.progressEmitter.lenRegistry() != 0 || p.queue.lenProgress() != 0 || p.queue.lenQueued() != 0 {
+			t.Fatal("Still registered", p.progressEmitter.lenRegistry(), p.queue.lenProgress(), p.queue.lenQueued())
 		}
 
 		// Doing it again should have no effect
 		finisherChan <- state
 		time.Sleep(100 * time.Millisecond)
 
-		if len(p.progressEmitter.registry) != 0 || len(p.queue.progress) != 0 || len(p.queue.queued) != 0 {
-			t.Fatal("Still registered")
+		if p.progressEmitter.lenRegistry() != 0 || p.queue.lenProgress() != 0 || p.queue.lenQueued() != 0 {
+			t.Fatal("Still registered", p.progressEmitter.lenRegistry(), p.queue.lenProgress(), p.queue.lenQueued())
 		}
 	case <-time.After(time.Second):
 		t.Fatal("Didn't get anything to the finisher")