Code review - Issue 6482081: environs/dummy: add delayhttps://codereview.appspot.com/2012-08-29T23:26:39+00:00rietveld
Message from unknown
2012-08-28T01:59:58+00:00dfcurn:md5:8125309097bfbff20400ed99a300c63e
Message from dave@cheney.net
2012-08-28T02:00:02+00:00dfcurn:md5:18353d508c0b4924f8efad643cc2fd69
Please take a look.
Message from unknown
2012-08-28T02:11:51+00:00dfcurn:md5:79cce20fc81bc9e49cb4a4d1b4fd1f36
Message from dave@cheney.net
2012-08-28T02:11:53+00:00dfcurn:md5:6c1a47e5c6d70bc27163a0c6b3ac2610
Please take a look.
Message from n13m3y3r@gmail.com
2012-08-28T13:11:56+00:00niemeyerurn:md5:4e562b4ad8893cdc883e6c078beaa465
http://codereview.appspot.com/6482081/diff/3001/environs/dummy/environs.go
File environs/dummy/environs.go (right):
http://codereview.appspot.com/6482081/diff/3001/environs/dummy/environs.go#newcode120
environs/dummy/environs.go:120: <-time.After(e.delayTime)
time.Sleep(e.delayTime)
http://codereview.appspot.com/6482081/diff/3001/environs/dummy/environs.go#newcode162
environs/dummy/environs.go:162: providerDelay, _ = time.ParseDuration(os.Getenv("JUJU_DUMMY_DELAY"))
I'd prefer to stay out of environment variables for that kind of purpose, as hidden state affects things in a way that is not obvious. What about having a -dummy.delay flag that is added by a test suite, and causes a dummy.DelayOps(duration) function in here to be called?
Message from dave@cheney.net
2012-08-29T04:48:12+00:00dfcurn:md5:192a30843a8c8080a4c45f0ed790c386
As discussed on IRC I think this is unavoidable in this case as a test flag only works local to the package being tested. In this case we need to test things that consume dummy.
Message from unknown
2012-08-29T04:49:09+00:00dfcurn:md5:aacaa123807f00d3a95f3c4ca1792ecc
Message from dave@cheney.net
2012-08-29T04:49:11+00:00dfcurn:md5:24eff0dad2f24a31480955cfab3e0371
Please take a look.
Message from unknown
2012-08-29T04:56:36+00:00dfcurn:md5:313fbe1fe94f54714664180e19515f29
Message from dave@cheney.net
2012-08-29T04:56:38+00:00dfcurn:md5:bad81239d55681f2f5523c2f03150a7b
Please take a look.
Message from n13m3y3r@gmail.com
2012-08-29T13:32:48+00:00niemeyerurn:md5:e679a0ec2e84eb85629362117422226c
LGTM with the unused variable dropped.
https://codereview.appspot.com/6482081/diff/9001/environs/dummy/environs.go
File environs/dummy/environs.go (right):
https://codereview.appspot.com/6482081/diff/9001/environs/dummy/environs.go#newcode113
environs/dummy/environs.go:113: delayTime time.Duration // delay before actioning request
This variable is unused.
https://codereview.appspot.com/6482081/diff/9001/environs/dummy/environs.go#newcode597
environs/dummy/environs.go:597: // pause execution to simulate the latency of a real provider
I'd pull down the variable and init function excerpt down here instead. One of the great benefits of Go's parsing model is being able to keep things in context in these cases, keeping logic clean from unrelated matters.
Message from unknown
2012-08-29T23:14:09+00:00dfcurn:md5:ef44efa8f04a909e36c2979e3f4f5be5
Message from dave@cheney.net
2012-08-29T23:14:11+00:00dfcurn:md5:326fb139e8a11e00e022b83e0f4bff06
Please take a look.
https://codereview.appspot.com/6482081/diff/9001/environs/dummy/environs.go
File environs/dummy/environs.go (right):
https://codereview.appspot.com/6482081/diff/9001/environs/dummy/environs.go#newcode113
environs/dummy/environs.go:113: delayTime time.Duration // delay before actioning request
On 2012/08/29 13:32:48, niemeyer wrote:
> This variable is unused.
Done.
https://codereview.appspot.com/6482081/diff/9001/environs/dummy/environs.go#newcode597
environs/dummy/environs.go:597: // pause execution to simulate the latency of a real provider
On 2012/08/29 13:32:48, niemeyer wrote:
> I'd pull down the variable and init function excerpt down here instead. One of
> the great benefits of Go's parsing model is being able to keep things in context
> in these cases, keeping logic clean from unrelated matters.
Done.
Message from unknown
2012-08-29T23:24:54+00:00dfcurn:md5:509def7f8938a863ffc5e0b160c7217c
Message from dave@cheney.net
2012-08-29T23:26:39+00:00dfcurn:md5:dda1938c4249438a56996d9815006a21
*** Submitted:
environs/dummy: add delay
This proposal implements a delay in most dummy operations. The delay
can be set test wide by setting JUJU_DUMMY_DELAY to a time.Duration
parsable value.
R=niemeyer
CC=
https://codereview.appspot.com/6482081