Merge lp://staging/~fwereade/juju-core/strawman-state-test-simplification into lp://staging/~juju/juju-core/trunk

Reviewers: mp+138465_code.launchpad.net,

Message:
Please take a look.

Description:
state: more expressive testing proposal

This shouldn't go in on its own -- I should fix up the rest of the state
tests before I merge it (and I don't want to fix machine_test until
we've
dropped the orgiginal WatchPrincipalUnits implementation) but I think
that
AssignSuite was a good enough test bed to evaluate the sanity of this
approach.

In particular, I found that even a few lines saved here and there made
it
much easier to follow the tests, and to notice both redundant tests and
missing ones; and that in some cases a *lot* of lines were saved.

I'm not sure AssignSuite uses all the new ConnSuite features yet (they
were
originally written against machine_test, but then I got demoralised) but
I'm
pretty sure that they will be useful; if not, of course, I'll trash
them.

https://code.launchpad.net/~fwereade/juju-core/strawman-state-test-simplification/+merge/138465

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M state/assign_test.go
   M state/conn_test.go
   M state/life_test.go
   M state/unit.go

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode99
state/conn_test.go:99: func (cs *ConnSuite) AddService(c *C, charmName
string) *state.Service {
I can easily see tests being copy & pasted and changing their meaning
wildly and silently depending on where the logic is pasted.

This should probably be AddService(c, charmName, serviceName), plus fail
the test if service name exists.

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode122
state/conn_test.go:122: cs.RemoveUnit(c, unit)
How often is that done/necessary in a test?

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode154
state/conn_test.go:154: err := unit.UnassignFromMachine()
Why do we have to unassign the principal? We should be able to remove a
unit without every unassigning it, right?

https://codereview.appspot.com/6862053/

Good comments, thanks.

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode99
state/conn_test.go:99: func (cs *ConnSuite) AddService(c *C, charmName
string) *state.Service {
On 2012/12/06 14:43:17, niemeyer wrote:
> I can easily see tests being copy & pasted and changing their meaning
wildly and
> silently depending on where the logic is pasted.

> This should probably be AddService(c, charmName, serviceName), plus
fail the
> test if service name exists.

Heh, I definitely should have explained this idea. The advantage here is
that we can write large executable-table-based for (eg) machine unit
watching, each of which uses independent machines/services/units etc,
*without* having to setup/teardown a complete State every time.

The disadvantage is, of course, that if you make use of this feature you
lose the ability to make assumptions about your service/unit names...
but then I've always felt a little bit uncomfortable about tests that do
that in the first place.

Does it seem any saner now?

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode122
state/conn_test.go:122: cs.RemoveUnit(c, unit)
On 2012/12/06 14:43:17, niemeyer wrote:
> How often is that done/necessary in a test?

Often enough that it was considered worth factoring out into
removeAllUnits :). (Not *that* many times, though: 4 in total, one of
which was replaced here.)

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode154
state/conn_test.go:154: err := unit.UnassignFromMachine()
On 2012/12/06 14:43:17, niemeyer wrote:
> Why do we have to unassign the principal? We should be able to remove
a unit
> without every unassigning it, right?

Good point. On closer inspection, this is actually redundant, because
RemoveUnit calls UnassignFromMachine directly, but I'm not sure the
existing implementation is quite sane, because it's not done in a single
transaction: the *intent* is pretty clearly that a unit should only be
removed when not assigned, but it's not guaranteed.

So, yeah, I should definitely change this, and add a TODO BUG to
RemoveUnit to ensure that machines can't end up with missing units
assigned to them.

https://codereview.appspot.com/6862053/

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode99
state/conn_test.go:99: func (cs *ConnSuite) AddService(c *C, charmName
string) *state.Service {
On 2012/12/06 15:02:36, fwereade wrote:
> Does it seem any saner now?

A thought: AddService should be as you suggest, but what I have there
should be... er... AddAnonymousService, perhaps? That's not quite the
right word but it has roughly the right connotations, I think.

https://codereview.appspot.com/6862053/

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode99
state/conn_test.go:99: func (cs *ConnSuite) AddService(c *C, charmName
string) *state.Service {
On 2012/12/06 15:38:02, fwereade wrote:
> On 2012/12/06 15:02:36, fwereade wrote:
> > Does it seem any saner now?

> A thought: AddService should be as you suggest, but what I have there
should
> be... er... AddAnonymousService, perhaps? That's not quite the right
word but it
> has roughly the right connotations, I think.

This sounds like a workaround for a problem that shouldn't exist. The
kind of test you describe, which is a never-ending rolling state, should
be extinguished IMO. We wrongly call them "tables", but really they are
an anti-pattern that diverge significantly from what table tests as
originally envisioned pursue: a succinct, isolated, and clear test case.

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode122
state/conn_test.go:122: cs.RemoveUnit(c, unit)
On 2012/12/06 15:02:36, fwereade wrote:
> On 2012/12/06 14:43:17, niemeyer wrote:
> > How often is that done/necessary in a test?

> Often enough that it was considered worth factoring out into
removeAllUnits :).
> (Not *that* many times, though: 4 in total, one of which was replaced
here.)

This sounds error prone. We have a method called RemoveService in state,
and this is doing a *ton* more than that method. If this is indeed
what's most useful in test, let's call it something like
ObliterateService then.

https://codereview.appspot.com/6862053/

quick comments; mileage may vary

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode99
state/conn_test.go:99: func (cs *ConnSuite) AddService(c *C, charmName
string) *state.Service {
On 2012/12/06 16:04:25, niemeyer wrote:
> On 2012/12/06 15:38:02, fwereade wrote:
> > On 2012/12/06 15:02:36, fwereade wrote:
> > > Does it seem any saner now?
> >
> > A thought: AddService should be as you suggest, but what I have
there should
> > be... er... AddAnonymousService, perhaps? That's not quite the right
word but
> it
> > has roughly the right connotations, I think.

> This sounds like a workaround for a problem that shouldn't exist. The
kind of
> test you describe, which is a never-ending rolling state, should be
extinguished
> IMO. We wrongly call them "tables", but really they are an
anti-pattern that
> diverge significantly from what table tests as originally envisioned
pursue: a
> succinct, isolated, and clear test case.

Isn't that throwing the baby out with the bathwater? I absolutely agree
that machine_test.go has some abominations, but the problem is surely
that the tests are affecting one another... AddAnonymousService gives us
a way to write (at least some) non-polluting tests against the same
State (without paying the ~100ms setup/teardown cost).

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode122
state/conn_test.go:122: cs.RemoveUnit(c, unit)
On 2012/12/06 16:04:25, niemeyer wrote:
> On 2012/12/06 15:02:36, fwereade wrote:
> > On 2012/12/06 14:43:17, niemeyer wrote:
> > > How often is that done/necessary in a test?
> >
> > Often enough that it was considered worth factoring out into
removeAllUnits
> :).
> > (Not *that* many times, though: 4 in total, one of which was
replaced here.)

> This sounds error prone. We have a method called RemoveService in
state, and
> this is doing a *ton* more than that method. If this is indeed what's
most
> useful in test, let's call it something like ObliterateService then.

In things like the machine unit watch tests, if we don't have something
like AddAnonymousService, we'll either be deferring this method *all the
time* (so we get something a bit more like an empty state for each
test)... or writing a whole load of expensive individual test cases.

I'm not quite sure the same-method-name argument holds water... none of
these methods do precisely what the state methods of the same name do.
However, I think they do express the intent of the test pretty clearly
-- when I call `s.RemoveService(c, foo)` I'm stating that I want that
service *gone*.

https://codereview.appspot.com/6862053/

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go
File state/conn_test.go (right):

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode99
state/conn_test.go:99: func (cs *ConnSuite) AddService(c *C, charmName
string) *state.Service {
On 2012/12/06 18:08:19, fwereade wrote:
> Isn't that throwing the baby out with the bathwater? I

Kind of.. I'd call it "throwing the alligator with the bathwater", which
sounds more positive. :-)

> machine_test.go has some abominations, but the problem is surely that
the tests
> are affecting one another... AddAnonymousService gives us a way to
write (at
> least some) non-polluting tests against the same State (without paying
the
> ~100ms setup/teardown cost).

How to write non-polluting tests when they are all dependent on the same
state? If the state is completely independent from one another, the test
can reset the state entirely before running the next entry in the list,
even without running SetUp/TearDown (which actually doesn't that big of
a deal on itself). This is not the case with our tests.. the state is
shared across all tests, and depended upon.

https://codereview.appspot.com/6862053/diff/1/state/conn_test.go#newcode122
state/conn_test.go:122: cs.RemoveUnit(c, unit)
On 2012/12/06 18:08:19, fwereade wrote:
> In things like the machine unit watch tests, if we don't have
something like
> AddAnonymousService, we'll either be deferring this method *all the
time* (so we
> get something a bit more like an empty state for each test)... or
writing a
> whole load of expensive individual test cases.

That seems to be a key reason why we have SetUp and TearDown, and test
suites that reset the state.

> I'm not quite sure the same-method-name argument holds water... none
of these
> methods do precisely what the state methods of the same name do.

Pretty much all of the other entries do exactly that, with trivial
differences. The only other case is perhaps RemoveUnit, which should
probably follow suit and be called ObliterateUnit if it is preserved as
it is.

> However, I
> think they do express the intent of the test pretty clearly -- when I
call
> `s.RemoveService(c, foo)` I'm stating that I want that service *gone*.

There is State.RemoveService, and there is RemoveService in the state
tests. They should be similar, or be named differently. It seems bad to
have s.RemoveService and state.RemoveService *vastly* different, and
trivial to avoid that by naming them differently.

I must be missing something about the way you're thinking this through.

https://codereview.appspot.com/6862053/

Is there a pending conversation to move this forward?

https://codereview.appspot.com/6862053/

No -I'm comfortable with your suggestions and will be picking it up again and working on it in parallel with the long stream of AddUnitSubordinateTo fixes.

Sent from my OMG-I'm-living-in-the-future toy

On 12 Dec 2012, at 19:07, <email address hidden> wrote:

> Is there a pending conversation to move this forward?
>
> https://codereview.appspot.com/6862053/

Sounds good, thanks. I'll mark it as WIP then.

https://codereview.appspot.com/6862053/