Merge lp://qastaging/~frankban/charms/precise/juju-gui/deployer-functional-test into lp://qastaging/~juju-gui/charms/precise/juju-gui/trunk

Reviewers: mp+187502_code.launchpad.net,

Message:
Please take a look.

Description:
Bundle deployment support functional test.

This branch includes a charm functional test
exercising the builtin server bundle support.

Reorganized the functional tests so that, at
least when the builtin server is used, the
deployed GUI service is reused by all tests.

Also fixed a pyJuju clean up error when the
builtin server is used to serve the GUI.

Added an helper function to retrieve, if
possible, the admin secret for the current
Juju environment, and a very simple WebSocket
client object used in tests.
Note that the websocket-client package used
by the client is now explicitly listed in
the requirements file, but it was already
installed before as a dependency of the
python juju client.

No QA required.

Tests:
run `make test JUJU_ENV="your-juju-environment"`
from the root of this branch.
Note that it is not currently possible to run
juju-test with a juju-core local provider:
with LXC juju-core requires the bootstrap and
destroy-environment commands to be run as root.
For this reason, I suggest to run the tests
using ec2; in my machine they take about 1:15h.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/deployer-functional-test/+merge/187502

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13890044/

Affected files (+532, -37 lines):
   M Makefile
   A [revision details]
   M revision
   M tests/20-functional.test
   A tests/example.py
   M tests/helpers.py
   M tests/requirements.pip
   M tests/test_helpers.py

LGTM with some suggestions on breaking up the long test.
My tests are still running. I'll update when they finish.

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

https://codereview.appspot.com/13890044/diff/1/Makefile#newcode17
Makefile:17: JUJUTEST = juju-test --timeout=120m -v -e "$(JUJU_ENV)"
--upload-tools
ugh.

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test#newcode153
tests/20-functional.test:153: if options.get('builtin-server') ==
'true':
should we make this test case-insensitive? or is 'true' the
one-true-way in YAML?

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test#newcode282
tests/20-functional.test:282: def test_deployer(self):
It would be nice if this super long test could be broken up into parts.
I can see it easily being split into 4 or more tests. Especially the
error conditions could be separate.

https://codereview.appspot.com/13890044/diff/1/tests/example.py
File tests/example.py (right):

https://codereview.appspot.com/13890044/diff/1/tests/example.py#newcode18
tests/example.py:18:
An entry with multiple bundles would be nice for testing.

https://codereview.appspot.com/13890044/

LGTM with suggestions. I see now I duplicated bac's comments, except he
had some other good ones additionally. :-)

Thank you!

Gary

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

https://codereview.appspot.com/13890044/diff/1/Makefile#newcode17
Makefile:17: JUJUTEST = juju-test --timeout=120m -v -e "$(JUJU_ENV)"
--upload-tools
ugh :-)

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test#newcode282
tests/20-functional.test:282: def test_deployer(self):
Wow!

My only concern with this is that I think we could divide up the test
into smaller chunks. Story tests like this have been historically
painful to maintain.

For example, we could put the different edge cases in separate tests.

I'm guessing there's a downside, or else you would have done just that.
Is the downside that it will mean a lot more time lost to
startup/teardown?

Please either add a comment explaining why it makes sense to combine all
of the edge cases together, or (if there isn't a good reason) divide
them up.

https://codereview.appspot.com/13890044/

Please take a look.

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test#newcode153
tests/20-functional.test:153: if options.get('builtin-server') ==
'true':
On 2013/09/25 13:49:48, bac wrote:
> should we make this test case-insensitive? or is 'true' the
one-true-way in
> YAML?

We set that value ourselves when deploying the GUI in tests, so we know
for sure the correct spelling.
FWIW, juju-core IIRC accepts True, true, Yes and yes...

https://codereview.appspot.com/13890044/diff/1/tests/20-functional.test#newcode282
tests/20-functional.test:282: def test_deployer(self):
On 2013/09/25 14:00:28, gary.poster wrote:
> Wow!

> My only concern with this is that I think we could divide up the test
into
> smaller chunks. Story tests like this have been historically painful
to
> maintain.

> For example, we could put the different edge cases in separate tests.

> I'm guessing there's a downside, or else you would have done just
that. Is the
> downside that it will mean a lot more time lost to startup/teardown?

> Please either add a comment explaining why it makes sense to combine
all of the
> edge cases together, or (if there isn't a good reason) divide them up.

Thank you both for suggesting this. I agree with you. Now we have a test
for each error case, and a test for the double deployment story.
I did a single test before because I mechanically converted a QA script
I have, but, as I mentioned, I agree we must pursue test separation.

https://codereview.appspot.com/13890044/diff/1/tests/example.py
File tests/example.py (right):

https://codereview.appspot.com/13890044/diff/1/tests/example.py#newcode18
tests/example.py:18:
On 2013/09/25 13:49:48, bac wrote:
> An entry with multiple bundles would be nice for testing.

Yes, and you also gave me an idea for a test. I'll change this in
another branch if you agree.

https://codereview.appspot.com/13890044/

LGTM - I ran the tests and they took about 70 minutes and all passed.

https://codereview.appspot.com/13890044/

*** Submitted:

Bundle deployment support functional test.

This branch includes a charm functional test
exercising the builtin server bundle support.

Reorganized the functional tests so that, at
least when the builtin server is used, the
deployed GUI service is reused by all tests.

Also fixed a pyJuju clean up error when the
builtin server is used to serve the GUI.

Added an helper function to retrieve, if
possible, the admin secret for the current
Juju environment, and a very simple WebSocket
client object used in tests.
Note that the websocket-client package used
by the client is now explicitly listed in
the requirements file, but it was already
installed before as a dependency of the
python juju client.

No QA required.

Tests:
run `make test JUJU_ENV="your-juju-environment"`
from the root of this branch.
Note that it is not currently possible to run
juju-test with a juju-core local provider:
with LXC juju-core requires the bootstrap and
destroy-environment commands to be run as root.
For this reason, I suggest to run the tests
using ec2; in my machine they take about 1:15h.

R=bac, gary.poster
CC=
https://codereview.appspot.com/13890044

https://codereview.appspot.com/13890044/

Thank you both for the reviews!

https://codereview.appspot.com/13890044/