Merge lp:~cprov/uci-engine/charm-tests into lp:uci-engine

[Celso Providelo]

Fixing project test runner to load and run charm tests properly.

Fixing project test runner to load and run charm tests properly.

On the bright side, there was no test failures per se, since there were probably run by developers via local Makefile. We have a 45/45, so DON'T PANIC!

It was required to create a custom test loader, so the tests with the same relative path (standard) don't shadow the next ones; and also a custom test suite that is capable of reloading 'hooks' and 'charmhelpers' module before running tests for each charm (suite really, it does more reloads than necessary :-/).

It's unfortunate that such a simple problem (aggregating tests) implies in that much complexity encoded in testing/run_tests.py ...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:813
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1483/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1483/rebuild

review: Approve (continuous-integration)

[Para Siva (psivaa)]

I keep getting the following trace on with and without a deployment. (both with novarc file sourced.)

Traceback (most recent call last):
  File "./run-tests", line 32, in <module>
    retval = run_tests.main(sys.argv[1:], sys.stdout, sys.stderr)
  File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line 565, in main
    options.include_regexps, options.exclude_regexps)
  File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line 448, in load_charm_tests
    suite.setCharmRoots()
  File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line 356, in setCharmRoots
    s.charm_root = '/'.join(flat_tests[0].id().split('.')[:3])
IndexError: list index out of range

Marking the MP, needs fixing. Please feel free to ignore if i'm doing abs stupid :)

review: Needs Fixing

[Joe Talbott (joetalbott)]

LGTM with one minor nit.

review: Approve

[Vincent Ladeuil (vila)]

There are many issues with the MP, the main ones are:
- it breaks --list

- it uses imp.load_module() whose doc says: "This function does more than importing the module: if the module was already imported, it is equivalent to a reload()!" (The question mark is theirs). See https://docs.python.org/2/library/functions.html?highlight=reload#reload and the endless list of issues associated with that function.

There is no way we can trust reload(), it's meant to be used interactively with a full understanding of the fallouts.

The root issue that you're pointing out here is that we're using hooks.py to mean different things, python doesn't allow that. So that's what need to be changed.

I had a look at it and the second similar issue is that we *also* do 'import charmhelpers' with various definitions across charms and also try to convince python that they are all the same. This one is slightly simpler to fix: as a project, we should use the *same* version in all charms (we do use the same revno but build a different python package via different charm-helpers.yaml files).

I think it's time to stop using bigger hammers that do more harm than good, step back to the original decision to rely on 'import hooks' and fix that.

review: Needs Fixing

[lp:~cprov/uci-engine/charm-tests]

814. By Celso Providelo on 2014-09-29

Addressing review comments.

[Celso Providelo (cprov)]

Sivaa,

Thanks for pointing this problem, it only happens when you run run-tests without filters (an empty test suite for charm tests shows up mysteriously). Fix added.

> I keep getting the following trace on with and without a deployment. (both
> with novarc file sourced.)
>
> Traceback (most recent call last):
> File "./run-tests", line 32, in <module>
> retval = run_tests.main(sys.argv[1:], sys.stdout, sys.stderr)
> File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line
> 565, in main
> options.include_regexps, options.exclude_regexps)
> File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line
> 448, in load_charm_tests
> suite.setCharmRoots()
> File "/home/psivaa/temp/code-reviews/charm-tests/testing/run_tests.py", line
> 356, in setCharmRoots
> s.charm_root = '/'.join(flat_tests[0].id().split('.')[:3])
> IndexError: list index out of range
>
>
> Marking the MP, needs fixing. Please feel free to ignore if i'm doing abs
> stupid :)

[Celso Providelo (cprov)]

Joe,

I've added an explanation for the line calculating the path from the mangled test id. Thanks for pointing it.

> LGTM with one minor nit.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:814
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1491/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/1491/rebuild

review: Approve (continuous-integration)

[Celso Providelo (cprov)]

Vila,

Thanks for your points, let's discuss them specifically.

> There are many issues with the MP, the main ones are:
> - it breaks --list

--list was probably broken because the problem with running run-test w/o filters, it's fixed now.

> - it uses imp.load_module() whose doc says: "This function does more than
> importing the module: if the module was already imported, it is equivalent to
> a reload()!" (The question mark is theirs). See
> https://docs.python.org/2/library/functions.html?highlight=reload#reload and
> the endless list of issues associated with that function.
>
> There is no way we can trust reload(), it's meant to be used interactively
> with a full understanding of the fallouts.
>
> The root issue that you're pointing out here is that we're using hooks.py to
> mean different things, python doesn't allow that. So that's what need to be
> changed.
>
> I had a look at it and the second similar issue is that we *also* do 'import
> charmhelpers' with various definitions across charms and also try to convince
> python that they are all the same. This one is slightly simpler to fix: as a
> project, we should use the *same* version in all charms (we do use the same
> revno but build a different python package via different charm-helpers.yaml
> files).

Charms are designed to be standalone projects, they just happen to be in the same tree because it's convenient to us, the same goes for running their tests inside a single *super-project* test suite, it's *just* way more convenient than spawning isolated `make check`s and collect results. That said, in order to keep the wanted benefits, UCI-E has to compromise.

We are all aware of the risks involved in reloading modules, but if it is what it takes to assure charm tests (and only them) are run as part of our test suite, so be it. And by all means, I agree with you on keeping this hack as isolated and instrumented as possible, so we will notice if it is behaving badly, if it ever does.

> I think it's time to stop using bigger hammers that do more harm than good,
> step back to the original decision to rely on 'import hooks' and fix that.

It sounds good, but `import hooks` & `import charmhelpers` are not going to change, they make all sense at the charm-level; and we have to ensure we run charm tests in tarmac, today, not tomorrow.

Moreover, It's not a *big hammer*, since it's well isolated (custom loaders and suite are only used for charm tests) and the behaviour can be entirely overridden by tweaking 2 lines, if we find real, not hypothetical, evidences that it's not doing what it is supposed to do.

[Francis Ginther (fginther)]

Here's a bit of a strawman. Is it time move some of these charms out of the lp:uci-engine source tree? Our original intent was to do this once the level of change had dropped to an acceptable level. This is not going to solve the problem, but maybe we just avoid it or lessen the risks.

Even if this isn't the right solution to solve this specific problem, is it time to remove one or more of these from the source? We should be able to do some simply analysis to see which charms have had the fewest recent updates.

[Vincent Ladeuil (vila)]

> Charms are designed to be standalone projects, they just happen to be in the
> same tree because it's convenient to us,

Components are also designed to be stand alone, we know we have issues there
around the overall project layout but we've already taken some steps to
ensure they use different parts of the python name space so their code can
be loaded in a single python process.

It's not about being convenient it's just about using python and respecting
the language definition.

> the same goes for running their tests
> inside a single *super-project* test suite, it's *just* way more convenient
> than spawning isolated `make check`s and collect results.

... and allowing test listing/selection and displaying the timings and being
able to use subunit as an output and being able to run them concurrently

> That said, in order
> to keep the wanted benefits, UCI-E has to compromise.

Says who ?

There *is* a valid fix which is to fix the charms to use their own name space like we do for the components.

>
> We are all aware of the risks involved in reloading modules,

I have no idea about how tricking python into believing it can import
different modules in the same name space spot can manifest itself or mask
issues, that's the whole point.

> but if it is what
> it takes to assure charm tests (and only them) are run as part of our test
> suite, so be it. And by all means, I agree with you on keeping this hack as
> isolated and instrumented as possible, so we will notice if it is behaving
> badly, if it ever does.
>
> > I think it's time to stop using bigger hammers that do more harm than good,
> > step back to the original decision to rely on 'import hooks' and fix that.
>
> It sounds good, but `import hooks` & `import charmhelpers` are not going to
> change,

Of course they can change, I already have one charm fixed to define its own
name space and fix the root issue.

> they make all sense at the charm-level; and we have to ensure we run
> charm tests in tarmac, today, not tomorrow.

The charms should be fixed, not run-tests, changing python semantics is not
run-tests job.

>
> Moreover, It's not a *big hammer*, since it's well isolated (custom loaders
> and suite are only used for charm tests) and the behaviour can be entirely
> overridden by tweaking 2 lines,

Your patch is already ~300 lines long, that's far from 2 lines.

> if we find real, not hypothetical, evidences
> that it's not doing what it is supposed to do.

The real evidence is that python can't load different modules under the same
name.

That's a real issue, perfectly clear and understood. I see no point in
tricking python when there is a way to use it properly.

[Evan (ev)]

We have always said that charms should be stand-alone and not depend on behaviour that only exists in lp:uci-engine. I think by putting the namespace of the charm in the filename, we move away from that. Pretty much every other charm-helpers using charm has hooks/hooks.py and now we have hooks/wsgi_app_hooks.py.

I also think the risks that Vincent highlights are very real and if they were to bite us, would be extremely hard to pinpoint.

I am almost inclined to agree with Francis that it's time to split the charms out into their own branches. We understand the various pieces of this project enough now to be able to accomplish that without branch landing being delicate. However, it's no small amount of work. Each charm requires a new stanza in Tarmac, a new branch for everyone on the team to watch MPs on, and so on.

I am also convinced that it greatly reduces visibility on these ancillary branches. It means that we sacrifice consistency, which already needs to be carefully managed in a microservices architecture.

Instead, I'd like to propose that we isolate the process environment for the charm tests. We fork a child process under the charm directory and run through the tests in complete isolation. The parent process can take the piped output from the child and combine it with its own results.

We've got more opinions on this one than we have people. Let's take it down a level and understand that there are tradeoffs with all approaches.

[Para Siva (psivaa)]

Thanks Celso for the fix to handle empty suite. Approving before disappearing for the day.

review: Approve

[Celso Providelo (cprov)]

Evan and Francis,

I have no arguments against your proposal for spawning `make check` on charms as a clean transition to move them away from our tree.

It is certainly a good ending-point to this polarised discussion full of hypothetical problems and no concrete solutions.

Any arguments against running charms tests in called_by_tarmac.py before deploying the new environment ? (users would chdir into charms and run `make check`)

[Joe Talbott (joetalbott)]

On Tue, Sep 30, 2014 at 12:52:38PM -0000, Celso Providelo wrote:
> Evan and Francis,
>
> I have no arguments against your proposal for spawning `make check` on charms as a clean transition to move them away from our tree.
>
> It is certainly a good ending-point to this polarised discussion full of hypothetical problems and no concrete solutions.
>
> Any arguments against running charms tests in called_by_tarmac.py before deploying the new environment ? (users would chdir into charms and run `make check`)

I am strongly against changing our charms to make them work with
uci-engine. I am also strongly in favor of getting our charms out of
our uci-engine branch. I don't think tracking several branches for MPs
is too much effort.

I am in support of running charm tests in isolation until they can be
put into separate branches.

Joe

Unmerged revisions

814. By Celso Providelo on 2014-09-29

Addressing review comments.

813. By Celso Providelo on 2014-09-27

Using a custom test suite for charms tests for reloding hooks & charmhelpers before running tests.

812. By Celso Providelo on 2014-09-27

Fix wsgi-app tests to import modules in the same way the other charms are doing (top-level 'hooks' module).

811. By Celso Providelo on 2014-09-26

Override ucitests.loaders.Loader test import behavior to cope with our charms default testing topology (<charm>/unit_tests/*.py).

810. By Vincent Ladeuil on 2014-09-26

Rename the test files to make them unique in the unit_tests namespace, force unit_tests to be a package with multiple directories so all the tests can be properly loaded.

Outcome: 36 failures for 45 charm tests which is better than 8 passing tests (which in turn means all but one hidden charm tests are broken :-/)

=== modified file 'charms/precise/wsgi-app/unit_tests/test_hooks.py'
--- charms/precise/wsgi-app/unit_tests/test_hooks.py	2014-09-25 20:38:15 +0000
+++ charms/precise/wsgi-app/unit_tests/test_hooks.py	2014-09-29 18:45:52 +0000
@@ -21,8 +21,8 @@
 
 import yaml
 
-from intercom import hooks as intercom_hooks
-from hooks import hooks
+import hooks
+import intercom
 
 
 HERE = os.path.abspath(os.path.dirname(__file__))
@@ -135,13 +135,13 @@
         self.revno = 'r1234_foo'
 
     def test_framework_restish(self):
-        hooks.execute(['hooks/install'])
+        hooks.hooks.execute(['hooks/install'])
         pkgs = self.apt_install.call_args_list[0][0][0]
         self.assertIn('python-restish', pkgs)
 
     def test_framework_pyramid(self):
         self.config.return_value['framework'] = 'pyramid'
-        hooks.execute(['hooks/install'])
+        hooks.hooks.execute(['hooks/install'])
         pkgs = self.apt_install.call_args_list[0][0][0]
         self.assertIn('python3-pyramid', pkgs)
 
@@ -194,7 +194,7 @@
         cron_file_path = os.path.join(self._cron_root(), 'local-test')
         with open(cron_file_path, 'w') as fd:
             fd.write('removed')
-        hooks.execute(['hooks/config-changed'])
+        hooks.hooks.execute(['hooks/config-changed'])
         self.assertEqual([], os.listdir(self._cron_root()))
         self.service_restart.assert_called_once_with('cron')
 
@@ -203,7 +203,7 @@
         self.config.return_value['cron_schedule'] = '*/10 * * * *'
         self.config.return_value['cron_cmd'] = 'foo.sh'
         self.config.return_value['wsgi_user'] = 'bar'
-        hooks.execute(['hooks/config-changed'])
+        hooks.hooks.execute(['hooks/config-changed'])
         self.assertEqual(['local-test'], os.listdir(self._cron_root()))
         code_dir = os.path.join(self.tmpdir, 'code')
         logs_dir = os.path.join(self.tmpdir, 'logs')
@@ -221,7 +221,7 @@
         self.service_restart.assert_called_once_with('cron')
 
     def test_allowed_hosts(self):
-        hooks.execute(['hooks/config-changed'])
+        hooks.hooks.execute(['hooks/config-changed'])
         self.assertTrue(
             os.path.exists(os.path.join(self.tmpdir, 'allowed_hosts.json')))
 
@@ -265,7 +265,7 @@
     def test_intercom_changed(self):
         # New 'intercom' relations are stored in the JSON registry
         # as <hostname>:<port>
-        intercom_hooks.execute(['hooks/intercom-relation-changed'])
+        intercom.hooks.execute(['hooks/intercom-relation-changed'])
         self.log.assert_called_with(
             "Intercom unit 'remote-test' (remote.com) joined.")
         self.assertEquals(
@@ -285,7 +285,7 @@
              '2': {'hostname': '10.0.0.2',
                    'port': '9000',
                    'unit': 'preserved'}})
-        intercom_hooks.execute(['hooks/intercom-relation-broken'])
+        intercom.hooks.execute(['hooks/intercom-relation-broken'])
         self.log.assert_called_with("Intercom unit '' () departed.")
         self.assertEquals(
             {'2': {'hostname': '10.0.0.2',
@@ -314,7 +314,7 @@
 class TestWSGIHooks(RestishTestCase):
 
     def test_wsgi_joined(self):
-        hooks.execute(['hooks/wsgi-relation-joined'])
+        hooks.hooks.execute(['hooks/wsgi-relation-joined'])
         self.open_port.assert_called_once_with(8080)
         expected = {
             'python_path': '',
@@ -329,7 +329,7 @@
             '/srv/local-test/logs', 'www-data', 'www-data', 0755)
 
     def test_wsgi_broken(self):
-        hooks.execute(['hooks/wsgi-relation-broken'])
+        hooks.hooks.execute(['hooks/wsgi-relation-broken'])
         self.close_port.assert_called_once_with(8080)
 
 
@@ -337,7 +337,7 @@
 
     def test_json_status_joined(self):
         self.config.return_value['json_status_path'] = 'foo'
-        hooks.execute(['hooks/json_status-relation-joined'])
+        hooks.hooks.execute(['hooks/json_status-relation-joined'])
         self.assertIn('status-url', self.relation_set.call_args_list[0][0][1])
 
 
@@ -358,7 +358,7 @@
         }
 
     def test_changed(self):
-        hooks.execute(['hooks/amqp-relation-changed'])
+        hooks.hooks.execute(['hooks/amqp-relation-changed'])
         self.wsgi_reload.assert_called_once_with()
         self.assertTrue(
             os.path.exists(os.path.join(self.tmpdir, 'amqp_config.py')))
@@ -366,7 +366,7 @@
     def test_departed(self):
         with open(os.path.join(self.tmpdir, 'amqp_config.py'), 'w') as f:
             f.write('')
-        hooks.execute(['hooks/amqp-relation-departed'])
+        hooks.hooks.execute(['hooks/amqp-relation-departed'])
         self.wsgi_reload.assert_called_once_with()
 
 
@@ -390,7 +390,7 @@
         self.relation_get.side_effect = [{'user': 'data', 'host': 'h'}]
 
     def test_pgsql_joined(self):
-        hooks.execute(['hooks/pgsql-relation-joined'])
+        hooks.hooks.execute(['hooks/pgsql-relation-joined'])
         self.assertTrue(
             os.path.exists(os.path.join(self.tmpdir, 'pgsql.json')))
         self.assertEqual(1, self.wsgi_reload.call_count)
@@ -402,7 +402,7 @@
             f.write('rm $0   # just delete ourself')
             f.flush()
         self.config.return_value['db_migration_cmd'] = 'dash ./tmp.sh'
-        hooks.execute([hook])
+        hooks.hooks.execute([hook])
         if runs:
             self.assertFalse(os.path.exists(os.path.join(cdir, 'tmp.sh')))
         else:
@@ -423,5 +423,5 @@
         self._do_migration('hooks/config-changed')
 
     def test_pgsql_broken(self):
-        hooks.execute(['hooks/pgsql-relation-broken'])
+        hooks.hooks.execute(['hooks/pgsql-relation-broken'])
         self.assertEqual(1, self.wsgi_reload.call_count)
 
=== modified file 'testing/run_tests.py'
--- testing/run_tests.py	2014-09-25 15:52:12 +0000
+++ testing/run_tests.py	2014-09-29 18:45:52 +0000
@@ -15,12 +15,14 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 """The CI Engine test runner."""
 
+import imp
 import logging
 import os
 import subunit
 import sys
 import testtools
-
+import traceback
+import unittest
 
 HERE = os.path.abspath(os.path.dirname(__file__))
 
@@ -336,16 +338,109 @@
     return suite
 
 
+class CharmTestSuite(unittest.TestSuite):
+    """Reloads charm modules before running the tests."""
+
+    def __init__(self, tests=()):
+        self.charm_root = None
+        super(CharmTestSuite, self).__init__(tests)
+
+    def setCharmRoots(self):
+        """Set 'charm_roots' for contained suites.
+
+        Calculates the 'charm_root' from the contained test 'id's
+        (see `prefix_test_ids` for details).
+        """
+        for s in self._tests:
+            flat_tests = list(filters.iter_flat(s))
+            # Empty test suites, obviously do not need charm_root.
+            if not flat_tests:
+                continue
+            # The mangled test IDs encode the charm path, which is common
+            # for all tests within a `TestSuite`. We grab the first test
+            # ID (charms.<series>.<name>) and turn it into a path
+            # (charms/<series>/<name>) that is set as the suite 'charm_root'.
+            s.charm_root = '/'.join(flat_tests[0].id().split('.')[:3])
+
+    def run(self, result):
+        """Reload local charm modules before running the inner tests.
+
+        Charm 'hooks' and 'charmhelpers' modules are reloaded in an
+        isolated environment before running the contained tests.
+        """
+        # Upper level TestSuite has no 'charm_root', we should do the
+        # usual run business.
+        if self.charm_root is None:
+            return super(CharmTestSuite, self).run(result)
+
+        with SysPath([os.path.join(self.charm_root, 'hooks')]):
+            for mod_name in ['charmhelpers', 'hooks']:
+                fp, path, desc = imp.find_module(mod_name)
+                try:
+                    imp.load_module(mod_name, fp, path, desc)
+                finally:
+                    if fp:
+                        fp.close()
+
+            return super(CharmTestSuite, self).run(result)
+
+    def __call__(self, *args, **kwds):
+        """Call local implementation of 'run' """
+        return self.run(*args, **kwds)
+
+
+class CharmTestLoader(loaders.Loader):
+    """Override ucitests.loaders.Loader fix tests importing conflicts.
+
+    All UCI-E charms contain python unittest cases in the <charm>/unit_tests/
+    directory and since the default ucitest module importer only considers
+    the context tree for naming modules, they all look the same and only the
+    first import is really effective (all the other tests/modules are
+    shadowed).
+
+    This class re-implements `importFromPath` to load charms test cases with
+    a *unique* name (<charm>.<test>) in a way they will never conflict.
+    """
+
+    # Loads tests with a *special* `CharmTestSuite` class, which is capable
+    # of re-importing local charm modules before running the tests.
+    suiteClass = CharmTestSuite
+
+    def importFromPath(self, path):
+        """Import and return the module for a given file path.
+
+        Modules (packages, really) are ignored and always result in an
+        empty module. Only files (*.py) are imported.
+
+        Raises ImportError if the given python file path could not be
+        imported.
+        """
+        if not path.endswith('.py'):
+            return imp.new_module('always_empty')
+
+        imp_path = os.path.join(self.root, os.path.normpath(path))
+        package = self.root.split('/')[-1].replace('-', '_')
+        module = os.path.basename(imp_path)[:-3]
+        mod_name = '{}.{}'.format(package, module)
+
+        try:
+            return imp.load_source(mod_name, imp_path)
+        except ImportError:
+            tb = traceback.format_exc()
+            msg = 'Failed to import {} at {}:\n{}'.format(mod_name, path, tb)
+            raise ImportError(msg)
+
+
 def load_charm_tests(include_regexps, exclude_regexps=None):
     """Load charm unittest from <charm>/unit_test."""
     charms = [
+        'charms/precise/key-secret-subordinate',
         'charms/precise/lander',
         'charms/precise/rabbitmq-worker',
+        'charms/precise/webui',
         'charms/precise/wsgi-app',
-        'charms/precise/webui',
-        'charms/precise/key-secret-subordinate',
     ]
-    loader = loaders.Loader()
+    loader = CharmTestLoader()
     suite = loader.suiteClass()
     for c in charms:
         with SysPath([c, os.path.join(c, 'hooks')]):
@@ -359,6 +454,9 @@
         suite.addTests(charm_suite)
     suite = filters.include_regexps(include_regexps, suite)
     suite = filters.exclude_regexps(exclude_regexps, suite)
+
+    suite.setCharmRoots()
+
     return suite