First browser tests and infrastruture for more.

First browser tests and infrastruture for more.

See docs/browser-testing.rst for usage instructions.

https://code.launchpad.net/~benji/juju-gui/ci-makings-2/+merge/146232

(do not edit description out of merge proposal)

[benji, 2013-02-01 21:35:58]

Please take a look.

[teknico, 2013-02-04 12:30:30]

Request review.

The command "bin/test-charm" does not run successfully because it tries to run
the command "bin/py" that is not there. I guess I need to run "make test-misc"
first.

You may want to add "make test-misc" to the Makefile help.

Please beef up the test running instructions in docs/browser.testing.rst, and
also link it from index.rst.

"make selenium" does not seem to be called by anything.

Trying to run tests now...

https://codereview.appspot.com/7248050/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode510
Makefile:510: view-code-doc view-docs view-main-doc yuidoc-lint shelltoolbox
selenium
Please keep the PHONY targets in alphabetical order. Thank you. :-)

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

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:18: return branch
All of this could be shortened down to:

    return argv[1] if len(argv) > 1 else None # trunk

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:23: return juju(*'status --environment
juju-gui-testing --format json'.split())
Nice one-liner. :-)

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:30: return state
This is curiously verbose, and could also be shortened down to one or maybe two
lines.

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

https://codereview.appspot.com/7248050/diff/1/test/browser.py#newcode33
test/browser.py:33: command_executor =
'http://%s@ondemand.saucelabs.com:80/wd/hub' % credentials
The credentials end up in the URL in clear, and the connection is not encrypted.
Is this secure enough?

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
File test/test_deploy_charm_for_testing.py (right):

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
test/test_deploy_charm_for_testing.py:42: wait_for_service(get_service_state,
sleep=None)
I guess you meant to pass sleep=noop here.

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
test/test_deploy_charm_for_testing.py:81: '{"agent-state": "started"}}}}}'
Maybe use round parentheses and implicit string concatenation instead? I dislike
continuation backslashes in Python code.

[bac, 2013-02-04 14:47:26]

Nice start Benji -- thanks for tackling it.

Land with changes.

https://codereview.appspot.com/7248050/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode266
Makefile:266: virtualenv/bin/easy_install ./python-shelltoolbox
I develop using a sandbox model.  My shared repository is configured to branch
with no tree, so this target breaks since the tree is not populated after the
branch.

Using 'bzr branch --standalone' will work for all situations so please add that
swtich.

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode267
Makefile:267: 
Why do we prefer the branch to the packaged version?

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode270
Makefile:270: 
This target isn't a dependency of anything so it must be made by hand. 
Shouldn't it be done via test-misc?

https://codereview.appspot.com/7248050/diff/1/docs/browser-testing.rst
File docs/browser-testing.rst (right):

https://codereview.appspot.com/7248050/diff/1/docs/browser-testing.rst#newcode28
docs/browser-testing.rst:28: 
Fails due to no selenium.
Shouldn't 'make test-misc' be run first?

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

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:24: 
Since you do the juju(string.split()) dance multiple times, it might be more
readable if you had a wrapper that just took the string.  And the wrapper should
be goldenrod with maybe an accent wall.

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:31: 
I'd be in favor of 

return unit['agent-state'] 

but that's all of the additional brevity I'd suggest.

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

https://codereview.appspot.com/7248050/diff/1/test/browser.py#newcode28
test/browser.py:28: }
Is it safe to have the access key in this branch?

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
File test/test_deploy_charm_for_testing.py (right):

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
test/test_deploy_charm_for_testing.py:42: wait_for_service(get_service_state,
sleep=None)
On 2013/02/04 12:30:30, teknico wrote:
> I guess you meant to pass sleep=noop here.

No, he is depending on the fact that None is not called as a function and does
not raise an exception.  Right Benji?

It might be easier to follow if you used the ZZZZ trick from below and just
showed that it wasn't called.  More consistent.

[benji, 2013-02-04 17:52:20]

> The command "bin/test-charm" does not run successfully because it tries to run
> the command "bin/py" that is not there. I guess I need to run "make test-misc"
> first.

Fixed.

> You may want to add "make test-misc" to the Makefile help.

Done.

> Please beef up the test running instructions in docs/browser.testing.rst,

I have added a bit about running make before attempting to run the
script.  What other additions would you like?

> and also link it from index.rst.

Done.

> "make selenium" does not seem to be called by anything.

Fixed.

https://codereview.appspot.com/7248050/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode266
Makefile:266: virtualenv/bin/easy_install ./python-shelltoolbox
On 2013/02/04 14:47:26, bac wrote:
> I develop using a sandbox model.  My shared repository is configured to branch
> with no tree, so this target breaks since the tree is not populated after the
> branch.
> 
> Using 'bzr branch --standalone' will work for all situations so please add
that
> swtich.

Done.

I question the wisdom of running a setup that renders vanilla branching
inoperative.

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode267
Makefile:267: 
On 2013/02/04 14:47:26, bac wrote:
> Why do we prefer the branch to the packaged version?

Because I was supposed to change that before proposing the branch.  Fixed.

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode270
Makefile:270: 
On 2013/02/04 14:47:26, bac wrote:
> This target isn't a dependency of anything so it must be made by hand. 

Good find.

> Shouldn't it be done via test-misc?

Actually it is required for bin/test-charm.  I rejiggered the Makefile so it all
works correctly now.

https://codereview.appspot.com/7248050/diff/1/Makefile#newcode510
Makefile:510: view-code-doc view-docs view-main-doc yuidoc-lint shelltoolbox
selenium
On 2013/02/04 12:30:30, teknico wrote:
> Please keep the PHONY targets in alphabetical order. Thank you. :-)

Done.

That rule is misguided.

https://codereview.appspot.com/7248050/diff/1/docs/browser-testing.rst
File docs/browser-testing.rst (right):

https://codereview.appspot.com/7248050/diff/1/docs/browser-testing.rst#newcode28
docs/browser-testing.rst:28: 
On 2013/02/04 14:47:26, bac wrote:
> Fails due to no selenium.
> Shouldn't 'make test-misc' be run first?

Fixed.

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

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:18: return branch
On 2013/02/04 12:30:30, teknico wrote:
> All of this could be shortened down to:
> 
>     return argv[1] if len(argv) > 1 else None # trunk

It cold be, but it would be unreadable.  I like it how it is.

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:24: 
On 2013/02/04 14:47:26, bac wrote:
> Since you do the juju(string.split()) dance multiple times, it might be more
> readable if you had a wrapper that just took the string.

The reason I did not do so was that in the past I have run into problems
in exactly this scenario.  The issue is that as the program evolves you
inevitably have to build commands that contain uncontrolled user input
and that input may contain unescaped spaces, therefore building a usable
string becomes impossible.  However, this script is so small that
perhaps the chances of that happening are low enough, and the cost of
refactoring small enough that a helper is the right path.

I have implemented the helper, we'll see if we're happy with it in the
long run.

> And the wrapper should be goldenrod with maybe an accent wall.

LOL!

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:30: return state
On 2013/02/04 12:30:30, teknico wrote:
> This is curiously verbose, and could also be shortened down to one or maybe
two
> lines.

Naming the intermediate values increases clarity.  Per Brad's comment, I
have removed one name that was truly superfluous.

https://codereview.appspot.com/7248050/diff/1/lib/deploy_charm_for_testing.py...
lib/deploy_charm_for_testing.py:31: 
On 2013/02/04 14:47:26, bac wrote:
> I'd be in favor of 
> 
> return unit['agent-state'] 
> 
> but that's all of the additional brevity I'd suggest.

Agreed.  Done.

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

https://codereview.appspot.com/7248050/diff/1/test/browser.py#newcode28
test/browser.py:28: }
On 2013/02/04 14:47:26, bac wrote:
> Is it safe to have the access key in this branch?

Please see the answer to Nicola's related question below.

https://codereview.appspot.com/7248050/diff/1/test/browser.py#newcode33
test/browser.py:33: command_executor =
'http://%s@ondemand.saucelabs.com:80/wd/hub' % credentials
On 2013/02/04 12:30:30, teknico wrote:
> The credentials end up in the URL in clear, and the connection is not
encrypted.
> Is this secure enough?

That is certainly a good question to ask.  Given the limited capabilities that
the credentials impart (primarily the ability to run a web browser via Sauce
Labs) and that anyone can sign up for their own credentials at no cost, this
seems like an appropriate handling of the credentials.

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
File test/test_deploy_charm_for_testing.py (right):

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
test/test_deploy_charm_for_testing.py:42: wait_for_service(get_service_state,
sleep=None)
On 2013/02/04 12:30:30, teknico wrote:
> I guess you meant to pass sleep=noop here.

The comment explains why None is passed.

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
test/test_deploy_charm_for_testing.py:42: wait_for_service(get_service_state,
sleep=None)
On 2013/02/04 14:47:26, bac wrote:
> On 2013/02/04 12:30:30, teknico wrote:
> > I guess you meant to pass sleep=noop here.
> 
> No, he is depending on the fact that None is not called as a function and does
> not raise an exception.  Right Benji?
> 
> It might be easier to follow if you used the ZZZZ trick from below and just
> showed that it wasn't called.  More consistent.

I can buy that.  Done.

https://codereview.appspot.com/7248050/diff/1/test/test_deploy_charm_for_test...
test/test_deploy_charm_for_testing.py:81: '{"agent-state": "started"}}}}}'
On 2013/02/04 12:30:30, teknico wrote:
> Maybe use round parentheses and implicit string concatenation instead? I
dislike
> continuation backslashes in Python code.

I like continuations.  However the prevailing style is otherwise, so I have
changed the code.

[benji, 2013-02-04 17:53:13]

Please take a look.

[teknico, 2013-02-04 18:43:09]

Thanks for this changes, land as is, and see comments below.

benji wrote:
>> Please beef up the test running instructions in
>> docs/browser.testing.rst,

> I have added a bit about running make before attempting
> to run the script.  What other additions would you like?

That should be enough to successfully run the tests, thanks.

>> Please keep the PHONY targets in alphabetical order. Thank you. :-)

> Done.

It does not look like that. :-)

> That rule is misguided.

Oh. Sorry; I thought we agreed on that, but maybe I am making this up. I cannot
think of a more useful layout, though. Suggestions? Ok, maybe elsewhere. :-)

>> This is curiously verbose, and could also be shortened down
>> to one or maybe two lines.

> Naming the intermediate values increases clarity.  Per Brad's
> comment, I have removed one name that was truly superfluous.

I get your point now, sorry for not realizing it earlier.

>> The credentials end up in the URL in clear, and the connection
>> is not encrypted. Is this secure enough?

> That is certainly a good question to ask.  Given the limited
> capabilities that the credentials impart (primarily the ability
> to run a web browser via Sauce Labs) and that anyone can sign
> up for their own credentials at no cost, this seems like
> an appropriate handling of the credentials.

Thanks for the evaluation.

>> Maybe use round parentheses and implicit string concatenation
>> instead? I dislike continuation backslashes in Python code.

> I like continuations.  However the prevailing style is otherwise,
> so I have changed the code.

Thank you.

[benji, 2013-02-04 19:18:24]

On Mon, Feb 4, 2013 at 12:43 PM,  <nicola.larosa@canonical.com> wrote:
> Thanks for this changes, land as is, and see comments below.

Thank you for the very good review.

>>> Please beef up the test running instructions in
>>> docs/browser.testing.rst,
>
>
>> I have added a bit about running make before attempting
>> to run the script.  What other additions would you like?
>
>
> That should be enough to successfully run the tests, thanks.

I also added bits about specifying which browser to use, the URL to run
the tests against, and how to run a single browser test.

>>> Please keep the PHONY targets in alphabetical order. Thank you. :-)
>
>
>> Done.
>
>
> It does not look like that. :-)

Ha!  Indeed.  Really done now.

>> That rule is misguided.
>
>
> Oh. Sorry; I thought we agreed on that, but maybe I am making this up. I
> cannot think of a more useful layout, though. Suggestions? Ok, maybe
> elsewhere. :-)

The rule is listed in the Makefile, I just didn't see it.  I don't have
a problem following misguided rules, presuming I can remember them.

>>> The credentials end up in the URL in clear, and the connection
>>> is not encrypted. Is this secure enough?
>
>
>> That is certainly a good question to ask.  Given the limited
>> capabilities that the credentials impart (primarily the ability
>> to run a web browser via Sauce Labs) and that anyone can sign
>> up for their own credentials at no cost, this seems like
>> an appropriate handling of the credentials.
>
>
> Thanks for the evaluation.

It is probably valuable to capture this information and place it in-line
with the code itself.  I have done so.
-- 
Benji York

[benji, 2013-02-04 19:21:17]

*** Submitted:

First browser tests and infrastruture for more.

See docs/browser-testing.rst for usage instructions.

R=teknico, bac
CC=
https://codereview.appspot.com/7248050