Initial project structure, mostly testing.

Initial project structure, mostly testing.



https://code.launchpad.net/~benji/charms/precise/juju-gui/second/+merge/134370

(do not edit description out of merge proposal)

[benji, 2012-11-14 21:03:44]

Please take a look.

[bac, 2012-11-19 18:57:34]

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

[gary.poster, 2012-11-19 22:21:00]

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 :-)

[teknico, 2012-11-21 12:02:08]

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

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. :-)

[benji, 2012-11-21 13:48:50]

Thanks for the review guys.  I have fixed everything you pointed out (and a
couple of small things you didn't, that's how good you are at reviewing).

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.

Good catch.  That was a late change that we didn't retry.  Fixed using Gary's
good suggestion.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode42
README.txt:42: autoreconf
I have added a note.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode43
README.txt:43: ./configure --prefix=/home/USER
On 2012/11/19 18:57:34, bac wrote:
> --prefix=$HOME is simpler

Good one.  Done.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode43
README.txt:43: ./configure --prefix=/home/USER
On 2012/11/21 12:02:08, teknico wrote:
> > --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.

I have added a note about installation options.

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

Done.

https://codereview.appspot.com/6850052/diff/1/README.txt#newcode71
README.txt:71: "setup-environment" is an easy way to get started.
On 2012/11/19 22:21:00, gary.poster wrote:
> Maybe it would be worth mentioning that you might want to set your default
> environment to lxc?

Sure.  Done.

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

I have added a note.  I don't know how far to go with this though because
setting up LXC is (way) outside the scope of this document.

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: 
On 2012/11/19 18:57:34, bac wrote:
> Passing or Unpassing?

Heh, fixed.

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

Done.

[benji, 2012-11-21 13:49:55]

*** Submitted:

Initial project structure, mostly testing.

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