Merge ~hloeung/jenkins-agent-charm:master into jenkins-agent-charm:master

[Haw Loeung]

Unit test all the things.

It's still missing unit tests for configure_nagios() which I'll work through next.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Joel Sing (jsing)]

The layer status change is a functional change, which I think should be split out and handled in a separate merge proposal to the testing additions (unless I'm missing the reason this cannot be done).

review: Needs Information

[Haw Loeung (hloeung)]

Okay, I've split out the change to layer-status to a separate MP - https://code.launchpad.net/~hloeung/jenkins-slave-charm/+git/jenkins-slave-charm/+merge/364411

review: Needs Resubmitting

[Stuart Bishop (stub)]

This all looks great.

The module mocks need to be reset in the TestCase's setUp method, rather than in the tests themselves.

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 98c0339c7227b49a74cd9583da140bd00b6ffd8b

diff --git a/pytest.ini b/pytest.ini
new file mode 100644
index 0000000..39da980
--- /dev/null
+++ b/pytest.ini
@@ -0,0 +1,6 @@
+[pytest]
+filterwarnings =
+    # https://github.com/juju/charm-helpers/issues/294
+    ignore:.*inspect.getargspec\(\) is deprecated.*:DeprecationWarning
+    # https://github.com/juju/charm-helpers/issues/293
+    ignore:.*dist\(\) and linux_distribution\(\) functions are deprecated:PendingDeprecationWarning
diff --git a/tests/unit/test_jenkins_slave.py b/tests/unit/test_jenkins_slave.py
index 7bd3ead..53e0e51 100644
--- a/tests/unit/test_jenkins_slave.py
+++ b/tests/unit/test_jenkins_slave.py
@@ -2,21 +2,33 @@ import grp
 import os
 import pwd
 import shutil
+import stat
 import sys
 import tempfile
 import unittest
 from unittest import mock
 
+
+# We also need to mock up charms.apt and charms.layer so we can run
+# unit tests without having to build the charm and pull in layers such
+# as layer-status.
 sys.modules['charms.apt'] = mock.MagicMock()
-from charms import apt  # NOQA: E402
 sys.modules['charms.layer'] = mock.MagicMock()
+from charms import apt  # NOQA: E402
+from charms.layer import status  # NOQA: E402
 
 # Add path to where our reactive layer lives and import.
 sys.path.append(os.path.dirname(os.path.dirname(os.path.dirname(os.path.realpath(__file__)))))
 from reactive.jenkins_slave import (
+    upgrade_charm,
     config_changed,
     install,
     configure_jenkins_slave,
+    blocked_on_jenkins_url,
+    set_active,
+    slave_relation_changed,
+    slave_relation_removed,
+    slave_relation,
     file_to_units,
     write_default_conf,
 )  # NOQA: E402
@@ -27,7 +39,9 @@ INITIAL_CONF = 'tests/unit/files/jenkins-slave-default'
 
 class TestSetDefaultConf(unittest.TestCase):
     def setUp(self):
+        self.maxDiff = None
         self.tmpdir = tempfile.mkdtemp(prefix='charm-unittests-')
+        self.addCleanup(shutil.rmtree, self.tmpdir)
         temp_file = tempfile.NamedTemporaryFile(delete=False, dir=self.tmpdir)
         with open(INITIAL_CONF, 'rb') as f:
             conf = f.read().decode('utf-8')
@@ -56,46 +70,103 @@ class TestSetDefaultConf(unittest.TestCase):
         self.addCleanup(patcher.stop)
         self.mock_local_unit.return_value = 'mock-jenkins-slave/0'
 
+        patcher = mock.patch('charmhelpers.core.hookenv.unit_private_ip')
+        self.mock_local_unit = patcher.start()
+        self.addCleanup(patcher.stop)
+        self.mock_local_unit.return_value = '10.1.2.3'
+
         patcher = mock.patch('charmhelpers.core.hookenv.log')
         self.mock_log = patcher.start()
         self.addCleanup(patcher.stop)
         self.mock_log.return_value = ''
 
-    def tearDown(self):
-        shutil.rmtree(self.tmpdir)
+        patcher = mock.patch('os.uname')
+        self.mock_log = patcher.start()
+        self.addCleanup(patcher.stop)
+        self.mock_log.return_value = ('Linux', 'mock-jenkins-slave', '4.15.0-46-generic',
+                                      '#49-Ubuntu SMP Wed Feb 6 09:33:07 UTC 2019', 'x86_64')
+
+        apt.reset_mock()
+        status.active.reset_mock()
+        status.maintenance.reset_mock()
+
+    @mock.patch('charms.reactive.clear_flag')
+    def test_hook_upgrade_charm_flags(self, clear_flag):
+        '''Test correct flags set via upgrade-charm hook'''
+        upgrade_charm()
+        self.assertFalse(status.maintenance.assert_called())
+        expected = [mock.call('jenkins-slave.active'), mock.call('jenkins-slave.installed')]
+        self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))
 
     @mock.patch('charms.reactive.clear_flag')
     def test_hook_config_changed(self, clear_flag):
+        '''Test correct flags are set via config-changed charm hook'''
         config_changed()
+        self.assertFalse(status.maintenance.assert_called())
         expected = [mock.call('jenkins-slave.blocked'),
                     mock.call('jenkins-slave.configured'),
                     mock.call('nagios-nrpe.configured')]
-        self.assertEqual(clear_flag.call_args_list, expected)
+        self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))
 
+    @mock.patch('charmhelpers.core.hookenv.config')
     @mock.patch('charmhelpers.core.host.adduser')
     @mock.patch('charmhelpers.core.host.mkdir')
     @mock.patch('charmhelpers.core.host.service')
+    @mock.patch('charmhelpers.core.host.user_exists')
     @mock.patch('reactive.jenkins_slave.apt_purge')
     @mock.patch('reactive.jenkins_slave.file_to_units')
     @mock.patch('reactive.jenkins_slave.write_default_conf')
-    def test_hook_install(self, write_default_conf, file_to_units, apt_purge, service, mkdir, adduser):
+    def test_hook_install(
+            self, write_default_conf, file_to_units, apt_purge, user_exists, service,
+            mkdir, adduser, config):
+        config.return_value = {'tools': 'some-tools-package'}
+        apt.install_queued.return_value = True
+        user_exists.return_value = False
         install()
+        self.assertFalse(status.maintenance.assert_called())
+
         expected = [mock.call(home_dir='/var/lib/jenkins', system_user=True, username='jenkins')]
-        self.assertEqual(adduser.call_args_list, expected)
+        self.assertFalse(adduser.assert_has_calls(expected, any_order=True))
         expected = [mock.call('/var/lib/jenkins', group='jenkins', owner='jenkins'),
                     mock.call('/var/log/jenkins', group='jenkins', owner='jenkins')]
-        self.assertEqual(mkdir.call_args_list, expected)
-        self.assertEqual(service.call_args_list, [mock.call('enable', 'jenkins-slave')])
-        self.assertEqual(apt_purge.call_args_list, [mock.call(['jenkins-slave'])])
+        self.assertFalse(mkdir.assert_has_calls(expected, any_order=True))
+        self.assertFalse(service.assert_has_calls([mock.call('enable', 'jenkins-slave')], any_order=True))
+        self.assertFalse(apt_purge.assert_has_calls([mock.call(['jenkins-slave'])], any_order=True))
         expected = [mock.call('files/download-slave.sh', '/usr/local/sbin/download-slave.sh'),
                     mock.call('files/jenkins-slave-logrotate-config', '/etc/logrotate.d/jenkins-slave'),
                     mock.call('files/jenkins-slave-systemd-config', '/lib/systemd/system/jenkins-slave.service')]
-        self.assertEqual(file_to_units.call_args_list, expected)
-        self.assertEqual(write_default_conf.call_args_list, [mock.call()])
-        expected = [mock.call.queue_install(['wget', 'default-jre-headless']),
+        self.assertFalse(file_to_units.assert_has_calls(expected, any_order=True))
+        self.assertFalse(write_default_conf.assert_called())
+        expected = [mock.call.queue_install(['wget', 'default-jre-headless', 'some-tools-package']),
                     mock.call.install_queued()]
         self.assertEqual(apt.method_calls, expected)
 
+        apt.install_queued.return_value = False
+        apt_purge.reset_mock()
+        install()
+        self.assertFalse(apt_purge.assert_not_called())
+
+        apt.install_queued.return_value = True
+        user_exists.return_value = True
+        adduser.reset_mock()
+        install()
+        self.assertFalse(adduser.assert_not_called())
+
+    @mock.patch('charmhelpers.core.host.lsb_release')
+    @mock.patch('charmhelpers.core.host.adduser')
+    @mock.patch('charmhelpers.core.host.mkdir')
+    @mock.patch('reactive.jenkins_slave.file_to_units')
+    @mock.patch('reactive.jenkins_slave.write_default_conf')
+    def test_hook_install_trusty(self, write_default_conf, file_to_units, mkdir, adduser, lsb_release):
+        apt.install_queued.return_value = True
+        lsb_release.return_value = {'DISTRIB_CODENAME': 'trusty'}
+        install()
+        self.assertFalse(status.maintenance.assert_called())
+
+        # file_to_units('files/jenkins-slave-upstart-config', '/etc/init/jenkins-slave.conf')
+        expected = [mock.call('files/jenkins-slave-upstart-config', '/etc/init/jenkins-slave.conf')]
+        self.assertFalse(file_to_units.assert_has_calls(expected, any_order=True))
+
     @mock.patch('charms.reactive.clear_flag')
     @mock.patch('charms.reactive.set_flag')
     @mock.patch('charmhelpers.core.hookenv.config')
@@ -107,9 +178,9 @@ class TestSetDefaultConf(unittest.TestCase):
         config.return_value = {}
         unitdata_kv.return_value = {}
         configure_jenkins_slave()
-        self.assertEqual(write_default_conf.call_args_list, [])
-        self.assertEqual(set_flag.call_args_list, [mock.call('jenkins-slave.blocked')])
-        self.assertEqual(clear_flag.call_args_list, [mock.call('jenkins-slave.active')])
+        self.assertFalse(write_default_conf.assert_not_called())
+        self.assertFalse(set_flag.assert_has_calls([mock.call('jenkins-slave.blocked')], any_order=True))
+        self.assertFalse(clear_flag.assert_has_calls([mock.call('jenkins-slave.active')], any_order=True))
 
     @mock.patch('charms.reactive.clear_flag')
     @mock.patch('charms.reactive.set_flag')
@@ -122,10 +193,10 @@ class TestSetDefaultConf(unittest.TestCase):
         config.return_value = {'master_url': 'http://10.1.1.1:8080'}
         unitdata_kv.return_value = {}
         configure_jenkins_slave()
-        self.assertEqual(write_default_conf.call_args_list, [mock.call('http://10.1.1.1:8080')])
-        self.assertEqual(set_flag.call_args_list, [mock.call('jenkins-slave.configured')])
+        self.assertFalse(write_default_conf.assert_called_once_with('http://10.1.1.1:8080'))
+        self.assertFalse(set_flag.assert_has_calls([mock.call('jenkins-slave.configured')], any_order=True))
         expected = [mock.call('jenkins-slave.active'), mock.call('nagios-nrpe.configured')]
-        self.assertEqual(clear_flag.call_args_list, expected)
+        self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))
 
     @mock.patch('charms.reactive.clear_flag')
     @mock.patch('charms.reactive.set_flag')
@@ -138,11 +209,107 @@ class TestSetDefaultConf(unittest.TestCase):
         config.return_value = {}
         unitdata_kv.return_value = {'url': 'http://10.22.22.22:8080'}
         configure_jenkins_slave()
-        print(write_default_conf.call_args_list)
-        self.assertEqual(write_default_conf.call_args_list, [mock.call('http://10.22.22.22:8080')])
-        self.assertEqual(set_flag.call_args_list, [mock.call('jenkins-slave.configured')])
+        self.assertTrue(write_default_conf.called_once_with('http://10.22.22.22:8080'))
+        self.assertFalse(set_flag.assert_has_calls([mock.call('jenkins-slave.configured')], any_order=True))
         expected = [mock.call('jenkins-slave.active'), mock.call('nagios-nrpe.configured')]
-        self.assertEqual(clear_flag.call_args_list, expected)
+        self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))
+
+    @mock.patch('charms.reactive.clear_flag')
+    def test_blocked_on_jenkins_url(self, clear_flag):
+        blocked_on_jenkins_url()
+        self.assertFalse(clear_flag.assert_has_calls([mock.call('jenkins-slave.active')], any_order=True))
+
+    @mock.patch('charmhelpers.core.host.service_running')
+    @mock.patch('charmhelpers.core.host.service_restart')
+    @mock.patch('charmhelpers.core.host.service_start')
+    def test_set_active_running(self, service_start, service_restart, service_running):
+        '''Test service restarted when already running'''
+        service_running.return_value = True
+        set_active()
+        self.assertFalse(status.maintenance.assert_called())
+        self.assertFalse(service_start.assert_not_called())
+        self.assertFalse(service_restart.assert_called_once_with('jenkins-slave'))
+
+    @mock.patch('charmhelpers.core.host.service_running')
+    @mock.patch('charmhelpers.core.host.service_restart')
+    @mock.patch('charmhelpers.core.host.service_start')
+    def test_set_active_not_running(self, service_start, service_restart, service_running):
+        '''Test service restarted when not running'''
+        service_running.return_value = False
+        set_active()
+        self.assertFalse(status.maintenance.assert_called())
+        self.assertFalse(service_start.assert_called_once_with('jenkins-slave'))
+        self.assertFalse(service_restart.assert_not_called())
+
+    @mock.patch('charms.reactive.clear_flag')
+    @mock.patch('charms.reactive.set_flag')
+    def test_hook_slave_relation_changed_flags(self, set_flag, clear_flag):
+        slave_relation_changed()
+        expected = [
+            mock.call('jenkins-slave.blocked'),
+            mock.call('jenkins-slave.configured'),
+            mock.call('slave-relation.configured'),
+        ]
+        self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))
+        self.assertFalse(set_flag.assert_has_calls([mock.call('slave-relation.available')], any_order=True))
+
+    @mock.patch('charms.reactive.clear_flag')
+    def test_hook_slave_relation_removed_flags(self, clear_flag):
+        slave_relation_removed()
+        self.assertFalse(clear_flag.assert_has_calls([mock.call('slave-relation.available')], any_order=True))
+
+    @mock.patch('charms.reactive.clear_flag')
+    @mock.patch('charms.reactive.set_flag')
+    @mock.patch('charmhelpers.core.hookenv.config')
+    @mock.patch('charmhelpers.core.hookenv.relation_get')
+    @mock.patch('charmhelpers.core.hookenv.relation_set')
+    @mock.patch('charmhelpers.core.unitdata.kv')
+    @mock.patch('reactive.jenkins_slave.write_default_conf')
+    def test_hook_slave_relation(
+            self, write_default_conf, unitdata_kv, relation_set, relation_get,
+            config, set_flag, clear_flag):
+        config.return_value = {'master_url': ''}
+        relation_get.return_value = 'http://10.1.1.1:8080'
+        slave_relation()
+        self.assertFalse(relation_get.assert_called_once_with('url'))
+        self.assertFalse(clear_flag.assert_called_once_with('jenkins-slave.active'))
+        self.assertFalse(set_flag.assert_called_once_with('slave-relation.configured'))
+        expected = [mock.call(executors=2),
+                    mock.call(labels='x86_64'),
+                    mock.call(slavehost='mock-jenkins-slave-0'),
+                    mock.call(slaveaddress='10.1.2.3')]
+        self.assertFalse(relation_set.assert_has_calls(expected, any_order=True))
+
+        config.return_value = {'master_url': '', 'labels': 'label1 label2'}
+        relation_get.return_value = 'http://10.1.1.1:8080'
+        relation_set.reset_mock()
+        slave_relation()
+        expected = [mock.call(executors=2),
+                    mock.call(labels='label1 label2'),
+                    mock.call(slavehost='mock-jenkins-slave-0'),
+                    mock.call(slaveaddress='10.1.2.3')]
+        self.assertFalse(relation_set.assert_has_calls(expected, any_order=True))
+
+    @mock.patch('charmhelpers.core.hookenv.config')
+    @mock.patch('charmhelpers.core.hookenv.relation_get')
+    @mock.patch('charms.reactive.set_flag')
+    def test_hook_slave_relation_master_url_set(self, set_flag, relation_get, config):
+        config.return_value = {'master_url': 'http://10.1.1.1:8080'}
+        relation_get.return_value = ''
+        slave_relation()
+        self.assertFalse(relation_get.assert_not_called())
+        self.assertFalse(set_flag.assert_called_once_with('slave-relation.configured'))
+
+    @mock.patch('charms.reactive.clear_flag')
+    @mock.patch('charms.reactive.set_flag')
+    @mock.patch('charmhelpers.core.hookenv.config')
+    @mock.patch('charmhelpers.core.hookenv.relation_get')
+    def test_hook_slave_relation_relation_url_not_set(
+            self, relation_get, config, set_flag, clear_flag):
+        config.return_value = {'master_url': ''}
+        relation_get.return_value = ''
+        slave_relation()
+        self.assertFalse(clear_flag.assert_not_called())
 
     def test_write_default_conf_update(self):
         write_default_conf('http://10.1.1.1:8080', self.user, self.group, self.conf_file)
@@ -196,6 +363,17 @@ class TestSetDefaultConf(unittest.TestCase):
         self.assertEqual(grp.getgrgid(os.stat(dest).st_gid).gr_name, self.group)
         self.assertFalse(os.access(dest, os.X_OK))
 
+    def test_file_to_units_with_perms(self):
+        source = os.path.join(self.charm_dir, 'tests/unit/files/somefile')
+        dest = os.path.join(self.tmpdir, os.path.basename(source))
+        file_to_units(source, dest, owner=self.user, group=self.group, perms=0o640)
+        with open(dest, 'rb') as fh:
+            want = fh.read().decode('utf-8')
+        self.assertTrue(conf_equals(source, want))
+        self.assertEqual(pwd.getpwuid(os.stat(dest).st_uid).pw_name, self.user)
+        self.assertEqual(grp.getgrgid(os.stat(dest).st_gid).gr_name, self.group)
+        self.assertEqual(os.stat(dest)[stat.ST_MODE], 0o100640)
+
 
 def conf_equals(conf_file, want):
     with open(conf_file, 'rb') as conf:
diff --git a/tox.ini b/tox.ini
index ee5dbeb..d6b7675 100644
--- a/tox.ini
+++ b/tox.ini
@@ -9,7 +9,7 @@ setenv =
   PYTHONPATH = .
 
 [testenv:unit]
-commands = pytest -v --ignore {toxinidir}/tests/functional --cov=lib --cov=reactive --cov=actions --cov-report=term
+commands = pytest -v --ignore {toxinidir}/tests/functional --cov=lib --cov=reactive --cov=actions --cov-report=term-missing --cov-branch
 deps = -r{toxinidir}/tests/unit/requirements.txt
        -r{toxinidir}/requirements.txt
 setenv = PYTHONPATH={toxinidir}/lib
@@ -32,6 +32,5 @@ exclude =
     .git,
     __pycache__,
     .tox,
-    hooks/install.d/,
 max-line-length = 120
 max-complexity = 10