Issue 11439043: various: Use Long/ShortWait

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 10 months ago by jameinel

Modified:
11 years, 10 months ago

Reviewers:
mp+175245, dimitern, rog

Visibility:
Public.

Description

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. https://code.launchpad.net/~jameinel/juju-core/long-timeouts/+merge/175245 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 11

Created: 11 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+140 lines, -108 lines)			Patch
A	[revision details]	View	1 chunk	+2 lines, -0 lines	0 comments	Download
M	cmd/juju/publish_test.go	View	1 chunk	+1 line, -0 lines	1 comment	Download
M	cmd/juju/status_test.go	View	5 chunks	+6 lines, -5 lines	0 comments	Download
M	cmd/jujud/deploy_test.go	View	3 chunks	+9 lines, -6 lines	0 comments	Download
M	cmd/jujud/unit_test.go	View	1 chunk	+1 line, -1 line	0 comments	Download
M	cmd/jujud/upgrade_test.go	View	5 chunks	+5 lines, -4 lines	0 comments	Download
M	downloader/downloader_test.go	View	1 chunk	+1 line, -1 line	0 comments	Download
M	state/api/watcher/watcher_test.go	View	2 chunks	+1 line, -5 lines	0 comments	Download
M	state/machine_test.go	View	3 chunks	+3 lines, -4 lines	0 comments	Download
M	state/presence/presence_test.go	View	6 chunks	+11 lines, -6 lines	3 comments	Download
M	state/relationunit_test.go	View	3 chunks	+3 lines, -2 lines	0 comments	Download
M	state/state_test.go	View	3 chunks	+3 lines, -3 lines	0 comments	Download
M	state/unit_test.go	View	3 chunks	+3 lines, -4 lines	0 comments	Download
M	state/watcher/watcher_test.go	View	2 chunks	+2 lines, -1 line	0 comments	Download
M	store/mgo_test.go	View	1 chunk	+3 lines, -2 lines	0 comments	Download
M	testing/constants.go	View	1 chunk	+1 line, -1 line	1 comment	Download
M	utils/fslock/fslock_test.go	View	2 chunks	+2 lines, -2 lines	0 comments	Download
M	worker/cleaner/cleaner_test.go	View	2 chunks	+6 lines, -4 lines	0 comments	Download
M	worker/deployer/deployer_test.go	View	1 chunk	+2 lines, -2 lines	0 comments	Download
M	worker/firewaller/firewaller_test.go	View	4 chunks	+10 lines, -5 lines	2 comments	Download
M	worker/provisioner/lxc-broker_test.go	View	4 chunks	+5 lines, -5 lines	0 comments	Download
M	worker/provisioner/provisioner_test.go	View	6 chunks	+9 lines, -6 lines	2 comments	Download
M	worker/uniter/charm/charm_test.go	View	2 chunks	+8 lines, -6 lines	0 comments	Download
M	worker/uniter/jujuc/server_test.go	View	2 chunks	+8 lines, -6 lines	0 comments	Download
M	worker/uniter/relation/hookqueue_test.go	View	4 chunks	+9 lines, -7 lines	1 comment	Download
M	worker/uniter/relationer_test.go	View	6 chunks	+17 lines, -13 lines	0 comments	Download
M	worker/uniter/uniter_test.go	View	8 chunks	+9 lines, -7 lines	1 comment	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

dimitern

LGTM apart from some minor grumble about a few comments. https://codereview.appspot.com/11439043/diff/1/cmd/juju/publish_test.go File cmd/juju/publish_test.go (right): https://codereview.appspot.com/11439043/diff/1/cmd/juju/publish_test.go#newcode56 ...

11 years, 10 months ago (2013-07-17 10:55:52 UTC) #2

rog

11 years, 10 months ago (2013-07-18 15:56:29 UTC) #3

LGTM with some changes below.
i'll take this forward and land it.

https://codereview.appspot.com/11439043/diff/1/state/presence/presence_test.go
File state/presence/presence_test.go (right):

https://codereview.appspot.com/11439043/diff/1/state/presence/presence_test.g...
state/presence/presence_test.go:342: //            since we always sleep for the
full timeout
On 2013/07/17 10:55:52, dimitern wrote:
> I don't think the leading whitespace contributes much here.

+1

https://codereview.appspot.com/11439043/diff/1/testing/constants.go
File testing/constants.go (right):

https://codereview.appspot.com/11439043/diff/1/testing/constants.go#newcode20
testing/constants.go:20: const LongWait = 1 * time.Second
i'd go for at least 5 seconds here.

https://codereview.appspot.com/11439043/diff/1/worker/provisioner/provisioner...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/11439043/diff/1/worker/provisioner/provisioner...
worker/provisioner/provisioner_test.go:179: case
<-time.After(coretesting.LongWait):
ShortWait

https://codereview.appspot.com/11439043/diff/1/worker/uniter/relation/hookque...
File worker/uniter/relation/hookqueue_test.go (right):

https://codereview.appspot.com/11439043/diff/1/worker/uniter/relation/hookque...
worker/uniter/relation/hookqueue_test.go:286: case
<-time.After(coretesting.LongWait):
ShortWait

Expand All Messages | Collapse All Messages