Add an option to start test server.

Add an option to start test server.

Added the "serve-tests" option to the charm, 
defaulting to False. When the option is enabled 
it is possible to run the Juju GUI unit tests 
using https://[sevice url]/test/.

I needed to conditionally add the corresponding 
location to the nginx configuration file. 
I discussed with Gary about possible ways to 
achieve this. Using the standard Python string 
substitution seemed hacky, and the resulting 
template file could be less readable. We decided 
to start using a templating library: ended up with 
tempita because it is lightweight and it is already 
used in maas.

nginx is now configured so that the /test/ location:
- serves unit tests if "serve-tests" is True;
- redirects to / if "serve-tests" is False.
The latter is required because otherwise the test 
location would be added to the browser appcache: 
the index.html file would be served and it references 
the manifest file.

Also fixed dependency management in the install
hook. Now all the Python libraries required by the
charm are installed at the beginning of the process,
before utils, launchpadlib, tempita etc. are imported. 

QA:

- deploy the charm;
- juju set juju-gui serve-tests=true;
- go to "https://[sevice url]/test/";
- you should see tests run (mocha html reporter).

Problems:

test-debug (staging=true) fails to run because 
it attempts to load external (insecure) resources.
Filed bug #1116320.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/bug-1110816-serve-tests/+merge/146639

(do not edit description out of merge proposal)

[frankban, 2013-02-05 14:48:25]

Please take a look.

[gary.poster, 2013-02-05 15:19:52]

Looks good to me.  I'll try it now, and then report back.  Thank you!

Gary

https://codereview.appspot.com/7306045/diff/1/config/nginx-site.template
File config/nginx-site.template (right):

https://codereview.appspot.com/7306045/diff/1/config/nginx-site.template#newc...
config/nginx-site.template:27: # automatically added to the application cache.
Nice.

Suggest revising second sentence to something like this: "This avoids
automatically adding the URL to the application cache."

https://codereview.appspot.com/7306045/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7306045/diff/1/hooks/utils.py#newcode158
hooks/utils.py:158: The tempita template language is used to render contents.
Maybe add link to http://pythonpaste.org/tempita/ to be nice. :-)

https://codereview.appspot.com/7306045/diff/1/hooks/utils.py#newcode168
hooks/utils.py:168: stream.write(template.substitute(context))
I'm impressed with the tempita simplicity.

https://codereview.appspot.com/7306045/diff/1/hooks/utils.py#newcode239
hooks/utils.py:239: build_dirname = 'build-debug' if in_staging else
'build-prod'
if (in_staging and not serve_tests)
?

I know that's nasty, but it would work around bug 1116320, and I'm not currently
interested in us tackling that bug unless it is really easy to fix.

Probably shouldn't do this, but I'm tempted.

[frankban, 2013-02-05 16:10:34]

Please take a look.

https://codereview.appspot.com/7306045/diff/1/config/nginx-site.template
File config/nginx-site.template (right):

https://codereview.appspot.com/7306045/diff/1/config/nginx-site.template#newc...
config/nginx-site.template:27: # automatically added to the application cache.
On 2013/02/05 15:19:52, gary.poster wrote:
> Nice.
> 
> Suggest revising second sentence to something like this: "This avoids
> automatically adding the URL to the application cache."

Done.

https://codereview.appspot.com/7306045/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7306045/diff/1/hooks/utils.py#newcode158
hooks/utils.py:158: The tempita template language is used to render contents.
On 2013/02/05 15:19:52, gary.poster wrote:
> Maybe add link to http://pythonpaste.org/tempita/ to be nice. :-)

Done.

https://codereview.appspot.com/7306045/diff/1/hooks/utils.py#newcode239
hooks/utils.py:239: build_dirname = 'build-debug' if in_staging else
'build-prod'
On 2013/02/05 15:19:52, gary.poster wrote:
> if (in_staging and not serve_tests)
> ?
> 
> I know that's nasty, but it would work around bug 1116320, and I'm not
currently
> interested in us tackling that bug unless it is really easy to fix.
> 
> Probably shouldn't do this, but I'm tempted.

What do you think about doing it and adding a XXX?

[gary.poster, 2013-02-05 16:19:00]

On 2013/02/05 16:10:34, frankban wrote:
> On 2013/02/05 15:19:52, gary.poster wrote:

...

> https://codereview.appspot.com/7306045/diff/1/hooks/utils.py#newcode239
> hooks/utils.py:239: build_dirname = 'build-debug' if in_staging else
> 'build-prod'
> On 2013/02/05 15:19:52, gary.poster wrote:
> > if (in_staging and not serve_tests)
> > ?
> > 
> > I know that's nasty, but it would work around bug 1116320, and I'm not
> currently
> > interested in us tackling that bug unless it is really easy to fix.
> > 
> > Probably shouldn't do this, but I'm tempted.
> 
> What do you think about doing it and adding a XXX?

Yeah, that sounds good to me.

[gary.poster, 2013-02-05 16:33:55]

Land as is or with changes as suggested.

The QA worked for me!  I initially was surprised that /test didn't work, but I
think it is fine that /test/ is required.

Thank you.

Gary

[frankban, 2013-02-05 16:58:50]

Please take a look.

[teknico, 2013-02-05 17:26:29]

Land with changes, see below.

Also: is it appropriate that the test URL bypasses authentication?

https://codereview.appspot.com/7306045/diff/8001/hooks/install
File hooks/install (right):

https://codereview.appspot.com/7306045/diff/8001/hooks/install#newcode21
hooks/install:21: # of python-charmhelpers.
Nitpick: I'd still list python-shelltoolbox here, because our code uses it
directly, and it might go away from python-charmhelpers' dependencies one day.

https://codereview.appspot.com/7306045/diff/8001/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7306045/diff/8001/hooks/utils.py#newcode160
hooks/utils.py:160: The arguments *template_name* is the name or path of the
template file:
s/arguments/argument/

[gary.poster, 2013-02-05 18:57:57]

On 2013/02/05 17:26:29, teknico wrote:
> Land with changes, see below.
> 
> Also: is it appropriate that the test URL bypasses authentication?

Seems good to me.  We aren't connecting to anything in our test that requires
authentication.

[frankban, 2013-02-06 09:23:52]

Please take a look.

https://codereview.appspot.com/7306045/diff/8001/hooks/install
File hooks/install (right):

https://codereview.appspot.com/7306045/diff/8001/hooks/install#newcode21
hooks/install:21: # of python-charmhelpers.
On 2013/02/05 17:26:29, teknico wrote:
> Nitpick: I'd still list python-shelltoolbox here, because our code uses it
> directly, and it might go away from python-charmhelpers' dependencies one day.

Done.

https://codereview.appspot.com/7306045/diff/8001/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7306045/diff/8001/hooks/utils.py#newcode160
hooks/utils.py:160: The arguments *template_name* is the name or path of the
template file:
On 2013/02/05 17:26:29, teknico wrote:
> s/arguments/argument/

Done, thanks.

[frankban, 2013-02-06 11:55:35]

On 2013/02/05 18:57:57, gary.poster wrote:
> On 2013/02/05 17:26:29, teknico wrote:
> > Land with changes, see below.
> > 
> > Also: is it appropriate that the test URL bypasses authentication?
> 
> Seems good to me.  We aren't connecting to anything in our test that requires
> authentication.

Indeed, the test suite does not require a connection to the backend.

Thanks for your reviews Gary and Nicola.

[frankban, 2013-02-06 11:56:48]

*** Submitted:

Add an option to start test server.

Added the "serve-tests" option to the charm, 
defaulting to False. When the option is enabled 
it is possible to run the Juju GUI unit tests 
using https://[sevice url]/test/.

I needed to conditionally add the corresponding 
location to the nginx configuration file. 
I discussed with Gary about possible ways to 
achieve this. Using the standard Python string 
substitution seemed hacky, and the resulting 
template file could be less readable. We decided 
to start using a templating library: ended up with 
tempita because it is lightweight and it is already 
used in maas.

nginx is now configured so that the /test/ location:
- serves unit tests if "serve-tests" is True;
- redirects to / if "serve-tests" is False.
The latter is required because otherwise the test 
location would be added to the browser appcache: 
the index.html file would be served and it references 
the manifest file.

Also fixed dependency management in the install
hook. Now all the Python libraries required by the
charm are installed at the beginning of the process,
before utils, launchpadlib, tempita etc. are imported. 

QA:

- deploy the charm;
- juju set juju-gui serve-tests=true;
- go to "https://[sevice url]/test/";
- you should see tests run (mocha html reporter).

Problems:

test-debug (staging=true) fails to run because 
it attempts to load external (insecure) resources.
Filed bug #1116320.

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