uniter: fix flaky tests, poor isolation

uniter: fix flaky tests, poor isolation

The changed test case wasn't waiting for the right thing, because there
wasn't anything clear to wait for; changed the uniter deploy code such
that it only notifies remote state about a new charm *after* it's downloaded
and verified it but still before the change is applied. I can then write the
test such that I immediately request the uniter stop just after it starts
installing the new charm -- a procedure it won't interrupt -- and thus delay
the verifyCharm check until the charm is sure to have been deployed.

This change has been verified by rogpeppe and frankban, both of whom saw the
original failures and now no longer do.

I also fixed up test isolation via Reset, which was kinda crackful before.

https://code.launchpad.net/~fwereade/juju-core/fix-1157898/+merge/154765

(do not edit description out of merge proposal)

[fwereade, 2013-03-21 17:30:53]

Please take a look.

[jameinel, 2013-03-21 17:50:04]

LGTM, though from your phrasing it sounds like we might still have a race.

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:581: // the charm state on disk is as we expect.
From your coverletter, it sounded like we might still have a race condition (if
the charm installs faster than we can stop the uniter). Is this true? Should we
note it, but note that the chance of the charm installing faster than stopping
is miniscule?
Or is this just stepping the uniter, so it won't go to the next step?

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:833: }()
You removed the os.RemoveAll() and coretesting.Server.Flush() from the cleanup
steps.

I'm assuming you're doing this in a defer to ensure it always runs even on a
panic() from ctx.run?
That seems fine. Is there a reason to remove the other cleanups?

[rog, 2013-03-22 08:13:28]

LGTM, nice to see this fixed.

https://codereview.appspot.com/7950043/diff/1/worker/uniter/jujuc/server.go
File worker/uniter/jujuc/server.go (right):

https://codereview.appspot.com/7950043/diff/1/worker/uniter/jujuc/server.go#n...
worker/uniter/jujuc/server.go:102: log.Infof("worker/uniter/jujuc: running hook
tool %q %q", req.CommandName, req.Args)
yes!

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:81: coretesting.Server.Flush()
shouldn't this be in TearDownTest too? or is it implicit in
JujuConnSuite.TearDownTest?

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:833: }()
On 2013/03/21 17:50:04, jameinel wrote:
> You removed the os.RemoveAll() and coretesting.Server.Flush() from the cleanup
> steps.

they've moved into s.Reset.

[fwereade, 2013-03-22 09:53:42]

*** Submitted:

uniter: fix flaky tests, poor isolation

The changed test case wasn't waiting for the right thing, because there
wasn't anything clear to wait for; changed the uniter deploy code such
that it only notifies remote state about a new charm *after* it's downloaded
and verified it but still before the change is applied. I can then write the
test such that I immediately request the uniter stop just after it starts
installing the new charm -- a procedure it won't interrupt -- and thus delay
the verifyCharm check until the charm is sure to have been deployed.

This change has been verified by rogpeppe and frankban, both of whom saw the
original failures and now no longer do.

I also fixed up test isolation via Reset, which was kinda crackful before.

R=jameinel, rog
CC=
https://codereview.appspot.com/7950043

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go
File worker/uniter/uniter_test.go (right):

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:81: coretesting.Server.Flush()
On 2013/03/22 08:13:29, rog wrote:
> shouldn't this be in TearDownTest too? or is it implicit in
> JujuConnSuite.TearDownTest?

I think it should be, thanks.

https://codereview.appspot.com/7950043/diff/1/worker/uniter/uniter_test.go#ne...
worker/uniter/uniter_test.go:581: // the charm state on disk is as we expect.
On 2013/03/21 17:50:04, jameinel wrote:
> From your coverletter, it sounded like we might still have a race condition
(if
> the charm installs faster than we can stop the uniter). Is this true?

Doesn't matter how fast we install. The problem was purely in checking we had
done so before we were guaranteed to have done so; being early was wrong, but we
can be as late as we like and it's still not a race because we're checking for a
state that persists until next perturbed (eg by the following resolveError).