Merge lp:~bcsaller/charm-tools/test-cfg into lp:charm-tools/1.2

[Benjamin Saller]

Substrate filter rules

As a slightly most testable impl of Marco's work I'm offering this as a
counter proposal. Happy to talk through the thought process.

https://codereview.appspot.com/49670043/

[Benjamin Saller (bcsaller)]

Reviewers: mp+200959_code.launchpad.net,

Message:
Please take a look.

Description:
Substrate filter rules

As a slightly most testable impl of Marco's work I'm offering this as a
counter proposal. Happy to talk through the thought process.

https://code.launchpad.net/~bcsaller/charm-tools/test-cfg/+merge/200959

(do not edit description out of merge proposal)

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

Affected files (+177, -28 lines):
   M .bzrignore
   A [revision details]
   M charmtools/bundles.py
   M charmtools/generate.py
   M charmtools/test.py
   M charmtools/update.py
   M tests/test_juju_test.py
   A tests/test_substrates.py

[Marco Ceppi (marcoceppi)]

This is an interesting concept of defining order and making it explicit.
I didn't quite grasp it until I saw the unit tests for it. I'm not
opposed to it now, test.py:652 is still a concern but my comment on 473
now no longer applies

https://codereview.appspot.com/49670043/diff/1/charmtools/test.py
File charmtools/test.py (right):

https://codereview.appspot.com/49670043/diff/1/charmtools/test.py#newcode473
charmtools/test.py:473: 'order should be defined using only include and
skip')
Isn't it counter intuitive to have both include and skip? By design if
you only have an `include` key it'll skip everything not in that include
key. Likewise, if you have a skip it's understood that you'll do
everything but. Seems this should just be OR

https://codereview.appspot.com/49670043/diff/1/charmtools/test.py#newcode652
charmtools/test.py:652: rules = parse_substrates(cfg)
This wouldn't work as cfg is a TestCfg object that only has everything
in the 'options' key. Substrate data would be in 'substrates' at the
same level of 'options'. So if parse_substrates took the test_plan file
and did a parsing that would work. Alternatively TestCfg could (should?)
parse what's in substrates and store it as part of the object so you can
just pass cfg as you're doing here instead of re-parsing the
test_config.yaml file

https://codereview.appspot.com/49670043/

Unmerged revisions

=== modified file '.bzrignore'
--- .bzrignore	2013-12-16 16:12:29 +0000
+++ .bzrignore	2014-01-09 07:49:22 +0000
@@ -9,3 +9,5 @@
 lib
 local
 man
+tests/.ropeproject/
+charmtools/.ropeproject/

=== modified file 'charmtools/bundles.py'
--- charmtools/bundles.py	2013-12-20 14:06:43 +0000
+++ charmtools/bundles.py	2014-01-09 07:49:22 +0000
@@ -2,7 +2,6 @@
 import yaml
 import glob
 import json
-import requests
 
 from linter import Linter
 from charmworldlib import bundle as cw_bundle

=== modified file 'charmtools/generate.py'
--- charmtools/generate.py	2013-12-12 17:20:39 +0000
+++ charmtools/generate.py	2014-01-09 07:49:22 +0000
@@ -16,7 +16,6 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import sys
 import shutil
 import argparse
 

=== modified file 'charmtools/test.py'
--- charmtools/test.py	2013-12-21 12:59:08 +0000
+++ charmtools/test.py	2014-01-09 07:49:22 +0000
@@ -1,18 +1,16 @@
 #!/usr/bin/python
 # coding=latin-1
 
+import argparse
+import glob
+import logging
 import os
+import re
+import signal
+import subprocess
 import sys
-import uuid
+import time
 import yaml
-import time
-import glob
-import signal
-import logging
-import argparse
-import subprocess
-
-from tempfile import mkdtemp
 from datetime import timedelta
 from contextlib import contextmanager
 
@@ -44,6 +42,10 @@
     pass
 
 
+class SubstrateMismatch(Exception):
+    pass
+
+
 class TimeoutError(Exception):
     def __init__(self, value="Timed Out"):
         self.value = value
@@ -114,7 +116,7 @@
                 self.destroy(self.juju_env)
             except DestroyUnreliable:
                 self.log.warn('Unable to destroy bootstrap, trying again')
-                sleep(2)
+                time.sleep(2)
                 try:
                     self.destroy(self.juju_env)
                 except:
@@ -258,6 +260,7 @@
         try:
             with timeout(self.conductor.args.timeout):
                 output = subprocess.check_output(self.test, env=self.env)
+                self.log.debug(output)
         except TimeoutError, e:
             self.log.debug('Killed by timeout after %s seconds',
                            self.conductor.args.timeout)
@@ -279,7 +282,7 @@
             try:
                 self.archive_logs()
             except OrchestraError, e:
-                juju.log.error(e)
+                self.log.error(e)
 
         if error:
             raise error
@@ -293,7 +296,7 @@
             raise OrchestraError('Unable to query juju status')
 
         services = status['services']
-        machines = status['machines']
+        # machines = status['machines']
 
         if self.conductor.juju_version.major == 0:
             logs.append('/var/lib/juju/units/./*/charm.log')
@@ -402,9 +405,10 @@
         with open(config_file) as f:
             cfg = yaml.safe_load(f.read())
 
-        for key, val in cfg['options'].iteritems():
-            if key in self._keys:
-                setattr(self, key, val)
+        if 'options' in cfg:
+            for key, val in cfg['options'].iteritems():
+                if key in self._keys:
+                    setattr(self, key, val)
 
 
 def get_juju_version():
@@ -455,6 +459,71 @@
     return logger
 
 
+class SubstrateFilter(object):
+    def __init__(self, spec):
+        self.order = spec.get('order', ['include', 'skip'])
+        self.include = spec.get('include', ['*'])
+        self.skip = spec.get('skip', [])
+
+        if isinstance(self.order, str):
+            self.order = [s.strip() for s in self.order.split(',')]
+        if self.order != ['include', 'skip'] and \
+                self.order != ['skip', 'include']:
+            raise ValueError(
+                'order should be defined using only include and skip')
+
+        if isinstance(self.include, str):
+            self.include = [self.include]
+        self.include = set(self.include)
+
+        if isinstance(self.skip, str):
+            self.skip = [self.skip]
+        self.skip = set(self.skip)
+
+    def filter(self, substrates):
+        """
+        Filter a list of substrates relative to the rules generated on class
+        creation.
+        """
+        if isinstance(substrates, str):
+            substrates = [s.strip() for s in re.split('[,\s]', substrates)]
+
+        # Apply the rules to the list of substrates returning anything that
+        # matches
+        if self.order == ['include', 'skip']:
+            result = self._filter_includes(substrates, True)
+            result = self._filter_skips(result)
+        else:
+            result = self._filter_skips(substrates, True)
+            result = self._filter_includes(result)
+        return result
+
+    def _filter_includes(self, inputList, priority=False):
+        if priority and '*' in self.include:
+            return inputList
+        return sorted(list(set(inputList).intersection(self.include)))
+
+    def _filter_skips(self, inputList, priority=False):
+        if priority and '*' in self.skip:
+            return list(self.include.intersection(inputList))
+        return sorted(list(set(inputList).difference(self.skip)))
+
+
+def parse_substrates(spec):
+    if isinstance(spec, basestring):
+        spec = yaml.safe_load(spec)
+    if not spec or 'substrates' not in spec:
+        raise ValueError(
+            "Invalid data passed to parse_substrates: {}".format(spec))
+
+    specRules = SubstrateFilter(spec['substrates'])
+    return specRules
+
+
+def allowed_substrates(spec, possible_substrates):
+    return parse_substrates(spec).filter(possible_substrates)
+
+
 def setup_parser():
     parser = argparse.ArgumentParser(
         prog='juju test',
@@ -564,8 +633,9 @@
     logger.info('Starting test run on %s using Juju %s'
                 % (args.juju_env, get_juju_version()))
     logger.debug('Loading configuration options from testplan YAML')
-    test_plans = glob.glob(os.path.join(os.getcwd(), 'tests', 'testplan.y*ml'))
-    test_plan = test_plan[0] if test_plans else None
+    test_plans = glob.glob(os.path.join(os.getcwd(), 'tests',
+                                        'test_config.y*ml'))
+    test_plan = test_plans[0] if test_plans else None
     if test_plan:
         cfg = TestCfg(test_plan)
         for key, val in args.iteritems():
@@ -577,10 +647,21 @@
     logger.debug('Creating a new Conductor')
     try:
         tester = Conductor(args)
+        env_yaml = tester.get_environment(cfg.juju_env)
+        if 'substrates' in cfg:
+            rules = parse_substrates(cfg)
+            allowed = rules.filter(env_yaml['type'])
+            if env_yaml['type'] not in allowed:
+                raise Exception('%s is not an allowed substrate: %s' %
+                                (env_yaml['type'],
+                                 allowed.join(', ')))
         errors, failures, passes = tester.run()
     except NoTests:
         logger.critical('No tests were found')
         sys.exit(1)
+    except Exception as e:
+        logger.critical(str(e))
+        sys.exit(1)
     except:
         raise
 

=== modified file 'charmtools/update.py'
--- charmtools/update.py	2013-12-16 16:12:29 +0000
+++ charmtools/update.py	2014-01-09 07:49:22 +0000
@@ -34,6 +34,7 @@
 
     return parser
 
+
 def update(charm_dir, fix=False):
     mr = Mr(charm_dir)
     for charm in charms.remote():
@@ -48,6 +49,7 @@
     except Exception as e:
         raise Exception(".mrconfig not saved: %s" % e.strerror)
 
+
 def main():
     parser = setup_parser()
     args = parser.parse_args()

=== modified file 'tests/test_juju_test.py'
--- tests/test_juju_test.py	2013-12-20 12:33:21 +0000
+++ tests/test_juju_test.py	2014-01-09 07:49:22 +0000
@@ -30,13 +30,16 @@
 
 PARSED_ENVIRONMENTS_YAML = yaml.safe_load(RAW_ENVIRONMENTS_YAML)
 
+
 class Arguments(object):
     def __init__(self, **kwargs):
         for key, value in kwargs.items():
             setattr(self, key, value)
+
     def __getattr__(self, name):
         return None
 
+
 class JujuTestPluginTest(unittest.TestCase):
     @patch('subprocess.check_output')
     def test_get_gojuju_version(self, mock_check_output):
@@ -77,7 +80,7 @@
         self.assertEqual(100, juju_test.convert_to_timedelta('100s'))
         self.assertEqual(100, juju_test.convert_to_timedelta(100))
         self.assertEqual(60, juju_test.convert_to_timedelta('1m'))
-        self.assertEqual(60*60, juju_test.convert_to_timedelta('1h'))
+        self.assertEqual(60 * 60, juju_test.convert_to_timedelta('1h'))
 
     @patch('glob.glob')
     @patch('os.path.isfile')
@@ -89,7 +92,7 @@
         mock_isfile.side_effect = files_exist
         mock_glob.return_value = tests_directory
 
-        args = Arguments(tests = 'dummy')
+        args = Arguments(tests='dummy')
         c = juju_test.Conductor(args)
         results = c.find_tests()
 
@@ -100,7 +103,7 @@
     def test_conductor_find_tests_fails(self, mock_glob):
         mock_glob.return_value = []
 
-        args = Arguments(tests = 'dummy')
+        args = Arguments(tests='dummy')
         c = juju_test.Conductor(args)
         results = c.find_tests()
 
@@ -126,7 +129,7 @@
 
         mcheck_output.side_effect = [yml_output, Exception('not bootstrapped')]
         juju_env = 'test'
-        args = Arguments(tests = 'dummy')
+        args = Arguments(tests='dummy')
         c = juju_test.Conductor(args)
         results = c.status(juju_env)
 
@@ -193,7 +196,7 @@
 
     @patch('subprocess.check_call')
     def test_conductor_destroy(self, mock_check_call):
-        args = Arguments(tests = 'dummy')
+        args = Arguments(tests='dummy')
         c = juju_test.Conductor(args)
         c.juju_version = juju_test.JujuVersion(major=1, minor=1, patch=1)
         good_env = 'valid'
@@ -216,7 +219,7 @@
 
         bad_env = 'invalid'
 
-        args = Arguments(tests = 'dummy')
+        args = Arguments(tests='dummy')
         c = juju_test.Conductor(args)
         c.juju_version = juju_test.JujuVersion(major=1, minor=8, patch=0)
         self.assertRaises(juju_test.DestroyUnreliable, c.destroy, bad_env)
@@ -448,7 +451,7 @@
         o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL,
                                                      juju_test.TEST_TIMEOUT))
 
-        o.conductor.args.on_timeout='skip'
+        o.conductor.args.on_timeout = 'skip'
         o.log.status.reset_mock()
         o.print_status(124)
         o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL,
@@ -583,7 +586,7 @@
         goyml_parsed = yaml.safe_load(goyml_output)
         mstatus.return_value = goyml_parsed
 
-        juju_env='testing'
+        juju_env = 'testing'
         args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp')
         c = juju_test.Conductor(args)
         c.juju_version = juju_test.JujuVersion(major=1, minor=10, patch=0)
@@ -623,7 +626,7 @@
         pyyml_parsed = yaml.safe_load(pyyml_output)
         mstatus.return_value = pyyml_parsed
 
-        juju_env='testing'
+        juju_env = 'testing'
         args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp')
         c = juju_test.Conductor(args)
         c.juju_version = juju_test.JujuVersion(major=0, minor=7, patch=0)

=== added file 'tests/test_substrates.py'
--- tests/test_substrates.py	1970-01-01 00:00:00 +0000
+++ tests/test_substrates.py	2014-01-09 07:49:22 +0000
@@ -0,0 +1,61 @@
+"""Unit test for juju tests substrate handling"""
+
+import unittest
+import yaml
+from charmtools.test import parse_substrates, allowed_substrates
+
+class JujuTestSubstrateTests(unittest.TestCase):
+    INCLUDE_SKIP = '''
+substrates:
+    order: include, skip
+    skip: amazon
+    include: "*"
+    '''
+
+    SKIP_INCLUDE = '''
+substrates:
+    order: skip, include
+    skip: "*"
+    include: amazon
+    '''
+
+    def test_parse_substrates_text(self):
+        self.assertRaises(ValueError,  parse_substrates, '')
+        self.assertIsNotNone(parse_substrates(self.INCLUDE_SKIP))
+
+    def test_parse_substrate_object(self):
+        data = yaml.load(self.INCLUDE_SKIP)
+        self.assertIsNotNone(parse_substrates(data))
+
+    def test_parse_substrates_order(self):
+        result = parse_substrates(self.INCLUDE_SKIP)
+        self.assertEqual(result.order, ['include', 'skip'])
+
+    def test_parse_substrates_order_invalid(self):
+        data = self.INCLUDE_SKIP.replace('skip', 'foobar')
+        self.assertRaises(ValueError, parse_substrates, data)
+
+    def test_parse_substrates_include(self):
+        result = parse_substrates(self.INCLUDE_SKIP)
+        self.assertEqual(result.include, set(['*']))
+
+    def test_parse_substrates_skip(self):
+        result = parse_substrates(self.INCLUDE_SKIP)
+        self.assertEqual(result.skip, set(['amazon']))
+
+    def test_filter_include_skip(self):
+        rules = parse_substrates(self.INCLUDE_SKIP)
+        self.assertEqual(rules.filter('amazon,hp,azure'), ['azure', 'hp'])
+
+    def test_filter_skip_include(self):
+        rules = parse_substrates(self.SKIP_INCLUDE)
+        self.assertEqual(rules.filter('amazon,hp,azure'), ['amazon'])
+
+    def test_allowed_substrates(self):
+        self.assertEqual(allowed_substrates(self.INCLUDE_SKIP,
+                                            ['foo', 'bar', 'amazon']),
+                         ['bar', 'foo'])
+
+
+if __name__ == '__main__':
+    unittest.main()