Merge lp:~dave-cheney/juju-core/124-worker-peergrouper-mock-fix into lp:~go-bot/juju-core/trunk

[Dave Cheney]

mock_test server uses fixed order

Update lp 1299958

In a previous attempt to fix this issue it was pointed out that the order of the top level is important. https://codereview.appspot.com/82490043/#msg2

However the mock server does not respect any order as the fakeMachines are stored in a map.

This CL attempts to solve the problem by storing fake machines in a slice. However if you run this test repeatedly you will see the results are still not stable and in fact the test was relying on a small map to order its keys.

https://codereview.appspot.com/84430044/

[Dave Cheney (dave-cheney)]

Reviewers: mp+214170_code.launchpad.net,

Message:
Please take a look.

Description:
mock_test server uses fixed order

Update lp 1299958

In a previous attempt to fix this issue it was pointed out that the
order of the top level is important.
https://codereview.appspot.com/82490043/#msg2

However the mock server does not respect any order as the fakeMachines
are stored in a map.

This CL attempts to solve the problem by storing fake machines in a
slice. However if you run this test repeatedly you will see the results
are still not stable and in fact the test was relying on a small map to
order its keys.

https://code.launchpad.net/~dave-cheney/juju-core/124-worker-peergrouper-mock-fix/+merge/214170

(do not edit description out of merge proposal)

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

Affected files (+32, -12 lines):
   A [revision details]
   M worker/peergrouper/mock_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-20140404064032-dyb8bxnswgcrfxvm
+New revision: <email address hidden>

Index: worker/peergrouper/mock_test.go
=== modified file 'worker/peergrouper/mock_test.go'
--- worker/peergrouper/mock_test.go 2014-03-28 07:27:26 +0000
+++ worker/peergrouper/mock_test.go 2014-04-04 07:15:19 +0000
@@ -26,8 +26,11 @@
  // that we don't want to directly depend on in unit tests.

  type fakeState struct {
- mu sync.Mutex
- machines map[string]*fakeMachine
+ mu sync.Mutex
+ machines []struct {
+ id string
+ *fakeMachine
+ }
   stateServers voyeur.Value // of *state.StateServerInfo
   session *fakeMongoSession
   check func(st *fakeState) error
@@ -105,9 +108,7 @@
  }

  func newFakeState() *fakeState {
- st := &fakeState{
- machines: make(map[string]*fakeMachine),
- }
+ st := new(fakeState)
   st.session = newFakeMongoSession(st)
   st.stateServers.Set(&state.StateServerInfo{})
   return st
@@ -171,14 +172,25 @@
  func (st *fakeState) machine(id string) *fakeMachine {
   st.mu.Lock()
   defer st.mu.Unlock()
- return st.machines[id]
+ return st.machine0(id)
+}
+
+// machine0 is like machine but must only be called when
+// st.mu.Lock() is held.
+func (st *fakeState) machine0(id string) *fakeMachine {
+ for _, m := range st.machines {
+ if m.id == id {
+ return m.fakeMachine
+ }
+ }
+ return nil
  }

  func (st *fakeState) Machine(id string) (stateMachine, error) {
   if err := errorFor("State.Machine", id); err != nil {
    return nil, err
   }
- if m := st.machine(id); m != nil {
+ if m := st.machine0(id); m != nil {
    return m, nil
   }
   return nil, errors.NotFoundf("machine %s", id)
@@ -188,7 +200,7 @@
   st.mu.Lock()
   defer st.mu.Unlock()
   logger.Infof("fakeState.addMachine %q", id)
- if st.machines[id] != nil {
+ if st.machine0(id) != nil {
    panic(fmt.Errorf("id %q already used", id))
   }
   m := &fakeMachine{
@@ -198,7 +210,10 @@
     wantsVote: wantsVote,
    },
   }
- st.machines[id] = m
+ st.machines = append(st.machines, struct {
+ id string
+ *fakeMachine
+ }{id, m}...

[Dave Cheney (dave-cheney)]

ping

https://codereview.appspot.com/84430044/

[Tim Penhey (thumper)]

LGTM

While I'm not entirely happy with the method name machine0, I can't
think of a better one for this.

https://codereview.appspot.com/84430044/

[Dave Cheney (dave-cheney)]

You shouldn't LGTM this change. It doesn't fix the problem, but it does highlight that the behaviour the test expects is not provided by the code.

> On 7 Apr 2014, at 9:29, <email address hidden> wrote:
>
> LGTM
>
> While I'm not entirely happy with the method name machine0, I can't
> think of a better one for this.
>
> https://codereview.appspot.com/84430044/

[Roger Peppe (rogpeppe)]

I don't understand this CL. The order of machines (which is what it
seems this CL is trying to fix) doesn't matter, but
the order of addresses within a machine does. In the previous
review I made my remark because it was treating {{addr2 addr1} {addr3}}
as equivalent to {{addr1 addr3} {addr2}}, whereas it should
actually be treating it as equivalent to {{addr3} {{addr2 addr1}} only.

Perhaps this was needless pedantry because I *think* that
in the peergrouper tests we only ever assign one address
to any given machine. It's perhaps still worth getting right
though.

https://codereview.appspot.com/84430044/

Unmerged revisions

2566. By Dave Cheney on 2014-04-04

Mock test should order results

=== modified file 'worker/peergrouper/mock_test.go'
--- worker/peergrouper/mock_test.go	2014-03-28 07:27:26 +0000
+++ worker/peergrouper/mock_test.go	2014-04-04 07:21:06 +0000
@@ -26,8 +26,11 @@
 // that we don't want to directly depend on in unit tests.
 
 type fakeState struct {
-	mu           sync.Mutex
-	machines     map[string]*fakeMachine
+	mu       sync.Mutex
+	machines []struct {
+		id string
+		*fakeMachine
+	}
 	stateServers voyeur.Value // of *state.StateServerInfo
 	session      *fakeMongoSession
 	check        func(st *fakeState) error
@@ -105,9 +108,7 @@
 }
 
 func newFakeState() *fakeState {
-	st := &fakeState{
-		machines: make(map[string]*fakeMachine),
-	}
+	st := new(fakeState)
 	st.session = newFakeMongoSession(st)
 	st.stateServers.Set(&state.StateServerInfo{})
 	return st
@@ -171,14 +172,25 @@
 func (st *fakeState) machine(id string) *fakeMachine {
 	st.mu.Lock()
 	defer st.mu.Unlock()
-	return st.machines[id]
+	return st.machine0(id)
+}
+
+// machine0 is like machine but must only be called when
+// st.mu.Lock() is held.
+func (st *fakeState) machine0(id string) *fakeMachine {
+	for _, m := range st.machines {
+		if m.id == id {
+			return m.fakeMachine
+		}
+	}
+	return nil
 }
 
 func (st *fakeState) Machine(id string) (stateMachine, error) {
 	if err := errorFor("State.Machine", id); err != nil {
 		return nil, err
 	}
-	if m := st.machine(id); m != nil {
+	if m := st.machine0(id); m != nil {
 		return m, nil
 	}
 	return nil, errors.NotFoundf("machine %s", id)
@@ -188,7 +200,7 @@
 	st.mu.Lock()
 	defer st.mu.Unlock()
 	logger.Infof("fakeState.addMachine %q", id)
-	if st.machines[id] != nil {
+	if st.machine0(id) != nil {
 		panic(fmt.Errorf("id %q already used", id))
 	}
 	m := &fakeMachine{
@@ -198,7 +210,10 @@
 			wantsVote: wantsVote,
 		},
 	}
-	st.machines[id] = m
+	st.machines = append(st.machines, struct {
+		id string
+		*fakeMachine
+	}{id, m})
 	m.val.Set(m.doc)
 	return m
 }
@@ -206,10 +221,13 @@
 func (st *fakeState) removeMachine(id string) {
 	st.mu.Lock()
 	defer st.mu.Unlock()
-	if st.machines[id] == nil {
-		panic(fmt.Errorf("removing non-existent machine %q", id))
+	for i, m := range st.machines {
+		if m.id == id {
+			st.machines = append(st.machines[:i], st.machines[i+1:]...)
+			return
+		}
 	}
-	delete(st.machines, id)
+	panic(fmt.Errorf("removing non-existent machine %q", id))
 }
 
 func (st *fakeState) setStateServers(ids ...string) {