state: some watcher testing consistency

state: some watcher testing consistency

Not great -- especially the duplication in watcher_test.go -- but rather
better than what was there before, I think.

And I think I've killed all the crazy-table-style watcher tests now, so
that's a bonus.

https://code.launchpad.net/~fwereade/juju-core/state-watcher-testing-consistency/+merge/170005

(do not edit description out of merge proposal)

[fwereade, 2013-06-18 07:20:53]

Please take a look.

[mue, 2013-06-18 07:43:52]

LGTM, great CL, makes much of the code clearer.

https://codereview.appspot.com/10251047/diff/1/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/10251047/diff/1/state/machine_test.go#newcode505
state/machine_test.go:505: var watchMachineTests = []func(m *state.Machine)
error{
Seems not to be used anymore.

https://codereview.appspot.com/10251047/diff/1/state/machine_test.go#newcode517
state/machine_test.go:517: func (s *MachineSuite) TestWatchMachine(c *C) {
Test is now clearer than before. Sometimes better than table driven tests.

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go#newcode32
state/watcher_test.go:32: type NotifyWatcherC struct {
Nice helper, makes a lot cleaner.

[thumper, 2013-06-18 23:19:58]

LGTM - just a few comments.

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go#newcode32
state/watcher_test.go:32: type NotifyWatcherC struct {
Not sure what I think of the name.

What about NotifyWatcherHelper?  The "C" is weird.

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go#newcode39
state/watcher_test.go:39: c.state.StartSync()
why is this one StartSync, and the one below Sync?

[fwereade, 2013-06-19 09:59:51]

Please take a look.

https://codereview.appspot.com/10251047/diff/1/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/10251047/diff/1/state/machine_test.go#newcode505
state/machine_test.go:505: var watchMachineTests = []func(m *state.Machine)
error{
On 2013/06/18 07:43:52, mue wrote:
> Seems not to be used anymore.

Done.

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go#newcode32
state/watcher_test.go:32: type NotifyWatcherC struct {
On 2013/06/18 23:19:59, thumper wrote:
> Not sure what I think of the name.
> 
> What about NotifyWatcherHelper?  The "C" is weird.

The "C" echoes with the gocheck.C it embeds; no comment re global weirdness, but
I think it's appropriate in context. Comments tweaked.

https://codereview.appspot.com/10251047/diff/1/state/watcher_test.go#newcode39
state/watcher_test.go:39: c.state.StartSync()
On 2013/06/18 23:19:59, thumper wrote:
> why is this one StartSync, and the one below Sync?

This is StartSync because we're not really expecting an event, and we can't be
bothered to wait for the backend to churn through everything and deliver the
absolute latest updates.

The Sync below is because we *do* generally want multiple backend events to have
been handled and coalesced; by waiting until they're all delivered to the
watcher we're testing, we ensure that by the time the watcher's ready to send
its own event it will have seen and incorporated [0] all necessary input.

[0] certain watchers themselves collate events from multiple watchers, and so
this guarantee doesn't actually apply; an intransigent scheduler could thus
cause this to go wrong. If and when this becomes a problem in practice, it'd be
possible to write an assert that collects events until the set of them
definitively matches, or does not match, expectations; but I'm not doing this
just yet.