Merge lp:~frankban/charms/precise/juju-gui/testing-improvements into lp:~juju-gui/charms/precise/juju-gui/trunk

[Francesco Banconi]

Charm test improvements.

This branch includes some improvements to
the charm testing infrastructure:

Strengthened the Juju implementation check by
introducing a new juju_version helper function.

Do not require the branch to be named "juju-gui"
anymore: the charm can be tested and deployed from
an arbitrary branch.

Updated documentation.

Added the .lbox.check file: now, before
proposing/submitting at least the lint check
and the unit tests must pass.

https://codereview.appspot.com/9714047/

[Francesco Banconi (frankban)]

Reviewers: mp+168673_code.launchpad.net,

Message:
Please take a look.

Description:
Charm test improvements.

This branch includes some improvements to
the charm testing infrastructure:

Strengthened the Juju implementation check by
introducing a new juju_version helper function.

Do not require the branch to be named "juju-gui"
anymore: the charm can be tested and deployed from
an arbitrary branch.

Updated documentation.

Added the .lbox.check file: now, before
proposing/submitting at least the lint check
and the unit tests must pass.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/testing-improvements/+merge/168673

(do not edit description out of merge proposal)

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

Affected files:
   A .lbox.check
   M HACKING.md
   A [revision details]
   M tests/20-functional.test
   M tests/deploy.py
   M tests/helpers.py
   M tests/requirements.pip
   M tests/test_deploy.py
   M tests/test_helpers.py

[lp:~frankban/charms/precise/juju-gui/testing-improvements]

76. By Francesco Banconi on 2013-06-11

Bump revision up.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/9714047/

[Nicola Larosa (teknico)]

LGTM, thanks for this. Nice tests as usual. A few trivials below.

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

https://codereview.appspot.com/9714047/diff/3001/tests/20-functional.test#newcode74
tests/20-functional.test:74: def wait_for(self, condition, error=None,
timeout=30):
Maybe a constant at top of file for all three default timeout values?

https://codereview.appspot.com/9714047/diff/3001/tests/helpers.py
File tests/helpers.py (right):

https://codereview.appspot.com/9714047/diff/3001/tests/helpers.py#newcode137
tests/helpers.py:137: except subprocess.CalledProcessError:
This means it's goJuju, right? A comment to that effect?

https://codereview.appspot.com/9714047/diff/3001/tests/helpers.py#newcode142
tests/helpers.py:142: toint = lambda num: 0 if num is None else int(num)
This looks like a mispelling of "taint". :-) "to_int" maybe?

https://codereview.appspot.com/9714047/diff/3001/tests/requirements.pip
File tests/requirements.pip (right):

https://codereview.appspot.com/9714047/diff/3001/tests/requirements.pip#newcode1
tests/requirements.pip:1: # Juju GUI test requirements.
Oh, you can put comments in here? Good to know. :-)

https://codereview.appspot.com/9714047/diff/3001/tests/test_helpers.py
File tests/test_helpers.py (right):

https://codereview.appspot.com/9714047/diff/3001/tests/test_helpers.py#newcode209
tests/test_helpers.py:209: def test_not_semantic(self,
mock_check_output):
Maybe a more explicit "test_not_semantic_versioning"?

https://codereview.appspot.com/9714047/

[lp:~frankban/charms/precise/juju-gui/testing-improvements]

77. By Francesco Banconi on 2013-06-11

Fix make lint.

[Francesco Banconi (frankban)]

Please take a look.

https://codereview.appspot.com/9714047/

[j.c.sackett (jcsackett)]

On 2013/06/11 14:58:05, frankban wrote:
> Please take a look.

LGTM.

I have nothing to add, though I do agree wtih teknico's suggestion of
using a constant for the default timeout value in 20-functional.test

https://codereview.appspot.com/9714047/

[lp:~frankban/charms/precise/juju-gui/testing-improvements]

78. By Francesco Banconi on 2013-06-12

Changes as per review.

79. By Francesco Banconi on 2013-06-12

Merged trunk and resolved conflicts.

[Francesco Banconi (frankban)]

*** Submitted:

Charm test improvements.

This branch includes some improvements to
the charm testing infrastructure:

Strengthened the Juju implementation check by
introducing a new juju_version helper function.

Do not require the branch to be named "juju-gui"
anymore: the charm can be tested and deployed from
an arbitrary branch.

Updated documentation.

Added the .lbox.check file: now, before
proposing/submitting at least the lint check
and the unit tests must pass.

R=teknico, j.c.sackett
CC=
https://codereview.appspot.com/9714047

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

https://codereview.appspot.com/9714047/diff/3001/tests/20-functional.test#newcode74
tests/20-functional.test:74: def wait_for(self, condition, error=None,
timeout=30):
On 2013/06/11 14:38:40, teknico wrote:
> Maybe a constant at top of file for all three default timeout values?

As discussed on IRC, those values are only incidentally the same, so
making a module level constant doesn't give us a lot.

https://codereview.appspot.com/9714047/diff/3001/tests/helpers.py
File tests/helpers.py (right):

https://codereview.appspot.com/9714047/diff/3001/tests/helpers.py#newcode137
tests/helpers.py:137: except subprocess.CalledProcessError:
On 2013/06/11 14:38:40, teknico wrote:
> This means it's goJuju, right? A comment to that effect?

Done.

https://codereview.appspot.com/9714047/diff/3001/tests/helpers.py#newcode142
tests/helpers.py:142: toint = lambda num: 0 if num is None else int(num)
On 2013/06/11 14:38:40, teknico wrote:
> This looks like a mispelling of "taint". :-) "to_int" maybe?

Done.

https://codereview.appspot.com/9714047/diff/3001/tests/test_helpers.py
File tests/test_helpers.py (right):

https://codereview.appspot.com/9714047/diff/3001/tests/test_helpers.py#newcode209
tests/test_helpers.py:209: def test_not_semantic(self,
mock_check_output):
On 2013/06/11 14:38:40, teknico wrote:
> Maybe a more explicit "test_not_semantic_versioning"?

Done.

https://codereview.appspot.com/9714047/

[Francesco Banconi (frankban)]

Thank you both for the reviews!

https://codereview.appspot.com/9714047/

=== added file '.lbox.check'
--- .lbox.check	1970-01-01 00:00:00 +0000
+++ .lbox.check	2013-06-12 09:11:27 +0000
@@ -0,0 +1,3 @@
+#!/bin/sh
+
+make lint unittest

=== modified file 'HACKING.md'
--- HACKING.md	2013-06-11 14:04:04 +0000
+++ HACKING.md	2013-06-12 09:11:27 +0000
@@ -43,11 +43,6 @@
 At this point, link "juju-plugins/plugins/juju_test.py" as "juju-test"
 somewhere in your PATH, so that it is possible to execute "juju-test".
 
-Our testing infrastructure requires the current directory name to 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
-
 Before being able to run the suite, test requirements need to be installed
 running the command:
 

=== modified file 'Makefile'
--- Makefile	2013-06-11 14:13:45 +0000
+++ Makefile	2013-06-12 09:11:27 +0000
@@ -36,7 +36,7 @@
 	$(JUJUTEST)
 
 lint: setup
-	@flake8 --show-source --exclude=.venv  ./hooks/ ./tests/
+	@$(VENV)/bin/flake8 --show-source --exclude=.venv  ./hooks/ ./tests/
 
 clean:
 	find . -name '*.pyc' -delete

=== modified file 'revision'
--- revision	2013-06-12 08:59:15 +0000
+++ revision	2013-06-12 09:11:27 +0000
@@ -1,1 +1,1 @@
-52
+53

=== modified file 'tests/20-functional.test'
--- tests/20-functional.test	2013-06-11 14:04:04 +0000
+++ tests/20-functional.test	2013-06-12 09:11:27 +0000
@@ -26,14 +26,14 @@
 from deploy import juju_deploy
 from helpers import (
     juju_destroy_service,
-    legacy_juju,
+    juju_version,
     ssh,
 )
 
 
 JUJU_GUI_TEST_BRANCH = 'lp:~juju-gui/juju-gui/charm-tests-branch'
 STAGING_SERVICES = ('haproxy', 'mediawiki', 'memcached', 'mysql', 'wordpress')
-is_legacy_juju = legacy_juju()
+is_legacy_juju = juju_version().major == 0
 
 
 class DeployTestMixin(object):
@@ -56,7 +56,7 @@
     def assertEnvironmentIsConnected(self):
         """Assert the GUI environment is connected to the Juju API agent."""
         self.wait_for_script(
-            'return app.env.get("connected");',
+            'return app && app.env && app.env.get("connected");',
             error='Environment not connected.')
 
     def handle_browser_warning(self):
@@ -87,7 +87,7 @@
             return driver.title == 'Juju Admin'
         self.wait_for(page_ready, error='Juju GUI not found.')
 
-    def wait_for(self, condition, error=None, timeout=20):
+    def wait_for(self, condition, error=None, timeout=30):
         """Wait for condition to be True.
 
         The argument condition is a callable accepting a driver object.
@@ -97,7 +97,7 @@
         wait = ui.WebDriverWait(self.selenium, timeout)
         return wait.until(condition, error)
 
-    def wait_for_css_selector(self, selector, error=None, timeout=20):
+    def wait_for_css_selector(self, selector, error=None, timeout=30):
         """Wait until the provided CSS selector is found.
 
         Fail printing the provided error if timeout is exceeded.
@@ -108,7 +108,7 @@
         elements = self.wait_for(condition, error=error, timeout=timeout)
         return elements[0]
 
-    def wait_for_script(self, script, error=None, timeout=20):
+    def wait_for_script(self, script, error=None, timeout=30):
         """Wait for the given JavaScript snippet to return a True value.
 
         Fail printing the provided error if timeout is exceeded.

=== modified file 'tests/deploy.py'
--- tests/deploy.py	2013-06-11 14:04:04 +0000
+++ tests/deploy.py	2013-06-12 09:11:27 +0000
@@ -33,23 +33,25 @@
 rsync = command('rsync', '-a', '--exclude', '.bzr', '--exclude', '.venv')
 
 
-def setup_repository(source, series='precise'):
+def setup_repository(name, source, series='precise'):
     """Create a temporary Juju repository to use for charm deployment.
 
-    Copy the charm files in source in the precise repository section, excluding
-    the virtualenv and Bazaar directories.
+    Copy the charm files in source in the precise repository section, using the
+    provided charm name and excluding the virtualenv and Bazaar directories.
 
     Return the repository path.
     """
-    source = os.path.abspath(source)
+    source = os.path.abspath(source) + os.path.sep
     repo = tempfile.mkdtemp()
-    destination = os.path.join(repo, series)
+    destination = os.path.join(repo, series, name)
     os.makedirs(destination)
     rsync(source, destination)
     return repo
 
 
-def juju_deploy(charm, options=None, force_machine=None, charm_source=None):
+def juju_deploy(
+        charm, 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.
@@ -60,14 +62,14 @@
     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_source)
+    repo = setup_repository(charm, charm_source, series=series)
     args = ['deploy', '--repository', repo]
     if options is not None:
         config_file = make_charm_config_file({charm: options})
         args.extend(['--config', config_file.name])
     if force_machine is not None:
         args.extend(['--force-machine', str(force_machine)])
-    args.append('local:{0}'.format(charm))
+    args.append('local:{}/{}'.format(series, charm))
     juju(*args)
     juju('expose', charm)
     return wait_for_unit(charm)

=== modified file 'tests/helpers.py'
--- tests/helpers.py	2013-06-11 14:04:04 +0000
+++ tests/helpers.py	2013-06-12 09:11:27 +0000
@@ -16,9 +16,11 @@
 
 """Juju GUI test helpers."""
 
+from collections import namedtuple
 from functools import wraps
 import json
 import os
+import re
 import subprocess
 import time
 
@@ -71,15 +73,7 @@
 juju_command = command('juju')
 juju_env = lambda: os.getenv('JUJU_ENV')  # This is propagated by juju-test.
 ssh = command('ssh')
-
-
-def legacy_juju():
-    """Return True if pyJuju is being used, False otherwise."""
-    try:
-        juju_command('--version')
-    except ProcessError:
-        return False
-    return True
+Version = namedtuple('Version', 'major minor patch')
 
 
 def retry(exception, tries=10, delay=1):
@@ -136,6 +130,36 @@
     return json.loads(status)
 
 
+_juju_version_expression = re.compile(r"""
+    ^  # Beginning of line.
+    (?:juju\s+)?  # Optional juju prefix.
+    (\d+)\.(\d+)  # Major and minor versions.
+    (?:\.(\d+))?  # Optional patch version.
+    .*  # Optional suffix.
+    $  # End of line.
+""", re.VERBOSE)
+
+
+def juju_version():
+    """Return the currently used Juju version.
+
+    The version is returned as a named tuple (major, minor, patch).
+    If the patch number is missing, it is set to zero.
+    """
+    try:
+        # In pyJuju, version info is printed to stderr.
+        output = subprocess.check_output(
+            ['juju', '--version'], stderr=subprocess.STDOUT)
+    except subprocess.CalledProcessError:
+        # Current juju-core exposes a version subcommand.
+        output = subprocess.check_output(['juju', 'version'])
+    match = _juju_version_expression.match(output)
+    if match is None:
+        raise ValueError('invalid juju version: {!r}'.format(output))
+    to_int = lambda num: 0 if num is None else int(num)
+    return Version._make(map(to_int, match.groups()))
+
+
 def wait_for_unit(sevice):
     """Wait for the first unit of the given service to be started.
 

=== modified file 'tests/requirements.pip'
--- tests/requirements.pip	2013-06-11 14:43:19 +0000
+++ tests/requirements.pip	2013-06-12 09:11:27 +0000
@@ -1,3 +1,5 @@
+# Juju GUI test requirements.
+
 # This file is part of the Juju GUI, which lets users view and manage Juju
 # environments within a graphical interface (https://launchpad.net/juju-gui).
 # Copyright (C) 2012-2013 Canonical Ltd.

=== modified file 'tests/test_deploy.py'
--- tests/test_deploy.py	2013-06-11 14:04:04 +0000
+++ tests/test_deploy.py	2013-06-12 09:11:27 +0000
@@ -31,10 +31,11 @@
 
 class TestSetupRepository(unittest.TestCase):
 
+    name = 'test-charm'
+
     def setUp(self):
         # Create a directory structure for the charm source.
         self.source = tempfile.mkdtemp()
-        self.charm_name = os.path.basename(self.source)
         self.addCleanup(shutil.rmtree, self.source)
         # Create a file in the source dir.
         _, self.root_file = tempfile.mkstemp(dir=self.source)
@@ -75,24 +76,24 @@
         series_dir = os.path.join(repo, series)
         self.assert_dir_exists(series_dir)
         # The series directory only contains our charm.
-        self.assertEqual([self.charm_name], os.listdir(series_dir))
-        self.assert_dir_exists(os.path.join(series_dir, self.charm_name))
+        self.assertEqual([self.name], os.listdir(series_dir))
+        self.assert_dir_exists(os.path.join(series_dir, self.name))
 
     def test_repository(self):
         # The charm repository is correctly created with the default series.
-        repo = setup_repository(self.source)
+        repo = setup_repository(self.name, self.source)
         self.check_repository(repo, 'precise')
 
     def test_series(self):
         # The charm repository is created with the given series.
-        repo = setup_repository(self.source, series='raring')
+        repo = setup_repository(self.name, self.source, series='raring')
         self.check_repository(repo, 'raring')
 
     def test_charm_files(self):
         # The charm files are correctly copied inside the repository, excluding
         # unwanted directories.
-        repo = setup_repository(self.source)
-        charm_dir = os.path.join(repo, 'precise', self.charm_name)
+        repo = setup_repository(self.name, self.source)
+        charm_dir = os.path.join(repo, 'precise', self.name)
         test_dir_name = os.path.basename(self.tests_dir)
         expected = set([
             os.path.basename(self.root_file),
@@ -107,7 +108,7 @@
     unit_info = {'public-address': 'unit.example.com'}
     charm = 'test-charm'
     expose_call = mock.call('expose', charm)
-    local_charm = 'local:{}'.format(charm)
+    local_charm = 'local:precise/{}'.format(charm)
     repo = '/tmp/repo/'
 
     @mock.patch('deploy.juju')
@@ -115,13 +116,19 @@
     @mock.patch('deploy.setup_repository')
     def call_deploy(
             self, mock_setup_repository, mock_wait_for_unit, mock_juju,
-            **kwargs):
+            options=None, force_machine=None, charm_source=None,
+            series='precise'):
         mock_setup_repository.return_value = self.repo
         mock_wait_for_unit.return_value = self.unit_info
-        charm_source = kwargs.setdefault(
-            'charm_source', os.path.join(os.path.dirname(__file__), '..'))
-        unit_info = juju_deploy(self.charm, **kwargs)
-        mock_setup_repository.assert_called_once_with(charm_source)
+        if charm_source is None:
+            expected_source = os.path.join(os.path.dirname(__file__), '..')
+        else:
+            expected_source = charm_source
+        unit_info = juju_deploy(
+            self.charm, options=options, force_machine=force_machine,
+            charm_source=charm_source, series=series)
+        mock_setup_repository.assert_called_once_with(
+            self.charm, expected_source, series=series)
         # The unit address is correctly returned.
         self.assertEqual(self.unit_info, unit_info)
         self.assertEqual(1, mock_wait_for_unit.call_count)
@@ -177,3 +184,13 @@
         )
         deploy_call = self.call_deploy(charm_source='/tmp/source/')
         self.assertEqual(expected_deploy_call, deploy_call)
+
+    def test_series(self):
+        # The function can deploy a charm from a specific series.
+        expected_deploy_call = mock.call(
+            'deploy',
+            '--repository', self.repo,
+            'local:raring/{}'.format(self.charm)
+        )
+        deploy_call = self.call_deploy(series='raring')
+        self.assertEqual(expected_deploy_call, deploy_call)

=== modified file 'tests/test_helpers.py'
--- tests/test_helpers.py	2013-06-11 14:04:04 +0000
+++ tests/test_helpers.py	2013-06-12 09:11:27 +0000
@@ -17,6 +17,7 @@
 """Juju GUI helpers tests."""
 
 import json
+import subprocess
 import unittest
 
 import mock
@@ -27,9 +28,10 @@
     juju_destroy_service,
     juju_env,
     juju_status,
-    legacy_juju,
+    juju_version,
     ProcessError,
     retry,
+    Version,
     wait_for_unit,
 )
 
@@ -193,20 +195,72 @@
         mock_juju.assert_called_once_with('status', '--format', 'json')
 
 
[e-mail]('helpers.juju_command')
-class TestLegacyJuju(unittest.TestCase):
-
-    def test_pyjuju(self, mock_juju_command):
-        # Legacy Juju is correctly recognized.
-        mock_juju_command.return_value = '0.7.0'
-        self.assertTrue(legacy_juju())
-        mock_juju_command.assert_called_once_with('--version')
-
-    def test_juju_core(self, mock_juju_command):
-        # juju-core is correctly recognized.
-        mock_juju_command.side_effect = ProcessError(1, 'failed', '', '')
-        self.assertFalse(legacy_juju())
-        mock_juju_command.assert_called_once_with('--version')
[e-mail]('subprocess.check_output')
+class TestJujuVersion(unittest.TestCase):
+
+    error = subprocess.CalledProcessError(2, 'invalid flag', 'output')
+
+    def test_pyjuju(self, mock_check_output):
+        # The pyJuju version is correctly retrieved.
+        mock_check_output.return_value = '0.7.2'
+        version = juju_version()
+        self.assertEqual(Version(0, 7, 2), version)
+        mock_check_output.assert_called_once_with(
+            ['juju', '--version'], stderr=subprocess.STDOUT,
+        )
+
+    def test_juju_core(self, mock_check_output):
+        # The juju-core version is correctly retrieved.
+        mock_check_output.side_effect = (self.error, '1.12.3')
+        version = juju_version()
+        self.assertEqual(Version(1, 12, 3), version)
+        self.assertEqual(2, mock_check_output.call_count)
+        first_call, second_call = mock_check_output.call_args_list
+        self.assertEqual(
+            mock.call(['juju', '--version'], stderr=subprocess.STDOUT),
+            first_call,
+        )
+        self.assertEqual(mock.call(['juju', 'version']), second_call)
+
+    def test_not_semantic_versioning(self, mock_check_output):
+        # If the patch number is missing, it is set to zero.
+        mock_check_output.return_value = '0.7'
+        version = juju_version()
+        self.assertEqual(Version(0, 7, 0), version)
+
+    def test_prefix(self, mock_check_output):
+        # The function handles versions returned as "juju x.y.z".
+        mock_check_output.return_value = 'juju 0.8.3'
+        version = juju_version()
+        self.assertEqual(Version(0, 8, 3), version)
+
+    def test_suffix(self, mock_check_output):
+        # The function handles versions returned as "x.y.z-series-arch".
+        mock_check_output.return_value = '1.10.3-raring-amd64'
+        version = juju_version()
+        self.assertEqual(Version(1, 10, 3), version)
+
+    def test_all(self, mock_check_output):
+        # Additional information is correctly handled.
+        mock_check_output.side_effect = (self.error, 'juju 1.234-precise-i386')
+        version = juju_version()
+        self.assertEqual(Version(1, 234, 0), version)
+        self.assertEqual(2, mock_check_output.call_count)
+
+    def test_invalid_version(self, mock_check_output):
+        # A ValueError is raised if the returned version is not valid.
+        mock_check_output.return_value = '42'
+        with self.assertRaises(ValueError) as info:
+            juju_version()
+        self.assertEqual("invalid juju version: '42'", str(info.exception))
+
+    def test_failure(self, mock_check_output):
+        # A CalledProcessError is raised if the Juju version cannot be found.
+        mock_check_output.side_effect = (self.error, self.error)
+        with self.assertRaises(subprocess.CalledProcessError) as info:
+            juju_version()
+        self.assertIs(self.error, info.exception)
+        self.assertEqual(2, mock_check_output.call_count)
 
 
 class TestProcessError(unittest.TestCase):