various: Use Long/ShortWait

various: Use Long/ShortWait

I tracked down all the references in our test suite to
time.Milliseconds and updated the references to use testing.ShortWait
and testing.LongWait where appropriate.

This also bumps the default LongWait timeout from 500ms to 1.0s.
The argument is:
 1) LongWait is used where it should never actually timeout
 2) We've encountered some reliability issues on go-bot, so lets bump
 this up a little bit and see if it gives us more security margin.

[Re-proposed to take this forward and land it - rog]

https://code.launchpad.net/~rogpeppe/juju-core/340-jameinel-long-timeouts/+merge/175621

(do not edit description out of merge proposal)

[rog, 2013-07-18 16:32:23]

Please take a look.

[dimitern, 2013-07-18 16:44:05]

LGTM, although some replacements seem a bit arbitrary: previously some places
now using ShortWait had 2x or 4x longer timeouts, in other the difference
between 200ms, 500ms and 10s (hence LongWait) is vast. Please reduce the
LongWait to 5s or expand why it needs to be so long.

https://codereview.appspot.com/11529043/diff/3001/state/presence/presence_tes...
File state/presence/presence_test.go (right):

https://codereview.appspot.com/11529043/diff/3001/state/presence/presence_tes...
state/presence/presence_test.go:410: case <-time.After(testing.ShortWait):
how come this is short and the one below...

https://codereview.appspot.com/11529043/diff/3001/state/presence/presence_tes...
state/presence/presence_test.go:477: case <-time.After(testing.LongWait):
...this one here is long? they're both 100ms

https://codereview.appspot.com/11529043/diff/3001/store/mgo_test.go
File store/mgo_test.go (right):

https://codereview.appspot.com/11529043/diff/3001/store/mgo_test.go#newcode74
store/mgo_test.go:74: time.Sleep(100 * time.Millisecond)
ShortWait then?

https://codereview.appspot.com/11529043/diff/3001/testing/constants.go
File testing/constants.go (right):

https://codereview.appspot.com/11529043/diff/3001/testing/constants.go#newcode20
testing/constants.go:20: const LongWait = 10 * time.Second
10s ?! I think that's way to lenient, make it 5 at most please.

https://codereview.appspot.com/11529043/diff/3001/worker/firewaller/firewalle...
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/11529043/diff/3001/worker/firewaller/firewalle...
worker/firewaller/firewaller_test.go:78: //            standard
coretesting.LongWait?
d whitespace please

https://codereview.appspot.com/11529043/diff/3001/worker/provisioner/provisio...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/11529043/diff/3001/worker/provisioner/provisio...
worker/provisioner/provisioner_test.go:179: case
<-time.After(coretesting.ShortWait):
why ShortWait?

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

https://codereview.appspot.com/11529043/diff/3001/worker/uniter/uniter_test.g...
worker/uniter/uniter_test.go:44: const worstCase = coretesting.LongWait
if this is the reason for LongWait to be 10s, please don't use it here, make
this a separate const.

[wallyworld, 2013-07-19 07:32:49]

LGTM. I'm ok with the timeouts being changed to be consistent. 
I agree with the question about the need for LongWait being 10s.

[rog, 2013-07-19 09:21:51]

Please take a look.

https://codereview.appspot.com/11529043/diff/3001/state/presence/presence_tes...
File state/presence/presence_test.go (right):

https://codereview.appspot.com/11529043/diff/3001/state/presence/presence_tes...
state/presence/presence_test.go:410: case <-time.After(testing.ShortWait):
On 2013/07/18 16:44:05, dimitern wrote:
> how come this is short and the one below...

good catch.

https://codereview.appspot.com/11529043/diff/3001/store/mgo_test.go
File store/mgo_test.go (right):

https://codereview.appspot.com/11529043/diff/3001/store/mgo_test.go#newcode74
store/mgo_test.go:74: time.Sleep(100 * time.Millisecond)
On 2013/07/18 16:44:05, dimitern wrote:
> ShortWait then?

Done (with a slight alteration to the logic)

https://codereview.appspot.com/11529043/diff/3001/testing/constants.go
File testing/constants.go (right):

https://codereview.appspot.com/11529043/diff/3001/testing/constants.go#newcode20
testing/constants.go:20: const LongWait = 10 * time.Second
On 2013/07/18 16:44:05, dimitern wrote:
> 10s ?! I think that's way to lenient, make it 5 at most please.

this is (or *should* be) only used in places
where it won't delay the usual (successful)
running of the test suite, as the comment says.

i don't think 10s is bad in that case, and it's
nice to have unified all the long delays.

https://codereview.appspot.com/11529043/diff/3001/worker/provisioner/provisio...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/11529043/diff/3001/worker/provisioner/provisio...
worker/provisioner/provisioner_test.go:179: case
<-time.After(coretesting.ShortWait):
On 2013/07/18 16:44:05, dimitern wrote:
> why ShortWait?

because this is the usual path - that's what we use ShortWait for.

[dimitern, 2013-07-19 09:31:52]

LGTM with a few suggestions. Nice to see some of the regular test paths now have
reduced timeouts due to using ShortWait everywhere.

https://codereview.appspot.com/11529043/diff/33002/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/state/machine_test.go#newc...
state/machine_test.go:285: err = s.machine.WaitAgentAlive(coretesting.LongWait)
Are you sure this shouldn't be ShortWait as well?

https://codereview.appspot.com/11529043/diff/33002/state/unit_test.go
File state/unit_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/state/unit_test.go#newcode653
state/unit_test.go:653: err = s.unit.WaitAgentAlive(coretesting.LongWait)
Here as well? (ShortWait)

https://codereview.appspot.com/11529043/diff/33002/worker/cleaner/cleaner_tes...
File worker/cleaner/cleaner_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/worker/cleaner/cleaner_tes...
worker/cleaner/cleaner_test.go:64: case <-timeout:
case <-time.After(coretesting.LongWait):
instead (and get rid of timeout) ?

https://codereview.appspot.com/11529043/diff/33002/worker/deployer/deployer_t...
File worker/deployer/deployer_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/worker/deployer/deployer_t...
worker/deployer/deployer_test.go:232: case <-timeout:
ditto

https://codereview.appspot.com/11529043/diff/33002/worker/provisioner/provisi...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:230: case <-timeout:
ditto

https://codereview.appspot.com/11529043/diff/33002/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:250: case <-timeout:
ditto

[rog, 2013-07-19 09:37:36]

https://codereview.appspot.com/11529043/diff/33002/state/machine_test.go
File state/machine_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/state/machine_test.go#newc...
state/machine_test.go:285: err = s.machine.WaitAgentAlive(coretesting.LongWait)
On 2013/07/19 09:31:52, dimitern wrote:
> Are you sure this shouldn't be ShortWait as well?

no - in this case we expect the operation to succeed, so the LongWait delay
should never happen.

https://codereview.appspot.com/11529043/diff/33002/worker/cleaner/cleaner_tes...
File worker/cleaner/cleaner_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/worker/cleaner/cleaner_tes...
worker/cleaner/cleaner_test.go:64: case <-timeout:
On 2013/07/19 09:31:52, dimitern wrote:
> case <-time.After(coretesting.LongWait):
> instead (and get rid of timeout) ?

no - we want to receive all the changes within the timeout; hence we want a
timeout for the entire duration of the loop, not just one iteration.

anyway, this CL isn't about refactoring test logic, just unifying timeouts.

https://codereview.appspot.com/11529043/diff/33002/worker/deployer/deployer_t...
File worker/deployer/deployer_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/worker/deployer/deployer_t...
worker/deployer/deployer_test.go:232: case <-timeout:
On 2013/07/19 09:31:52, dimitern wrote:
> ditto

definitely not - then it would never happen!

https://codereview.appspot.com/11529043/diff/33002/worker/provisioner/provisi...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/11529043/diff/33002/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:230: case <-timeout:
On 2013/07/19 09:31:52, dimitern wrote:
> ditto

again, we want the timeout for the entire duration of the loop.

https://codereview.appspot.com/11529043/diff/33002/worker/provisioner/provisi...
worker/provisioner/provisioner_test.go:250: case <-timeout:
On 2013/07/19 09:31:52, dimitern wrote:
> ditto

ditto