Code review comment for lp://staging/~fwereade/juju-core/strawman-state-test-simplification

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/