Code review comment for lp:~axwalk/juju-core/lp1319652-instancepoller-testbatching

Reviewers: mp+220014_code.launchpad.net,

Message:
Please take a look.

Description:
worker/instancepoller: TestBatching less race-y

The existing TestBatching test was very prone to
failure due to the nature of the test. The test
ensures that requests to Instances are batched
using a token bucket.

Whereas the existing test streamed requests, the
new implementation compresses the addition of
requests into the point where we service an
Instances call. We can thus reliably determine
the number of batches that will be made, as calls
to Instances must complete before additional
requests are gathered.

Fixes lp:1319652

https://code.launchpad.net/~axwalk/juju-core/lp1319652-instancepoller-testbatching/+merge/220014

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/96440043/

Affected files (+48, -24 lines):
   A [revision details]
   M worker/instancepoller/aggregate_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140519073812-9ribeb9ud0q2glco
+New revision: <email address hidden>

Index: worker/instancepoller/aggregate_test.go
=== modified file 'worker/instancepoller/aggregate_test.go'
--- worker/instancepoller/aggregate_test.go 2014-05-13 06:12:51 +0000
+++ worker/instancepoller/aggregate_test.go 2014-05-19 11:11:39 +0000
@@ -116,32 +116,54 @@
   c.Assert(len(testGetter.ids), gc.DeepEquals, 2)
  }

+type batchingInstanceGetter struct {
+ testInstanceGetter
+ wg sync.WaitGroup
+ aggregator *aggregator
+ batchSize int
+ started int
+}
+
+func (g *batchingInstanceGetter) Instances(ids []instance.Id)
([]instance.Instance, error) {
+ insts, err := g.testInstanceGetter.Instances(ids)
+ g.startRequests()
+ return insts, err
+}
+
+func (g *batchingInstanceGetter) startRequests() {
+ n := len(g.results) - g.started
+ if n > g.batchSize {
+ n = g.batchSize
+ }
+ for i := 0; i < n; i++ {
+ g.startRequest()
+ }
+}
+
+func (g *batchingInstanceGetter) startRequest() {
+ g.started++
+ go func() {
+ _, err := g.aggregator.instanceInfo("foo")
+ if err != nil {
+ panic(err)
+ }
+ g.wg.Done()
+ }()
+}
+
  func (s *aggregateSuite) TestBatching(c *gc.C) {
   s.PatchValue(&gatherTime, 10*time.Millisecond)
- testGetter := new(testInstanceGetter)
-
- aggregator := newAggregator(testGetter)
- for i := 0; i < 100; i++ {
- testGetter.results = append(testGetter.results,
newTestInstance("foobar", []string{"127.0.0.1", "192.168.1.1"}))
- }
- var wg sync.WaitGroup
- makeRequest := func() {
- _, err := aggregator.instanceInfo("foo")
- c.Check(err, gc.IsNil)
- wg.Done()
- }
- startTime := time.Now()
- wg.Add(100)
- for i := 0; i < 100; i++ {
- go makeRequest()
- time.Sleep(time.Millisecond)
- }
- wg.Wait()
- totalTime := time.Now().Sub(startTime)
- // +1 because we expect one extra call for the first request
- expectedMax := int32((totalTime / (10 * time.Millisecond)) + 1)
- c.Assert(testGetter.counter, jc.LessThan, expectedMax+1)
- c.Assert(testGetter.counter, jc.GreaterThan, 10)
+ var testGetter batchingInstanceGetter
+ testGetter.aggregator = newAggregator(&testGetter)
+ testGetter.results = make([]instance.Instance, 100)
+ for i := range testGetter.results {
+ testGetter.results[i] = newTestInstance("foobar",
[]string{"127.0.0.1", "192.168.1.1"})
+ }
+ testGetter.batchSize = 10
+ testGetter.wg.Add(len(testGetter.results))
+ testGetter.startRequest()
+ testGetter.wg.Wait()
+ c.Assert(testGetter.counter, gc.Equals,
int32(len(testGetter.results)/testGetter.batchSize)+1)
  }

  func (s *aggregateSuite) TestError(c *gc.C) {