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

[Marco Ceppi]

Added support for substrate parsing in TestCfg

https://codereview.appspot.com/49160046/

[lp:~marcoceppi/charm-tools/test-cfg]

287. By Marco Ceppi on 2014-01-08

PEP8 Fixes

[Charm Tester (charmtester)]

Reviewers: mp+200921_code.launchpad.net,

Message:
Please take a look.

Description:

Added support for substrate parsing in TestCfg

https://code.launchpad.net/~marcoceppi/charm-tools/test-cfg/+merge/200921

(do not edit description out of merge proposal)

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

Affected files (+52, -21 lines):
   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

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: charmtools/bundles.py
=== modified file 'charmtools/bundles.py'
--- charmtools/bundles.py 2013-12-20 14:06:43 +0000
+++ charmtools/bundles.py 2014-01-08 22:43:34 +0000
@@ -2,7 +2,6 @@
  import yaml
  import glob
  import json
-import requests

  from linter import Linter
  from charmworldlib import bundle as cw_bundle

Index: charmtools/generate.py
=== modified file 'charmtools/generate.py'
--- charmtools/generate.py 2013-12-12 17:20:39 +0000
+++ charmtools/generate.py 2014-01-08 22:43:34 +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

Index: charmtools/test.py
=== modified file 'charmtools/test.py'
--- charmtools/test.py 2013-12-21 12:59:08 +0000
+++ charmtools/test.py 2014-01-08 22:43:34 +0000
@@ -3,7 +3,6 @@

  import os
  import sys
-import uuid
  import yaml
  import time
  import glob
@@ -12,7 +11,6 @@
  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']
+...

[Charm Tester (charmtester)]

Please take a look.

https://codereview.appspot.com/49160046/

[Benjamin Saller (bcsaller)]

I like what you started here and put a few comments inline.

I took a stab at extending your work in another branch.

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

Look at this when you have time. I think its more testable than what you
have there (and more tested infact). If you like the model we can look
at cleaning it up a little more and merging. Let me know what you think.

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py
File charmtools/test.py (right):

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py#newcode411
charmtools/test.py:411: setattr(self, key, val)
While this is a common pattern, its generally better to whitelist vars
that can be set in this way. Its possible now for the options to break
the tests by setting a name they sholdn't have on this class.

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py#newcode413
charmtools/test.py:413: self.substrates = {}
I think while we could validate the substrates its important to note
that what we produce from this parse is a set of rules, not a list of
substrates.

As we add substrates we want to automatically include those unless the
charm explicitly said it only tests on one substrate (which could happen
for EC2 specific charms for example) or that it explicitly doesn't run
on one (or more) substrates

https://codereview.appspot.com/49160046/

[Charm Tester (charmtester)]

I'll take a look at your proposal too, thanks for the review!

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py
File charmtools/test.py (right):

https://codereview.appspot.com/49160046/diff/20001/charmtools/test.py#newcode413
charmtools/test.py:413: self.substrates = {}
I agree, think of self.substrates as more self.substrate_rules where it
as if it's inclusive or not and the values for inclusive or not. It
never has a comprehensive list of _all_ substrates at any one time, it's
more a parsing of the substrate option provided in the test_config.yaml

On 2014/01/09 07:52:40, benjamin.saller wrote:
> I think while we could validate the substrates its important to note
that what
> we produce from this parse is a set of rules, not a list of
substrates.

> As we add substrates we want to automatically include those unless the
charm
> explicitly said it only tests on one substrate (which could happen for
EC2
> specific charms for example) or that it explicitly doesn't run on one
(or more)
> substrates

https://codereview.appspot.com/49160046/

Unmerged revisions

=== modified file 'charmtools/bundles.py'
--- charmtools/bundles.py	2013-12-20 14:06:43 +0000
+++ charmtools/bundles.py	2014-01-08 22:43:54 +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-08 22:43:54 +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-08 22:43:54 +0000
@@ -3,7 +3,6 @@
 
 import os
 import sys
-import uuid
 import yaml
 import time
 import glob
@@ -12,7 +11,6 @@
 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,18 @@
         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)
+        if 'substrates' in cfg:
+            self.substrates = {}
+            if 'allow' in cfg:
+                self.substrates['inclusive'] = True
+                self.substrates['values'] = cfg['substrates']['allow']
+            elif 'skip' in cfg:
+                self.substrates['inclusive'] = False
+                self.substrates['values'] = cfg['substrates']['skip']
 
 
 def get_juju_version():
@@ -564,8 +576,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 +590,23 @@
     logger.debug('Creating a new Conductor')
     try:
         tester = Conductor(args)
+        env_yaml = tester.get_environment(cfg.juju_env)
+        if 'substrates' in cfg:
+            # We have substrate configuration data.
+            substrate_match = env_yaml['type'] in cfg.substrates['values']
+            if cfg.substrates['inclusive'] and not substrate_match:
+                pass
+            if not cfg.substrates['inclusive'] and substrate_match:
+                raise Exception('%s is not in allowed substrates: %s' %
+                                (env_yaml['type'],
+                                 cfg.substrates['values'].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-08 22:43:54 +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-08 22:43:54 +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)