state: more expressive testing proposal

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)

[fwereade, 2012-12-06 14:21:30]

Please take a look.

[niemeyer, 2012-12-06 14:43:17]

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?

[fwereade, 2012-12-06 15:02:36]

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.

[fwereade, 2012-12-06 15:38:02]

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.

[niemeyer, 2012-12-06 16:04:25]

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.

[fwereade, 2012-12-06 18:08:19]

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*.

[niemeyer, 2012-12-06 19:32:18]

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.

[niemeyer, 2012-12-12 18:07:02]

Is there a pending conversation to move this forward?

[fwereade, 2012-12-13 10:43:21]

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, n13m3y3r@gmail.com wrote:

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

[niemeyer, 2012-12-13 14:38:54]

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