Merge lp:~frankban/charms/precise/juju-gui/ftests-bootstrap-node into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

Deploy the GUI to machine 0 in functional tests.

Also added guiserver info checks, and fixed the
deploy module so that the previous behavior of
"make deploy" is restored and the resulting
service name is not random generated.

Removed test_local_release: that scenario is
already tested in TestBuiltinServerLocalRelease.

Moved test_nrpe_check_available to
TestBuiltinServerLocalRelease.

Tests: assuming ec2 is the name of your ec2
environment, run the following:
`time make ftest JUJU_ENV="ec2"`
Tests should pass and take ~25-30 minutes.

https://codereview.appspot.com/34130043/

[Francesco Banconi (frankban)]

Reviewers: mp+196954_code.launchpad.net,

Message:
Please take a look.

Description:
Deploy the GUI to machine 0 in functional tests.

Also added guiserver info checks, and fixed the
deploy module so that the previous behavior of
"make deploy" is restored and the resulting
service name is not random generated.

Removed test_local_release: that scenario is
already tested in TestBuiltinServerLocalRelease.

Moved test_nrpe_check_available to
TestBuiltinServerLocalRelease.

Tests: assuming ec2 is the name of your ec2
environment, run the following:
`time make ftest JUJU_ENV="ec2"`
Tests should pass and take ~25-30 minutes.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/ftests-bootstrap-node/+merge/196954

(do not edit description out of merge proposal)

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

Affected files (+141, -108 lines):
   M Makefile
   A [revision details]
   M revision
   M tests/20-functional.test
   M tests/deploy.py
   M tests/example.py
   M tests/helpers.py
   M tests/test_deploy.py

[Gary Poster (gary)]

LGTM with trivial. Thank you!

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

https://codereview.appspot.com/34130043/diff/1/tests/20-functional.test#newcode58
tests/20-functional.test:58: Deploy the charm in the bootstrap node if
possible in the current Juju
Suggest slight tweak for readability:

Deploy the charm, using the bootstrap node if the current Juju
implementation supports it.

https://codereview.appspot.com/34130043/

[Madison Scott-Clary (makyo)]

Code LGTMly, was not able to QA due to unrelated failures; can someone
else QA please?

https://codereview.appspot.com/34130043/

[Gary Poster (gary)]

Francesco, under the circumstances, I suggest that you either run the
test on another computer or in another virtual machine or something,
and then land it; or just land it. :-)

> On Nov 27, 2013, at 10:24 PM, Matthew Scott <email address hidden> wrote:
>
> Code LGTMly, was not able to QA due to unrelated failures; can someone
> else QA please?
>
> https://codereview.appspot.com/34130043/
>
> --
> https://code.launchpad.net/~frankban/charms/precise/juju-gui/ftests-bootstrap-node/+merge/196954
> Your team Juju GUI Hackers is subscribed to branch lp:~juju-gui/charms/precise/juju-gui/trunk.

[Jeff Pihach (hatch)]

QA OK

real 23m39.657s

https://codereview.appspot.com/34130043/

[lp:~frankban/charms/precise/juju-gui/ftests-bootstrap-node]

142. By Francesco Banconi on 2013-11-28

Merged trunk.

143. By Francesco Banconi on 2013-11-28

Checkpoint.

[Francesco Banconi (frankban)]

*** Submitted:

Deploy the GUI to machine 0 in functional tests.

Also added guiserver info checks, and fixed the
deploy module so that the previous behavior of
"make deploy" is restored and the resulting
service name is not random generated.

Removed test_local_release: that scenario is
already tested in TestBuiltinServerLocalRelease.

Moved test_nrpe_check_available to
TestBuiltinServerLocalRelease.

Tests: assuming ec2 is the name of your ec2
environment, run the following:
`time make ftest JUJU_ENV="ec2"`
Tests should pass and take ~25-30 minutes.

R=gary.poster, matthew.scott, jeff.pihach
CC=
https://codereview.appspot.com/34130043

https://codereview.appspot.com/34130043/

[Francesco Banconi (frankban)]

Thank you!

https://codereview.appspot.com/34130043/

=== modified file 'Makefile'
--- Makefile	2013-11-22 16:01:36 +0000
+++ Makefile	2013-11-28 16:43:14 +0000
@@ -14,7 +14,7 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-JUJUTEST = juju-test --timeout=120m -v -e "$(JUJU_ENV)"
+JUJUTEST = yes | juju-test --timeout=40m -v -e "$(JUJU_ENV)"
 VENV = tests/.venv
 SYSDEPS = build-essential bzr libapt-pkg-dev libpython-dev python-virtualenv \
 	rsync xvfb

=== modified file 'server/guiserver/tests/test_auth.py'
--- server/guiserver/tests/test_auth.py	2013-11-25 14:02:15 +0000
+++ server/guiserver/tests/test_auth.py	2013-11-28 16:43:14 +0000
@@ -317,7 +317,7 @@
             self.assertFalse(is_login, request)
 
 
-class TestAuthenticationTokenHandler(unittest.TestCase):
+class TestAuthenticationTokenHandler(LogTrapTestCase, unittest.TestCase):
 
     def setUp(self):
         super(TestAuthenticationTokenHandler, self).setUp()

=== modified file 'tests/20-functional.test'
--- tests/20-functional.test	2013-11-26 19:43:20 +0000
+++ tests/20-functional.test	2013-11-28 16:43:14 +0000
@@ -18,8 +18,10 @@
 
 from __future__ import print_function
 import httplib
+import json
 import itertools
 import unittest
+import urllib2
 import urlparse
 
 from selenium.webdriver import Firefox
@@ -34,6 +36,7 @@
     get_admin_secret,
     juju_destroy_service,
     juju_version,
+    make_service_name,
     stop_services,
     WebSocketClient,
 )
@@ -49,9 +52,48 @@
     print(err)
 
 
+def juju_deploy_gui(options=None):
+    """Deploy the Juju GUI charm with the given options.
+
+    Deploy the charm in the bootstrap node if possible in the current Juju
+    implementation. Use a random service name.
+
+    Return a tuple containing the deployed service name, unit info and a list
+    of services that are started in the Juju GUI unit and that must be
+    manually stopped by tests. In juju-core, the services list is always empty.
+    """
+    force_machine = None if is_legacy_juju else 0
+    service_name = make_service_name(prefix='juju-gui-')
+    unit_info = juju_deploy(
+        'juju-gui', service_name=service_name, options=options,
+        force_machine=force_machine)
+    # XXX 2012-11-29 frankban bug=872264:
+        # Just invoking ``juju destroy-service juju-gui`` in tearDown
+        # should execute the ``stop`` hook, stopping all the services
+        # started by the charm in the machine. Right now this does not
+        # work in pyJuju, so the desired effect is achieved by keeping
+        # track of started services and manually stopping them here.
+    # Once pyJuju works correctly or we drop support for it altogether, we
+    # can remove this shim.
+    cleanup_services = []
+    if is_legacy_juju:
+        if options is None:
+            options = {}
+        # Either stop the builtin server or the old apache2/haproxy setup.
+        if options.get('builtin-server') == 'true':
+            cleanup_services.append('guiserver')
+        else:
+            cleanup_services.extend(['haproxy', 'apache2'])
+        # Staging uses improv, otherwise the API agent is used.
+        if options.get('staging') == 'true':
+            cleanup_services.append('juju-api-improv')
+        else:
+            cleanup_services.append('juju-api-agent')
+    return service_name, unit_info, cleanup_services
+
+
 class DeployTestMixin(object):
 
-    charm = 'juju-gui'
     port = '443'
 
     def setUp(self):
@@ -135,53 +177,35 @@
         services = self.wait_for(services_found, 'Services not displayed.')
         return set([element.text for element in services])
 
-    def juju_deploy(self, *args, **kwargs):
+    def deploy_gui(self, options=None):
         """Shim in our additional cleanup for pyJuju."""
-        # XXX 2012-11-29 frankban bug=872264:
-            # Just invoking ``juju destroy-service juju-gui`` in tearDown
-            # should execute the ``stop`` hook, stopping all the services
-            # started by the charm in the machine. Right now this does not
-            # work in pyJuju, so the desired effect is achieved by keeping
-            # track of started services and manually stopping them here.
         # Once pyJuju works correctly or we drop support for it altogether, we
         # can remove this shim.
-        unit_info, self.service_name = juju_deploy(*args, **kwargs)
+        self.service_name, unit_info, cleanup_services = juju_deploy_gui(
+            options=options)
+        # XXX 2013-11-27 frankban bug=872264:
+            # See juju_deploy_gui above.
         if is_legacy_juju:
             hostname = unit_info['public-address']
-            options = kwargs.get('options', {})
-            # Either stop the builtin server or the old apache2/haproxy setup.
-            if options.get('builtin-server') == 'true':
-                services = ['guiserver']
-            else:
-                services = ['haproxy', 'apache2']
-            # Staging uses improv, otherwise the API agent is used.
-            if options.get('staging') == 'true':
-                services.append('juju-api-improv')
-            else:
-                services.append('juju-api-agent')
-            self.addCleanup(stop_services, hostname, services)
+            self.addCleanup(stop_services, hostname, cleanup_services)
         return unit_info
 
-
-class TestDeploy(DeployTestMixin, unittest.TestCase):
+    def get_builtin_server_info(self, hostname):
+        """Return a dictionary of info as exposed by the builtin server."""
+        url = 'https://{}/gui-server-info'.format(hostname)
+        response = urllib2.urlopen(url)
+        self.assertEqual(200, response.code)
+        return json.load(response)
+
+
+class TestDeployOptions(DeployTestMixin, unittest.TestCase):
 
     def tearDown(self):
         juju_destroy_service(self.service_name)
 
-    def test_local_release(self):
-        # Ensure the Juju GUI and API agent services are correctly set up when
-        # deploying the local release.
-        unit_info = self.juju_deploy(self.charm)
-        hostname = unit_info['public-address']
-        self.navigate_to(hostname)
-        self.handle_browser_warning()
-        self.assertEnvironmentIsConnected()
-
     def test_stable_release(self):
-        # Ensure the Juju GUI and API agent services are correctly set up when
-        # deploying the stable release.
-        options = {'juju-gui-source': 'stable'}
-        unit_info = self.juju_deploy(self.charm, options=options)
+        # Ensure the stable Juju GUI release is correctly set up.
+        unit_info = self.deploy_gui(options={'juju-gui-source': 'stable'})
         hostname = unit_info['public-address']
         self.navigate_to(hostname)
         self.handle_browser_warning()
@@ -189,8 +213,8 @@
 
     @unittest.skipUnless(is_legacy_juju, 'staging only works in pyJuju')
     def test_staging(self):
-        # Ensure the Juju GUI and improv services are correctly set up.
-        unit_info = self.juju_deploy(self.charm, options={'staging': 'true'})
+        # Ensure the Juju GUI and staging services are correctly set up.
+        unit_info = self.deploy_gui(options={'staging': 'true'})
         hostname = unit_info['public-address']
         self.navigate_to(hostname)
         self.handle_browser_warning()
@@ -200,16 +224,19 @@
 
     def test_sandbox(self):
         # The GUI is correctly deployed and set up in sandbox mode.
-        unit_info = self.juju_deploy(self.charm, options={'sandbox': 'true'})
+        unit_info = self.deploy_gui(options={'sandbox': 'true'})
         hostname = unit_info['public-address']
         self.navigate_to(hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
+        # Ensure the builtin server is set up to run in sandbox mode.
+        server_info = self.get_builtin_server_info(hostname)
+        self.assertTrue(server_info['sandbox'])
 
     def test_branch_source(self):
         # Ensure the Juju GUI is correctly deployed from a Bazaar branch.
         options = {'juju-gui-source': JUJU_GUI_TEST_BRANCH}
-        unit_info = self.juju_deploy(self.charm, options=options)
+        unit_info = self.deploy_gui(options=options)
         hostname = unit_info['public-address']
         self.navigate_to(hostname)
         self.handle_browser_warning()
@@ -218,12 +245,11 @@
     def test_legacy_server(self):
         # The legacy apache + haproxy server configuration works correctly.
         # Also make sure the correct cache headers are sent.
-        options = {
-            'builtin-server': False,
-            'juju-gui-source': JUJU_GUI_TEST_BRANCH,
-        }
-        unit_info = self.juju_deploy(self.charm, options=options)
+        unit_info = self.deploy_gui(options={'builtin-server': 'false'})
         hostname = unit_info['public-address']
+        self.navigate_to(hostname)
+        self.handle_browser_warning()
+        self.assertEnvironmentIsConnected()
         conn = httplib.HTTPSConnection(hostname)
         conn.request('HEAD', '/')
         headers = conn.getresponse().getheaders()
@@ -236,34 +262,14 @@
         self.assertIn('public', cache_directives)
         self.assertIn('must-revalidate', cache_directives)
 
-    @unittest.skipIf(is_legacy_juju, 'force-machine only works in juju-core')
-    def test_force_machine(self):
-        # Ensure the Juju GUI is correctly set up in the Juju bootstrap node.
-        unit_info = self.juju_deploy(self.charm, force_machine=0)
-        self.assertEqual('0', unit_info['machine'])
-        self.navigate_to(unit_info['public-address'])
-        self.handle_browser_warning()
-        self.assertEnvironmentIsConnected()
-
-    def test_nrpe_check_available(self):
-        # Make sure the check-app-access.sh script's ADDRESS is available.
-        options = {'juju-gui-source': JUJU_GUI_TEST_BRANCH}
-        unit_info = self.juju_deploy(self.charm, options=options)
-        hostname = unit_info['public-address']
-        conn = httplib.HTTPSConnection(hostname)
-        # This request matches the ADDRESS var in the script.
-        conn.request('GET', '/juju-ui/version.js')
-        message = 'ADDRESS in check-app-access.sh is not accessible.'
-        self.assertEqual(200, conn.getresponse().status, message)
-
-
-class TestBuiltinServer(DeployTestMixin, unittest.TestCase):
+
+class TestBuiltinServerLocalRelease(DeployTestMixin, unittest.TestCase):
 
     @classmethod
     def setUpClass(cls):
         # Deploy the charm. The resulting service is used by all the tests
         # in this test case.
-        unit_info, cls.service_name = juju_deploy(cls.charm)
+        cls.service_name, unit_info, cls.cleanup_services = juju_deploy_gui()
         cls.hostname = unit_info['public-address']
         # The counter is used to produce API request identifiers.
         cls.counter = itertools.count()
@@ -273,10 +279,10 @@
         # Destroy the GUI service, and perform additional clean up in the case
         # we are in a pyJuju environment.
         juju_destroy_service(cls.service_name)
+        # XXX 2013-11-27 frankban bug=872264:
+            # See juju_deploy_gui above.
         if is_legacy_juju:
-            # XXX 2012-11-29 frankban bug=872264:
-            # see DeployTestMixin.juju_deploy above.
-            stop_services(cls.hostname, ['guiserver', 'juju-api-agent'])
+            stop_services(cls.hostname, cls.cleanup_services)
 
     def make_websocket_client(self, authenticated=True):
         """Create and return a WebSocket client connected to the Juju backend.
@@ -297,10 +303,14 @@
         return client
 
     def test_environment_connection(self):
-        # Ensure the Juju GUI and builtin server are correctly set up.
+        # Ensure the Juju GUI and builtin server are correctly set up using
+        # the local release.
         self.navigate_to(self.hostname)
         self.handle_browser_warning()
         self.assertEnvironmentIsConnected()
+        # Ensure the builtin server is set up to be connected to the real env.
+        server_info = self.get_builtin_server_info(self.hostname)
+        self.assertFalse(server_info['sandbox'])
 
     def test_headers(self):
         # Ensure the Tornado headers are correctly sent.
@@ -479,6 +489,14 @@
         statuses = [change['Status'] for change in changes]
         self.assertEqual(['completed', 'completed'], statuses)
 
+    def test_nrpe_check_available(self):
+        # Make sure the check-app-access.sh script's ADDRESS is available.
+        conn = httplib.HTTPSConnection(self.hostname)
+        # This request matches the ADDRESS var in the script.
+        conn.request('GET', '/juju-ui/version.js')
+        message = 'ADDRESS in check-app-access.sh is not accessible.'
+        self.assertEqual(200, conn.getresponse().status, message)
+
 
 if __name__ == '__main__':
     unittest.main(verbosity=2)

=== modified file 'tests/deploy.py'
--- tests/deploy.py	2013-11-26 20:42:52 +0000
+++ tests/deploy.py	2013-11-28 16:43:14 +0000
@@ -20,9 +20,8 @@
 
 
 import json
+import logging
 import os
-import random
-import string
 import tempfile
 
 from charmhelpers import make_charm_config_file
@@ -53,44 +52,46 @@
     return repo
 
 
-SERVICE_NAME_PREFIX = 'service-'
-
-
-def make_service_name():
-    """Generate a long, random service name."""
-    characters = string.ascii_lowercase
-    suffix = ''.join([random.choice(characters) for x in xrange(20)])
-    return SERVICE_NAME_PREFIX + suffix
-
-
 def juju_deploy(
-        charm, options=None, force_machine=None, charm_source=None,
-        series='precise'):
+        charm_name, service_name=None, options=None, force_machine=None,
+        charm_source=None, series='precise'):
     """Deploy and expose the charm. Return the first unit's public address.
 
     Also wait until the service is exposed and the first unit started.
+
+    If service_name is None, use the name of the charm.
     If options are provided, they will be used when deploying the charm.
     If force_machine is not None, create the unit in the specified machine.
     If charm_source is None, dynamically retrieve the charm source directory.
     """
+    # Note: this function is used by both the functional tests and
+    # "make deploy": see the "if main" section below.
     if charm_source is None:
         # Dynamically retrieve the charm source based on the path of this file.
         charm_source = os.path.join(os.path.dirname(__file__), '..')
-    repo = setup_repository(charm, charm_source, series=series)
+    logging.debug('setting up the charms repository')
+    repo = setup_repository(charm_name, charm_source, series=series)
     args = ['deploy', '--repository', repo]
-    service_name = make_service_name()
+    if service_name is None:
+        service_name = charm_name
     if options is not None:
         config_file = make_charm_config_file({service_name: options})
         args.extend(['--config', config_file.name])
     if force_machine is not None:
         args.extend(['--to', str(force_machine)])
-    args.append('local:{}/{}'.format(series, charm))
+    charm_url = 'local:{}/{}'.format(series, charm_name)
+    args.append(charm_url)
     args.append(service_name)
+    logging.debug('deploying {} from the repository in {}'.format(
+        charm_url, repo))
     juju(*args)
+    logging.debug('exposing {}'.format(service_name))
     juju('expose', service_name)
-    return wait_for_unit(service_name), service_name
+    logging.debug('waiting for the unit to be ready')
+    return wait_for_unit(service_name)
 
 
 if __name__ == '__main__':
+    logging.getLogger().setLevel(logging.DEBUG)
     unit = juju_deploy('juju-gui')
     print(json.dumps(unit, indent=2))

=== modified file 'tests/example.py'
--- tests/example.py	2013-11-12 14:51:48 +0000
+++ tests/example.py	2013-11-28 16:43:14 +0000
@@ -24,6 +24,7 @@
     wordpress:
       charm: "cs:precise/wordpress-15"
       num_units: 1
+      to: '0'
       options:
         debug: "no"
         engine: nginx
@@ -67,6 +68,7 @@
     mediawiki:
       charm: "cs:precise/mediawiki-9"
       num_units: 1
+      to: '0'
       options:
         admins: ""
         debug: false

=== modified file 'tests/helpers.py'
--- tests/helpers.py	2013-11-26 17:36:49 +0000
+++ tests/helpers.py	2013-11-28 16:43:14 +0000
@@ -20,7 +20,9 @@
 from functools import wraps
 import json
 import os
+import random
 import re
+import string
 import subprocess
 import time
 
@@ -200,6 +202,13 @@
     return Version._make(map(to_int, match.groups()))
 
 
+def make_service_name(prefix='service-'):
+    """Generate a long, random service name."""
+    characters = string.ascii_lowercase
+    suffix = ''.join([random.choice(characters) for _ in range(20)])
+    return prefix + suffix
+
+
 def stop_services(hostname, services):
     """Stop the given upstart services running on hostname."""
     target = 'ubuntu@{}'.format(hostname)

=== modified file 'tests/test_deploy.py'
--- tests/test_deploy.py	2013-11-26 20:42:52 +0000
+++ tests/test_deploy.py	2013-11-28 16:43:14 +0000
@@ -26,9 +26,7 @@
 import deploy
 from deploy import (
     juju_deploy,
-    make_service_name,
     setup_repository,
-    SERVICE_NAME_PREFIX,
 )
 
 
@@ -128,17 +126,18 @@
     @mock.patch('deploy.wait_for_unit')
     def call_deploy(
             self, mock_wait_for_unit, mock_juju,
-            options=None, force_machine=None, charm_source=None,
-            series='precise'):
+            service_name=None, options=None, force_machine=None,
+            charm_source=None, series='precise'):
         mock_wait_for_unit.return_value = self.unit_info
         if charm_source is None:
             expected_source = os.path.join(os.path.dirname(__file__), '..')
         else:
             expected_source = charm_source
         deploy.setup_repository.reset_mock()
-        unit_info, service_name = juju_deploy(
-            self.charm, options=options, force_machine=force_machine,
-            charm_source=charm_source, series=series)
+        unit_info = juju_deploy(
+            self.charm, service_name=service_name, options=options,
+            force_machine=force_machine, charm_source=charm_source,
+            series=series)
         deploy.setup_repository.assert_called_once_with(
             self.charm, expected_source, series=series)
         # The unit address is correctly returned.
@@ -148,7 +147,7 @@
         juju_calls = mock_juju.call_args_list
         self.assertEqual(2, len(juju_calls))
         # We expect a "juju expose" to have been called on the service.
-        expected_expose_call = mock.call('expose', service_name)
+        expected_expose_call = mock.call('expose', service_name or self.charm)
         deploy_call, expose_call = juju_calls
         self.assertEqual(expected_expose_call, expose_call)
         self.assertEqual(deploy_call[0][0], 'deploy')
@@ -196,13 +195,15 @@
         command = self.call_deploy(series='raring')
         self.assertIn(charm_url, command)
 
+    def test_no_service_name(self):
+        # If the service name is not provided, the charm name is used.
+        command = self.call_deploy()
+        service_name = command[-1]
+        self.assertEqual(self.charm, service_name)
+
     def test_service_name(self):
-        # A random service name is generated for each service started.  That
-        # service name is provided to juju as the last argument.
-        command = self.call_deploy(series='raring')
-        # We'll take it as proof that the last argument is a random service
-        # name if it starts with the service name prefix and is the same
-        # length as another random service name.
+        # A customized service name can be provided and it is passed to Juju as
+        # the last argument.
+        command = self.call_deploy(service_name='my-service')
         service_name = command[-1]
-        self.assertTrue(service_name.startswith(SERVICE_NAME_PREFIX))
-        self.assertEqual(len(service_name), len(make_service_name()))
+        self.assertEqual('my-service', service_name)