Merge lp:~frankban/charms/oneiric/buildbot-slave/tests_again into lp:~yellow/charms/oneiric/buildbot-slave/trunk

[Francesco Banconi]

- buildslave charm tests refactoring
- using run function to chmod the script in install hook
- changed symlinks (helpers.py, tests.cfg)

[Graham Binns (gmb)]

82 + wait_for_unit(service_name, timeout=600)

We should probably add an XXX here to remind ourselves to try and either bring this down or do something smarter with it depending on the state of the Juju environment.

review: Approve (code)

[lp:~frankban/charms/oneiric/buildbot-slave/tests_again]

12. By Francesco Banconi on 2012-02-09

Added timeout XXX.

=== modified file 'hooks/install'
--- hooks/install	2012-02-09 10:05:06 +0000
+++ hooks/install	2012-02-09 12:09:17 +0000
@@ -10,6 +10,7 @@
     log,
     log_entry,
     log_exit,
+    run,
     )
 import os
 import shlex
@@ -70,8 +71,7 @@
     log('Retrieving script: {}.'.format(url))
     script = retrieve(url, path)
     log('Changing script mode.')
-    chmod = command('chmod')
-    chmod('+x', script)
+    run('chmod', '+x', script)
     log('Executing script: {}.'.format(script))
     return subprocess.call([script] + shlex.split(str(args)))
 
 
=== modified file 'tests/buildbot-slave.test'
--- tests/buildbot-slave.test	2012-02-08 18:00:09 +0000
+++ tests/buildbot-slave.test	2012-02-09 12:09:17 +0000
@@ -12,6 +12,8 @@
     command,
     encode_file,
     unit_info,
+    wait_for_unit,
+    wait_for_relation,
     )
 from openport import (
     PORT,
@@ -19,12 +21,6 @@
     )
 
 
-# To run tests, having juju bootstrapped:
-# JUJU_REPOSITORY=. oneiric/buildbot-slave/tests/buildbot-slave.test
-# XXX 2012-02-08 gmb:
-#     This needs reducing; it's set high for debugging purposes only.
-MAX_ATTEMPTS = 600
-
 juju = command('juju')
 
 
@@ -35,10 +31,15 @@
         self.repository = os.getenv('JUJU_REPOSITORY')
         self.environment = os.getenv('JUJU_ENVIRONMENT')
         self.tests_dir = os.path.dirname(os.path.abspath(__file__))
-
-    def deploy(self, config=None, sleep_time=0.1, charm_name=None):
-        deploy_name = charm_name = charm_name or self.charm_name
+        self.started_services = set()
+
+    def tearDown(self):
+        for service_name in self.started_services:
+            juju('destroy-service', service_name)
+
+    def deploy(self, service_name, config=None):
         args = ['deploy']
+        deploy_name = service_name
         if self.repository is not None:
             args.append('--repository=' + self.repository)
             deploy_name = 'local:' + deploy_name
@@ -48,24 +49,29 @@
             args.append('--config=' + config)
         args.append(deploy_name)
         juju(*args)
-        status_attempts = 0
-        while status_attempts < MAX_ATTEMPTS:
-            status = unit_info(charm_name, 'state')
-            if 'error' in status:
-                return False
-            if status == 'started':
-                return True
-            time.sleep(sleep_time)
-            status_attempts += 1
-        raise Exception("Charm took too long to deploy.")
+        self.started_services.add(service_name)
+        # XXX 2012-02-09 frankban
+        #     We use a timeout of 600 seconds to avoid runtime errors
+        #     occurring if the instance is not created
+        #     (e.g. just after `juju bootstrap`).
+        #     We should try to wait for unit in a smarter way
+        #     (maybe checking for "pending"?).
+        wait_for_unit(service_name, timeout=600)
+
+    def add_relation(self, relation, service1, service2):
+        juju('add-relation',
+             '{}:{}'.format(service1, relation),
+             '{}:{}'.format(service2, relation))
+        wait_for_relation(service1, relation)
+
+    def get_url(self, service_name, port, path=''):
+        address = unit_info(service_name, 'public-address')
+        return 'http://{}:{}{}'.format(address, port, path)
 
     def test_deploy(self):
         # Ensure the charm starts correctly when deployed.
-        try:
-            started = self.deploy()
-            self.assertTrue(started)
-        finally:
-            juju('destroy-service', self.charm_name)
+        self.deploy(self.charm_name)
+        self.assertEqual('started', unit_info(self.charm_name, 'state'))
 
     def do_not_test_script(self):
         # DISABLED BECAUSE IT FAILS ALL THE TIME.
@@ -87,44 +93,31 @@
 
     def test_master_slave_relationship(self):
         master_charm_name = 'buildbot-master'
+        self.deploy(master_charm_name)
+        self.deploy(self.charm_name)
+        juju('set', master_charm_name, 'extra-packages=git')
+        config_path = os.path.join(os.path.dirname(__file__), 'test.cfg')
+        config = encode_file(config_path)
+        juju('set', 'buildbot-master', 'config-file='+config)
+        juju('expose', master_charm_name)
+        juju('set', self.charm_name, 'builders=runtests')
+        # XXX 2012-02-08 gmb
+        #     This avoids us trying to check whether the slave
+        #     is connected until the relation is initialized,
+        #     but... (see next XXX)
+        self.add_relation('buildbot', self.charm_name, master_charm_name)
+        # XXX 2012-02-08 gmb:
+        #     This still sometimes gets called _before_ the
+        #     buildslave connects to the buildbot master. Hence
+        #     this rather ugly sleep here.
+        time.sleep(5)
+        url = self.get_url(master_charm_name, 8010, '/buildslaves')
         try:
-            self.deploy(charm_name=master_charm_name)
-            self.deploy()
-            juju('set', master_charm_name, 'extra-packages=git')
-            encoded_config = encode_file(
-                os.path.join(self.tests_dir, '..', 'examples', 'master.cfg'))
-            juju(
-                'set', master_charm_name,
-                'config-file={}'.format(encoded_config))
-            juju('expose', master_charm_name)
-            juju('set', self.charm_name, 'builders=runtests')
-            juju('add-relation', self.charm_name, master_charm_name)
-            address = unit_info(master_charm_name, 'public-address')
-            url = 'http://{}:{}/buildslaves'.format(address, 8010)
-            # Wait for buildbot to restart, since there's a
-            # potential race condition with an asynchronous service.
-            while True:
-                # XXX 2012-02-08 gmb
-                #     This avoids us trying to check whether the slave
-                #     is connected until the relation is initialized,
-                #     but... (see next XXX)
-                relation_info = unit_info(self.charm_name, 'relations')
-                if relation_info['buildbot']['state'] == 'up':
-                    break
-                time.sleep(0.1)
-            try:
-                # XXX 2012-02-08 gmb:
-                #     This still sometimes gets called _before_ the
-                #     buildslave connects to the buildbot master. Hence
-                #     this rather ugly sleep here.
-                time.sleep(5)
-                stream = urllib2.urlopen(url)
-            except urllib2.HTTPError:
-                self.fail('Unable to connect to {}.'.format(url))
-            self.assertIn('Idle', stream.read())
-        finally:
-            juju('destroy-service', self.charm_name)
-            juju('destroy-service', master_charm_name)
+            stream = urllib2.urlopen(url)
+        except urllib2.HTTPError:
+            self.fail('Unable to connect to {}.'.format(url))
+        self.assertIn('Idle', stream.read())
+
 
 if __name__ == '__main__':
     unittest.main()
 
=== modified symlink 'tests/helpers.py'
=== target changed u'../../buildbot-master/hooks/helpers.py' => u'../hooks/helpers.py'
=== added symlink 'tests/test.cfg'
=== target is u'../../buildbot-master/tests/test.cfg'