Merge lp:~benji/charms/precise/juju-gui/second into lp:~juju-gui/charms/precise/juju-gui/trunk

[Benji York]

Initial project structure, mostly testing.

https://codereview.appspot.com/6850052/

[Brad Crittenden (bac)]

Hi Nicola and Benji,

This looks like a good start. I tried to work through your instructions
and ran into some problems that I have noted. Ultimately I failed due
to Quantal and LXC. I need to revisit it after sorting out those
problems.

So, consider this a tepid +1.

https://codereview.appspot.com/6850052/diff/1/README.txt
File README.txt (right):

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode21
README.txt:21:
This doesn't actually work as unit.test has '.discover('.') but is being
run from above. Either unit.test should read '.discover('./tests') or
it should be run from within the tests directory.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode43
README.txt:43: ./configure --prefix=/home/USER
--prefix=$HOME is simpler

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode46
README.txt:46:
Has anyone talked to Jim about packaging this into a PPA?

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode62
README.txt:62:
You should mention the cwd must be the top of the charm, which is
unintuitive since you pass it as a parameter.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode69
README.txt:69:
Ugh, I seem to have lxc issues since upgrading to Quantal, so I cannot
get it to start now.

https://codereview.appspot.com/6850052/diff/1/tests/test_addition.py
File tests/test_addition.py (right):

https://codereview.appspot.com/6850052/diff/1/tests/test_addition.py#newcode10
tests/test_addition.py:10:
Passing or Unpassing?

https://codereview.appspot.com/6850052/diff/1/tests/unit.test
File tests/unit.test (right):

https://codereview.appspot.com/6850052/diff/1/tests/unit.test#newcode6
tests/unit.test:6:
sys.exit(unittest.TextTestRunner().run(unittest.TestLoader().discover('.')))
See comment in README

https://codereview.appspot.com/6850052/

[Gary Poster (gary)]

Looks good, thank you! Maybe make the os.path change I mention, and
look at my logs and make sure they are healthy.

Otherwise approved.

Gary

https://codereview.appspot.com/6850052/diff/1/README.txt
File README.txt (right):

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode21
README.txt:21:
On 2012/11/19 18:57:34, bac wrote:
> This doesn't actually work as unit.test has '.discover('.') but is
being run
> from above. Either unit.test should read '.discover('./tests') or it
should be
> run from within the tests directory.
Maybe better would be to .discover(os.path.dirname(__file__))

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode42
README.txt:42: autoreconf
Before this I needed to sudo apt-get install libtool autoconf (and maybe
some other bits too)

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode46
README.txt:46:
On 2012/11/19 18:57:34, bac wrote:
> Has anyone talked to Jim about packaging this into a PPA?

It is supposed to land...sometime...

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode69
README.txt:69:
On 2012/11/19 18:57:34, bac wrote:
> Ugh, I seem to have lxc issues since upgrading to Quantal, so I cannot
get it to
> start now.
If you want to pay, ec2 works, I think.

The unit tests seem to run in a separate machine, which is seems
unnecessarily annoying. Not a huge deal, and maybe I'm misinterpreting:
http://pastebin.ubuntu.com/1371194/

Wow, running the tests is slow. :-/ I'd forgotten about this.

My lxc results were as follows. They seem to show some unexpected
problems. Are they OK? http://pastebin.ubuntu.com/1371234/

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode71
README.txt:71: "setup-environment" is an easy way to get started.
Maybe it would be worth mentioning that you might want to set your
default environment to lxc?

Maybe it would be worth mentioning that using an lxc environment
requires that you install the apt-cacher-ng and lxc packages?

Your call.

https://codereview.appspot.com/6850052/diff/1/tests/unit.test
File tests/unit.test (right):

https://codereview.appspot.com/6850052/diff/1/tests/unit.test#newcode6
tests/unit.test:6:
sys.exit(unittest.TextTestRunner().run(unittest.TestLoader().discover('.')))
On 2012/11/19 18:57:34, bac wrote:
> See comment in README

Mine too :-)

https://codereview.appspot.com/6850052/

[Nicola Larosa (teknico)]

FWIW, some of the raised points are addressed in a subsequent branch:
https://code.launchpad.net/~frankban/charms/precise/juju-gui/juju-gui/+merge/135213

https://codereview.appspot.com/6850052/diff/1/README.txt
File README.txt (right):

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode21
README.txt:21:
> Maybe better would be to .discover(os.path.dirname(__file__))

Yes, that's better.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode42
README.txt:42: autoreconf
> Before this I needed to sudo apt-get install libtool autoconf (and
maybe some
> other bits too)

Yes, good idea mentioning it too.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode43
README.txt:43: ./configure --prefix=/home/USER
> --prefix=$HOME is simpler

It can actually be any directory on $PATH, or left out if one is
comfortable with /usr/local. Whatever path is chosen, the subdirs bin/
and share/ will be created inside it, if not already there.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode46
README.txt:46:
>> Has anyone talked to Jim about packaging this into a PPA?

> It is supposed to land...sometime...

Yes, it should become part of Jitsu eventually.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode62
README.txt:62:
> You should mention the cwd must be the top of the charm, which is
unintuitive
> since you pass it as a parameter.

Good idea mentioning it too.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode69
README.txt:69:
> My lxc results were as follows. They seem to show some unexpected
problems.
> Are they OK? http://pastebin.ubuntu.com/1371234/

The failure of collecting logs is a known problem, and we forgot making
the assertion succeed again. :-)

https://codereview.appspot.com/6850052/diff/1/tests/test_addition.py
File tests/test_addition.py (right):

https://codereview.appspot.com/6850052/diff/1/tests/test_addition.py#newcode10
tests/test_addition.py:10:
> Passing or Unpassing?

Eh. We forgot to put the expected value back to 2. :-)

https://codereview.appspot.com/6850052/

[lp:~benji/charms/precise/juju-gui/second]

5. By Benji York on 2012-11-21

review fixes

=== added file 'README.txt'
--- README.txt	1970-01-01 00:00:00 +0000
+++ README.txt	2012-11-14 21:04:22 +0000
@@ -0,0 +1,71 @@
+==============
+Juju GUI Charm
+==============
+
+This charm makes it easy to deploy a Juju GUI into an existing environment.
+
+
+Testing
+=======
+
+There are two types of tests for the charm: unit tests and functional tests.
+
+
+Unit tests
+----------
+
+The unit tests do not require a functional Juju environment, and can be run
+with this command::
+
+    python tests/unit.test
+
+Unit tests should be created in the "tests" subdirectory and be named in the
+customary way (i.e., "test_*.py").
+
+
+Functional tests
+----------------
+
+Running the functional tests requires a Juju testing environment as provided
+by the Jitsu test command.  All files in the tests directory which end with
+".test" will be run in a Juju Jitsu test environment.
+
+
+Functional test setup
+~~~~~~~~~~~~~~~~~~~~~
+
+At the time of this writing the Jitsu test command is not yet released.  To
+run it you must first install it locally (replace USER with your user name)::
+
+    bzr branch lp:~jimbaker/juju-jitsu/unit-test jitsu-unit-test
+    cd jitsu-unit-test
+    autoreconf
+    ./configure --prefix=/home/USER
+    make
+    make install
+
+The current incarnation of the Jitsu test command requires that the current
+directory name match the charm name, so you must check out the charm into a
+directory named "juju-gui"::
+
+    bzr branch lp:~juju-gui/charms/precise/juju-gui/trunk juju-gui
+
+Now you are ready to run the functional tests (see the next section).
+
+
+Running the functional tests
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Run the functional tests thusly::
+
+    ~/bin/jitsu test juju-gui --logdir /tmp
+
+If Jitsu generates errors about not being able bootstrap::
+
+    CalledProcessError: Command '['juju', 'bootstrap']'...
+
+...or it hangs, then you may need to bootstrap the environment yourself and
+pass the --no-bootstrap switch to Jitsu.
+
+If you do not yet have an environment defined, the Jitsu command
+"setup-environment" is an easy way to get started.

=== added directory 'tests'
=== added file 'tests/functional-noop.test'
--- tests/functional-noop.test	1970-01-01 00:00:00 +0000
+++ tests/functional-noop.test	2012-11-14 21:04:22 +0000
@@ -0,0 +1,8 @@
+#!/usr/bin/env python2
+
+import sys
+
+# The only requirement for functional tests is that they have a non-zero exit
+# code on error.  This noop is successful, therefore we exit with zero.
+
+sys.exit(0)

=== added file 'tests/test_addition.py'
--- tests/test_addition.py	1970-01-01 00:00:00 +0000
+++ tests/test_addition.py	2012-11-14 21:04:22 +0000
@@ -0,0 +1,13 @@
+#!/usr/bin/env python2
+
+import sys
+import unittest
+
+
+class PassingTest(unittest.TestCase):
+    def test_addition(self):
+        self.assertEqual(1+1, 3)
+
+
+if __name__ == '__main__':
+    sys.exit(unittest.main())

=== added file 'tests/unit.test'
--- tests/unit.test	1970-01-01 00:00:00 +0000
+++ tests/unit.test	2012-11-14 21:04:22 +0000
@@ -0,0 +1,6 @@
+#!/usr/bin/env python2
+
+import unittest
+import sys
+
+sys.exit(unittest.TextTestRunner().run(unittest.TestLoader().discover('.')))