Juju Charm Tools

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

  1. test-cfg
  2. Merge into 1.2
Proposed by Marco Ceppi
Status: Rejected
Rejected by: Marco Ceppi
Proposed branch: lp:~marcoceppi/charm-tools/test-cfg
Merge into: lp:charm-tools/1.2
Diff against target: 269 lines (+50/-21)
5 files modified
charmtools/bundles.py (+0/-1)
charmtools/generate.py (+0/-1)
charmtools/test.py (+36/-10)
charmtools/update.py (+2/-0)
tests/test_juju_test.py (+12/-9)
To merge this branch: bzr merge lp:~marcoceppi/charm-tools/test-cfg
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+200921@code.launchpad.net

Description of the change

Added support for substrate parsing in TestCfg

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

To post a comment you must log in.
lp:~marcoceppi/charm-tools/test-cfg updated
287. By Marco Ceppi

PEP8 Fixes

Revision history for this message
Charm Tester (charmtester) wrote :
Download full text (9.8 KiB)

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']
+...

Read more...

Revision history for this message
Charm Tester (charmtester) wrote :

Please take a look.

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

Revision history for this message
Benjamin Saller (bcsaller) wrote :

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/

Revision history for this message
Charm Tester (charmtester) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
2import yaml2import yaml
3import glob3import glob
4import json4import json
5import requests
65
7from linter import Linter6from linter import Linter
8from charmworldlib import bundle as cw_bundle7from charmworldlib import bundle as cw_bundle
98
=== 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 @@
16# along with this program. If not, see <http://www.gnu.org/licenses/>.16# along with this program. If not, see <http://www.gnu.org/licenses/>.
1717
18import os18import os
19import sys
20import shutil19import shutil
21import argparse20import argparse
2221
2322
=== 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 @@
33
4import os4import os
5import sys5import sys
6import uuid
7import yaml6import yaml
8import time7import time
9import glob8import glob
@@ -12,7 +11,6 @@
12import argparse11import argparse
13import subprocess12import subprocess
1413
15from tempfile import mkdtemp
16from datetime import timedelta14from datetime import timedelta
17from contextlib import contextmanager15from contextlib import contextmanager
1816
@@ -44,6 +42,10 @@
44 pass42 pass
4543
4644
45class SubstrateMismatch(Exception):
46 pass
47
48
47class TimeoutError(Exception):49class TimeoutError(Exception):
48 def __init__(self, value="Timed Out"):50 def __init__(self, value="Timed Out"):
49 self.value = value51 self.value = value
@@ -114,7 +116,7 @@
114 self.destroy(self.juju_env)116 self.destroy(self.juju_env)
115 except DestroyUnreliable:117 except DestroyUnreliable:
116 self.log.warn('Unable to destroy bootstrap, trying again')118 self.log.warn('Unable to destroy bootstrap, trying again')
117 sleep(2)119 time.sleep(2)
118 try:120 try:
119 self.destroy(self.juju_env)121 self.destroy(self.juju_env)
120 except:122 except:
@@ -258,6 +260,7 @@
258 try:260 try:
259 with timeout(self.conductor.args.timeout):261 with timeout(self.conductor.args.timeout):
260 output = subprocess.check_output(self.test, env=self.env)262 output = subprocess.check_output(self.test, env=self.env)
263 self.log.debug(output)
261 except TimeoutError, e:264 except TimeoutError, e:
262 self.log.debug('Killed by timeout after %s seconds',265 self.log.debug('Killed by timeout after %s seconds',
263 self.conductor.args.timeout)266 self.conductor.args.timeout)
@@ -279,7 +282,7 @@
279 try:282 try:
280 self.archive_logs()283 self.archive_logs()
281 except OrchestraError, e:284 except OrchestraError, e:
282 juju.log.error(e)285 self.log.error(e)
283286
284 if error:287 if error:
285 raise error288 raise error
@@ -293,7 +296,7 @@
293 raise OrchestraError('Unable to query juju status')296 raise OrchestraError('Unable to query juju status')
294297
295 services = status['services']298 services = status['services']
296 machines = status['machines']299 # machines = status['machines']
297300
298 if self.conductor.juju_version.major == 0:301 if self.conductor.juju_version.major == 0:
299 logs.append('/var/lib/juju/units/./*/charm.log')302 logs.append('/var/lib/juju/units/./*/charm.log')
@@ -402,9 +405,18 @@
402 with open(config_file) as f:405 with open(config_file) as f:
403 cfg = yaml.safe_load(f.read())406 cfg = yaml.safe_load(f.read())
404407
405 for key, val in cfg['options'].iteritems():408 if 'options' in cfg:
406 if key in self._keys:409 for key, val in cfg['options'].iteritems():
407 setattr(self, key, val)410 if key in self._keys:
411 setattr(self, key, val)
412 if 'substrates' in cfg:
413 self.substrates = {}
414 if 'allow' in cfg:
415 self.substrates['inclusive'] = True
416 self.substrates['values'] = cfg['substrates']['allow']
417 elif 'skip' in cfg:
418 self.substrates['inclusive'] = False
419 self.substrates['values'] = cfg['substrates']['skip']
408420
409421
410def get_juju_version():422def get_juju_version():
@@ -564,8 +576,9 @@
564 logger.info('Starting test run on %s using Juju %s'576 logger.info('Starting test run on %s using Juju %s'
565 % (args.juju_env, get_juju_version()))577 % (args.juju_env, get_juju_version()))
566 logger.debug('Loading configuration options from testplan YAML')578 logger.debug('Loading configuration options from testplan YAML')
567 test_plans = glob.glob(os.path.join(os.getcwd(), 'tests', 'testplan.y*ml'))579 test_plans = glob.glob(os.path.join(os.getcwd(), 'tests',
568 test_plan = test_plan[0] if test_plans else None580 'test_config.y*ml'))
581 test_plan = test_plans[0] if test_plans else None
569 if test_plan:582 if test_plan:
570 cfg = TestCfg(test_plan)583 cfg = TestCfg(test_plan)
571 for key, val in args.iteritems():584 for key, val in args.iteritems():
@@ -577,10 +590,23 @@
577 logger.debug('Creating a new Conductor')590 logger.debug('Creating a new Conductor')
578 try:591 try:
579 tester = Conductor(args)592 tester = Conductor(args)
593 env_yaml = tester.get_environment(cfg.juju_env)
594 if 'substrates' in cfg:
595 # We have substrate configuration data.
596 substrate_match = env_yaml['type'] in cfg.substrates['values']
597 if cfg.substrates['inclusive'] and not substrate_match:
598 pass
599 if not cfg.substrates['inclusive'] and substrate_match:
600 raise Exception('%s is not in allowed substrates: %s' %
601 (env_yaml['type'],
602 cfg.substrates['values'].join(', ')))
580 errors, failures, passes = tester.run()603 errors, failures, passes = tester.run()
581 except NoTests:604 except NoTests:
582 logger.critical('No tests were found')605 logger.critical('No tests were found')
583 sys.exit(1)606 sys.exit(1)
607 except Exception as e:
608 logger.critical(str(e))
609 sys.exit(1)
584 except:610 except:
585 raise611 raise
586612
587613
=== 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 @@
3434
35 return parser35 return parser
3636
37
37def update(charm_dir, fix=False):38def update(charm_dir, fix=False):
38 mr = Mr(charm_dir)39 mr = Mr(charm_dir)
39 for charm in charms.remote():40 for charm in charms.remote():
@@ -48,6 +49,7 @@
48 except Exception as e:49 except Exception as e:
49 raise Exception(".mrconfig not saved: %s" % e.strerror)50 raise Exception(".mrconfig not saved: %s" % e.strerror)
5051
52
51def main():53def main():
52 parser = setup_parser()54 parser = setup_parser()
53 args = parser.parse_args()55 args = parser.parse_args()
5456
=== 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 @@
3030
31PARSED_ENVIRONMENTS_YAML = yaml.safe_load(RAW_ENVIRONMENTS_YAML)31PARSED_ENVIRONMENTS_YAML = yaml.safe_load(RAW_ENVIRONMENTS_YAML)
3232
33
33class Arguments(object):34class Arguments(object):
34 def __init__(self, **kwargs):35 def __init__(self, **kwargs):
35 for key, value in kwargs.items():36 for key, value in kwargs.items():
36 setattr(self, key, value)37 setattr(self, key, value)
38
37 def __getattr__(self, name):39 def __getattr__(self, name):
38 return None40 return None
3941
42
40class JujuTestPluginTest(unittest.TestCase):43class JujuTestPluginTest(unittest.TestCase):
41 @patch('subprocess.check_output')44 @patch('subprocess.check_output')
42 def test_get_gojuju_version(self, mock_check_output):45 def test_get_gojuju_version(self, mock_check_output):
@@ -77,7 +80,7 @@
77 self.assertEqual(100, juju_test.convert_to_timedelta('100s'))80 self.assertEqual(100, juju_test.convert_to_timedelta('100s'))
78 self.assertEqual(100, juju_test.convert_to_timedelta(100))81 self.assertEqual(100, juju_test.convert_to_timedelta(100))
79 self.assertEqual(60, juju_test.convert_to_timedelta('1m'))82 self.assertEqual(60, juju_test.convert_to_timedelta('1m'))
80 self.assertEqual(60*60, juju_test.convert_to_timedelta('1h'))83 self.assertEqual(60 * 60, juju_test.convert_to_timedelta('1h'))
8184
82 @patch('glob.glob')85 @patch('glob.glob')
83 @patch('os.path.isfile')86 @patch('os.path.isfile')
@@ -89,7 +92,7 @@
89 mock_isfile.side_effect = files_exist92 mock_isfile.side_effect = files_exist
90 mock_glob.return_value = tests_directory93 mock_glob.return_value = tests_directory
9194
92 args = Arguments(tests = 'dummy')95 args = Arguments(tests='dummy')
93 c = juju_test.Conductor(args)96 c = juju_test.Conductor(args)
94 results = c.find_tests()97 results = c.find_tests()
9598
@@ -100,7 +103,7 @@
100 def test_conductor_find_tests_fails(self, mock_glob):103 def test_conductor_find_tests_fails(self, mock_glob):
101 mock_glob.return_value = []104 mock_glob.return_value = []
102105
103 args = Arguments(tests = 'dummy')106 args = Arguments(tests='dummy')
104 c = juju_test.Conductor(args)107 c = juju_test.Conductor(args)
105 results = c.find_tests()108 results = c.find_tests()
106109
@@ -126,7 +129,7 @@
126129
127 mcheck_output.side_effect = [yml_output, Exception('not bootstrapped')]130 mcheck_output.side_effect = [yml_output, Exception('not bootstrapped')]
128 juju_env = 'test'131 juju_env = 'test'
129 args = Arguments(tests = 'dummy')132 args = Arguments(tests='dummy')
130 c = juju_test.Conductor(args)133 c = juju_test.Conductor(args)
131 results = c.status(juju_env)134 results = c.status(juju_env)
132135
@@ -193,7 +196,7 @@
193196
194 @patch('subprocess.check_call')197 @patch('subprocess.check_call')
195 def test_conductor_destroy(self, mock_check_call):198 def test_conductor_destroy(self, mock_check_call):
196 args = Arguments(tests = 'dummy')199 args = Arguments(tests='dummy')
197 c = juju_test.Conductor(args)200 c = juju_test.Conductor(args)
198 c.juju_version = juju_test.JujuVersion(major=1, minor=1, patch=1)201 c.juju_version = juju_test.JujuVersion(major=1, minor=1, patch=1)
199 good_env = 'valid'202 good_env = 'valid'
@@ -216,7 +219,7 @@
216219
217 bad_env = 'invalid'220 bad_env = 'invalid'
218221
219 args = Arguments(tests = 'dummy')222 args = Arguments(tests='dummy')
220 c = juju_test.Conductor(args)223 c = juju_test.Conductor(args)
221 c.juju_version = juju_test.JujuVersion(major=1, minor=8, patch=0)224 c.juju_version = juju_test.JujuVersion(major=1, minor=8, patch=0)
222 self.assertRaises(juju_test.DestroyUnreliable, c.destroy, bad_env)225 self.assertRaises(juju_test.DestroyUnreliable, c.destroy, bad_env)
@@ -448,7 +451,7 @@
448 o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL,451 o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL,
449 juju_test.TEST_TIMEOUT))452 juju_test.TEST_TIMEOUT))
450453
451 o.conductor.args.on_timeout='skip'454 o.conductor.args.on_timeout = 'skip'
452 o.log.status.reset_mock()455 o.log.status.reset_mock()
453 o.print_status(124)456 o.print_status(124)
454 o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL,457 o.log.status.assert_called_with('%s (%s)' % (juju_test.TEST_FAIL,
@@ -583,7 +586,7 @@
583 goyml_parsed = yaml.safe_load(goyml_output)586 goyml_parsed = yaml.safe_load(goyml_output)
584 mstatus.return_value = goyml_parsed587 mstatus.return_value = goyml_parsed
585588
586 juju_env='testing'589 juju_env = 'testing'
587 args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp')590 args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp')
588 c = juju_test.Conductor(args)591 c = juju_test.Conductor(args)
589 c.juju_version = juju_test.JujuVersion(major=1, minor=10, patch=0)592 c.juju_version = juju_test.JujuVersion(major=1, minor=10, patch=0)
@@ -623,7 +626,7 @@
623 pyyml_parsed = yaml.safe_load(pyyml_output)626 pyyml_parsed = yaml.safe_load(pyyml_output)
624 mstatus.return_value = pyyml_parsed627 mstatus.return_value = pyyml_parsed
625628
626 juju_env='testing'629 juju_env = 'testing'
627 args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp')630 args = Arguments(tests='dummy', juju_env=juju_env, logdir='/tmp')
628 c = juju_test.Conductor(args)631 c = juju_test.Conductor(args)
629 c.juju_version = juju_test.JujuVersion(major=0, minor=7, patch=0)632 c.juju_version = juju_test.JujuVersion(major=0, minor=7, patch=0)

Subscribers

People subscribed via source and target branches

to all changes: