worker/singular: mongo sanity-check tests

worker/singular: mongo sanity-check tests

All the previous tests of this package test that
the logic works as written, but nothing checks
that the approach will actually work when integrated
with mongo.

These tests actually start a replica set and
mimic some of the structure that we intend
to use in a full juju installation.

By their nature, the tests are slow (~ 2 minutes
to run) and unreliable (mongo occasionally core
dumps), so I have disabled them by default.
They can be enabled with the -juju.unreliabletests flag.

https://code.launchpad.net/~rogpeppe/juju-core/538-worker-singular-mongo-test/+merge/214189

(do not edit description out of merge proposal)

[rog, 2014-04-04 09:44:08]

Please take a look.

[dimitern, 2014-04-04 10:43:17]

Looks sane, here are some suggestions/questions.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test.go
File worker/singular/mongo_test.go (right):

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:1: package singular_test
Copyright header.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:12: gc "launchpad.net/gocheck"
Move gocheck in the group above.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:62: time.Sleep(1 * time.Minute)
How did you come up with this specific delay?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:64: // Try to hoose a different agent than the
primary to
hoose?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:66: nextId :=
((globalState.activeId+1)-1)%len(insts) + 1
Why not just:

nextId := globalState.activeId%len(insts)+1

?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:196: case <-time.After(250 * time.Millisecond):
Use testing.ShortWait instead?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:306: if isConnected {
This block gets a bit too nested for my taste - perhaps factor it out in (a)
helper(s) ?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:340: func mkEvent(s string) event {
s/mkEvent/makeEvent/

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:348: func mkEvents(ss ...string) []event {
s/mkEvents/makeEvents/

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:386: c.Fatalf("timed out waiting for %q",
possible)
s/%q/%v/ (since it's a slice) ?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:388: panic("unreachable")
Do we still need this in Go 1.2 ?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:410: member.Priority = nil
So nil is the highest priority?

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:496: dialInfo.Timeout = 60 * time.Second
I'd define this and the other one (1*time.Minute) as package vars to have it in
one place.

[rog, 2014-04-04 11:54:55]

Please take a look.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test.go
File worker/singular/mongo_test.go (right):

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:1: package singular_test
On 2014/04/04 10:43:17, dimitern wrote:
> Copyright header.

Done.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:12: gc "launchpad.net/gocheck"
On 2014/04/04 10:43:17, dimitern wrote:
> Move gocheck in the group above.

Done.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:62: time.Sleep(1 * time.Minute)
On 2014/04/04 10:43:17, dimitern wrote:
> How did you come up with this specific delay?

it's entirely arbitrary. i don't know of a better way to wait for server
synchronisation.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:64: // Try to hoose a different agent than the
primary to
On 2014/04/04 10:43:17, dimitern wrote:
> hoose?

ha ha. "choose" done.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:66: nextId :=
((globalState.activeId+1)-1)%len(insts) + 1
On 2014/04/04 10:43:17, dimitern wrote:
> Why not just:
> 
> nextId := globalState.activeId%len(insts)+1
> 
> ?

because ids are 1-origin.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:196: case <-time.After(250 * time.Millisecond):
On 2014/04/04 10:43:17, dimitern wrote:
> Use testing.ShortWait instead?

I don't think that ShortWait isn't really
appropriate here - i think that would do it too frequently.

ShortWait is about waiting for nothing to happen - this
is about doing something at regular intervals.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:306: if isConnected {
On 2014/04/04 10:43:17, dimitern wrote:
> This block gets a bit too nested for my taste - perhaps factor it out in (a)
> helper(s) ?

I don't think factoring out pieces will help.
I started writing it as a switch statement, but
that's definitely worse. If you have a better idea
for how it might look, please suggest something.

This structure maps as closely as I can make it
to the actual invariant conditions.

I lost one level by checking for isConnected
early,  which hopefully helps a little.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:340: func mkEvent(s string) event {
On 2014/04/04 10:43:17, dimitern wrote:
> s/mkEvent/makeEvent/

it's no longer used anyway. deleted.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:348: func mkEvents(ss ...string) []event {
On 2014/04/04 10:43:17, dimitern wrote:
> s/mkEvents/makeEvents/

ditto.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:386: c.Fatalf("timed out waiting for %q",
possible)
On 2014/04/04 10:43:17, dimitern wrote:
> s/%q/%v/ (since it's a slice) ?

no, i want the quotes.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:388: panic("unreachable")
On 2014/04/04 10:43:17, dimitern wrote:
> Do we still need this in Go 1.2 ?

we do because Go doesn't know that Fatalf never returns.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:410: member.Priority = nil
On 2014/04/04 10:43:17, dimitern wrote:
> So nil is the highest priority?

nil is the default value for the priority, which
implies 1.

https://codereview.appspot.com/84470043/diff/40001/worker/singular/mongo_test...
worker/singular/mongo_test.go:496: dialInfo.Timeout = 60 * time.Second
On 2014/04/04 10:43:17, dimitern wrote:
> I'd define this and the other one (1*time.Minute) as package vars to have it
in
> one place.

they're actually unrelated. the other one is waiting for server synchronisation.
this one is waiting for the server to come
up initially.

[dimitern, 2014-04-04 12:25:33]

LGTM, thanks! I'd ask for a separate review from someone more familiar with
replicaset internals though.

[rog, 2014-04-04 12:36:46]

Please take a look.