Merge lp:~themue/juju-core/031-fix-resumer-test into lp:~go-bot/juju-core/trunk

[Frank Mueller]

resumer: fix test race

The resumer test might lead to a race (detected
by Roger). So changed it to taking and comparing
timestamps for each call of the resumer mock.

https://codereview.appspot.com/10550043/

resumer: fix test race

The resumer test might lead to a race (detected
by Roger). So changed it to taking and comparing
timestamps for each call of the resumer mock.

https://codereview.appspot.com/10550043/

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/10550043/

[Dimiter Naydenov (dimitern)]

LGTM modulo a single suggestion.

https://codereview.appspot.com/10550043/diff/1/worker/resumer/resumer_test.go
File worker/resumer/resumer_test.go (right):

https://codereview.appspot.com/10550043/diff/1/worker/resumer/resumer_test.go#newcode47
worker/resumer/resumer_test.go:47: c.Assert(len(tr.timestamps) > 0,
Equals, true)
This code block is a bit obscure to my puny mind - can you put some
comment describing what we're actually doing here please?

https://codereview.appspot.com/10550043/

[Frank Mueller (themue)]

https://codereview.appspot.com/10550043/diff/1/worker/resumer/resumer_test.go
File worker/resumer/resumer_test.go (right):

https://codereview.appspot.com/10550043/diff/1/worker/resumer/resumer_test.go#newcode47
worker/resumer/resumer_test.go:47: c.Assert(len(tr.timestamps) > 0,
Equals, true)
On 2013/06/27 14:33:26, dimitern wrote:
> This code block is a bit obscure to my puny mind - can you put some
comment
> describing what we're actually doing here please?

Done.

https://codereview.appspot.com/10550043/

=== modified file 'worker/resumer/resumer_test.go'
--- worker/resumer/resumer_test.go	2013-06-21 00:19:44 +0000
+++ worker/resumer/resumer_test.go	2013-06-27 18:05:33 +0000
@@ -34,24 +34,36 @@
 func (s *ResumerSuite) TestResumerCalls(c *C) {
 	// Shorter interval and mock help to count
 	// the resumer calls in a given timespan.
-	resumer.SetInterval(10 * time.Millisecond)
+	testInterval := 10 * time.Millisecond
+	resumer.SetInterval(testInterval)
 	defer resumer.RestoreInterval()
 
-	tr := &transactionResumerMock{0}
+	tr := &transactionResumerMock{[]time.Time{}}
 	rr := resumer.NewResumer(tr)
 	defer func() { c.Assert(rr.Stop(), IsNil) }()
 
-	time.Sleep(55 * time.Millisecond)
-	c.Assert(tr.counter, Equals, 5)
+	time.Sleep(10 * testInterval)
+
+	// Check that a numner of calls has happened with a time
+	// difference somewhere between the interval and twice the
+	// interval. A more precise time behavior cannot be 
+	// specified due to the load during the test.
+	c.Assert(len(tr.timestamps) > 0, Equals, true)
+	for i := 1; i < len(tr.timestamps); i++ {
+		diff := tr.timestamps[i].Sub(tr.timestamps[i-1])
+
+		c.Assert(diff >= testInterval, Equals, true)
+		c.Assert(diff <= 2*testInterval, Equals, true)
+	}
 }
 
 // transactionResumerMock is used to check the
 // calls of ResumeTransactions().
 type transactionResumerMock struct {
-	counter int
+	timestamps []time.Time
 }
 
-func (t *transactionResumerMock) ResumeTransactions() error {
-	t.counter++
+func (tr *transactionResumerMock) ResumeTransactions() error {
+	tr.timestamps = append(tr.timestamps, time.Now())
 	return nil
 }