CI testing support/improvements

CI testing support/improvements

This branch includes the changes to support CI testing. At the time 
of this writing there is still one IE/Selenium interaction issue that
would fail the deploy test. That is conditionally skipped and must
be revisited in the future.

https://code.launchpad.net/~bcsaller/juju-gui/remote-testing/+merge/153441

(do not edit description out of merge proposal)

[bcsaller, 2013-03-14 18:53:05]

Please take a look.

[gary.poster, 2013-03-14 19:19:18]

Very nice.  Thank you very much for doing this.

I have a few mostly trivial comments.  I'll try this out before I complete the
review.

https://codereview.appspot.com/7816043/diff/1/bin/test-charm
File bin/test-charm (right):

https://codereview.appspot.com/7816043/diff/1/bin/test-charm#newcode24
bin/test-charm:24: if [ ${FAIL_FAST:-0} -eq 1 -a ${EXIT_CODE} -eq 1 ]; then
It would be great to document the FAIL_FAST option somewhere.  Maybe you have,
and I haven't seen it yet.  Otherwise, minimally putting it at the top of this
file and in Jeff's docs seem like a good idea.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py
File lib/deploy_charm_for_testing.py (right):

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:15: DEFAULT_BRANCH = 'lp:juju-gui'
This is actually the DEFAULT_SOURCE, from the perspective of the charm.  We can
supply anything here that the charm allows in juju-gui-source, and we may want
to do that so that we can test a release with the charm, for instance.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:70: parser = argparse.ArgumentParser(
cool.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:72: parser.add_argument('--branch',
default=DEFAULT_BRANCH)
--source?  --gui-source?  --gui?

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:96: wait_for_machine()
Cool change to handle the IP.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:98: instance_ip =
os.environ.get("JUJU_INSTANCE_IP")
Oh cool.  So if you don't provide this variable, everything works fine on ec2,
yeah?

I want to verify that Jeff's docs describe this.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:102: "euca-describe-instances | grep INSTANCE |
grep juju-juju-gui-testing-instance-1 | awk '{print $2;}'", shell=True).strip()
IWBNI this were < 80 chars.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:104: "euca-describe-instances | grep INSTANCE |
grep juju-juju-gui-testing-instance-1 | awk '{print $12;}'", shell=True).strip()
IWBNI this were < 80 chars.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:108:
subprocess.check_call("euca-associate-address -i %s %s" % (instance_id,
instance_ip), shell=True)
IWBNI this were < 80 chars.

https://codereview.appspot.com/7816043/diff/1/test/browser.py
File test/browser.py (right):

https://codereview.appspot.com/7816043/diff/1/test/browser.py#newcode26
test/browser.py:26: 'command-timeout' : 300,
Heh, ow.

https://codereview.appspot.com/7816043/diff/1/test/browser.py#newcode27
test/browser.py:27: 'idle-timeout': 100,
ow x 2.

https://codereview.appspot.com/7816043/diff/1/test/browser.py#newcode141
test/browser.py:141: @retry(WebDriverException,
format_error=formatWebDriverError)
Why?  Maybe worth a comment.

https://codereview.appspot.com/7816043/diff/1/test/browser.py#newcode172
test/browser.py:172: @retry(subprocess.CalledProcessError)
why?

https://codereview.appspot.com/7816043/diff/1/test/browser.py#newcode186
test/browser.py:186: # firewalls and provider issues in some cases.
nice

https://codereview.appspot.com/7816043/diff/1/test/retry.py
File test/retry.py (right):

https://codereview.appspot.com/7816043/diff/1/test/retry.py#newcode8
test/retry.py:8:
http://www.saltycrane.com/blog/2009/11/trying-out-retry-decorator-python/
BSD licensed
(https://github.com/saltycrane/retry-decorator/blob/master/LICENSE).  Maybe we
should mention that?

[jeff.pihach, 2013-03-14 19:33:21]

LGTM This looks great thanks!- I'm glad a lot of the things we discussed ended
up working out! I don't have anything to add to the changes mentioned by Gary.
You might want another python dev to review. Thanks again!

[gary.poster, 2013-03-14 20:50:22]

LGTM, as incremental progress.  As we discussed elsewhere, there are still tests
failing, unfortunately (http://pastebin.ubuntu.com/5614608/).  I don't see why
we shouldn't land these changes now, though.

Small additional suggestion below.

Thanks again.

https://codereview.appspot.com/7816043/diff/1/bin/test-charm
File bin/test-charm (right):

https://codereview.appspot.com/7816043/diff/1/bin/test-charm#newcode6
bin/test-charm:6: python lib/deploy_charm_for_testing.py "$@"
We ought to also document the fact that you can pass the
deploy_charm_for_testing options to the bash script

[bcsaller, 2013-03-14 21:29:27]

Thanks for the reviews, even with the problems we are seeing I think this is a
good point to move forward from and will merge soon.

https://codereview.appspot.com/7816043/diff/1/bin/test-charm
File bin/test-charm (right):

https://codereview.appspot.com/7816043/diff/1/bin/test-charm#newcode6
bin/test-charm:6: python lib/deploy_charm_for_testing.py "$@"
On 2013/03/14 20:50:22, gary.poster wrote:
> We ought to also document the fact that you can pass the
> deploy_charm_for_testing options to the bash script

Done.

https://codereview.appspot.com/7816043/diff/1/bin/test-charm#newcode24
bin/test-charm:24: if [ ${FAIL_FAST:-0} -eq 1 -a ${EXIT_CODE} -eq 1 ]; then
On 2013/03/14 19:19:18, gary.poster wrote:
> It would be great to document the FAIL_FAST option somewhere.  Maybe you have,
> and I haven't seen it yet.  Otherwise, minimally putting it at the top of this
> file and in Jeff's docs seem like a good idea.

Done.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py
File lib/deploy_charm_for_testing.py (right):

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:15: DEFAULT_BRANCH = 'lp:juju-gui'
On 2013/03/14 19:19:18, gary.poster wrote:
> This is actually the DEFAULT_SOURCE, from the perspective of the charm.  We
can
> supply anything here that the charm allows in juju-gui-source, and we may want
> to do that so that we can test a release with the charm, for instance.

Changed to default origin (as this is like the term used in juju).

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:72: parser.add_argument('--branch',
default=DEFAULT_BRANCH)
On 2013/03/14 19:19:18, gary.poster wrote:
> --source?  --gui-source?  --gui?

--origin

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:98: instance_ip =
os.environ.get("JUJU_INSTANCE_IP")
On 2013/03/14 19:19:18, gary.poster wrote:
> Oh cool.  So if you don't provide this variable, everything works fine on ec2,
> yeah?
> 
It should.

> I want to verify that Jeff's docs describe this.

I was wary of documenting this as its only used in the CI tests, behind the VPN
and Firewalls. We can include it though. We only have access to the single IP
address now and it is set in the CI

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:102: "euca-describe-instances | grep INSTANCE |
grep juju-juju-gui-testing-instance-1 | awk '{print $2;}'", shell=True).strip()
On 2013/03/14 19:19:18, gary.poster wrote:
> IWBNI this were < 80 chars.

Done.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:104: "euca-describe-instances | grep INSTANCE |
grep juju-juju-gui-testing-instance-1 | awk '{print $12;}'", shell=True).strip()
On 2013/03/14 19:19:18, gary.poster wrote:
> IWBNI this were < 80 chars.

Done.

https://codereview.appspot.com/7816043/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:108:
subprocess.check_call("euca-associate-address -i %s %s" % (instance_id,
instance_ip), shell=True)
On 2013/03/14 19:19:18, gary.poster wrote:
> IWBNI this were < 80 chars.

Done.

https://codereview.appspot.com/7816043/diff/1/test/browser.py
File test/browser.py (right):

https://codereview.appspot.com/7816043/diff/1/test/browser.py#newcode141
test/browser.py:141: @retry(WebDriverException,
format_error=formatWebDriverError)
On 2013/03/14 19:19:18, gary.poster wrote:
> Why?  Maybe worth a comment.
Some operations intermittently fail, the retries help work around it sometimes.
Sad story. This one isn't as important as the usage on restart api which would
sometimes 
fail to answer and restart in time. In the future when things are a little more
solid I'd be happy to try it again w/o these.

https://codereview.appspot.com/7816043/diff/1/test/browser.py#newcode186
test/browser.py:186: # firewalls and provider issues in some cases.
On 2013/03/14 19:19:18, gary.poster wrote:
> nice

Took a sad amount of time to see this was needed as it only appeared as a
timeout error :(

https://codereview.appspot.com/7816043/diff/1/test/retry.py
File test/retry.py (right):

https://codereview.appspot.com/7816043/diff/1/test/retry.py#newcode8
test/retry.py:8:
http://www.saltycrane.com/blog/2009/11/trying-out-retry-decorator-python/
On 2013/03/14 19:19:18, gary.poster wrote:
> BSD licensed
> (https://github.com/saltycrane/retry-decorator/blob/master/LICENSE).  Maybe we
> should mention that?

I already added support for the format_error and one other minor cleanup. As
mentioned around the usage we may be able drop this in the near future, but
until things are solid I'd like to keep or rewrite it if you think the license
is an issue.

[bcsaller, 2013-03-14 21:34:30]

*** Submitted:

CI testing support/improvements

This branch includes the changes to support CI testing. At the time 
of this writing there is still one IE/Selenium interaction issue that
would fail the deploy test. That is conditionally skipped and must
be revisited in the future.

R=gary.poster
CC=
https://codereview.appspot.com/7816043