Giuseppe Lavagetto has submitted this change and it was merged. ( 
https://gerrit.wikimedia.org/r/363351 )

Change subject: Raise test coverage percentage
......................................................................


Raise test coverage percentage

Also, as a consequence:
* removed some dead code
* fixed a few errors here and there

Change-Id: Id04f2283df66f3742536367d814b9cccc898c7a7
---
M puppet_compiler/controller.py
M puppet_compiler/prepare.py
A puppet_compiler/tests/fixtures/test_config.yaml.invalid
M puppet_compiler/tests/test_controller.py
M puppet_compiler/tests/test_directories.py
M puppet_compiler/tests/test_hostworker.py
M puppet_compiler/tests/test_prepare.py
7 files changed, 88 insertions(+), 20 deletions(-)

Approvals:
  Giuseppe Lavagetto: Looks good to me, approved
  Alexandros Kosiaris: Looks good to me, but someone else must approve
  jenkins-bot: Verified



diff --git a/puppet_compiler/controller.py b/puppet_compiler/controller.py
index 1d967a5..f2b61b3 100644
--- a/puppet_compiler/controller.py
+++ b/puppet_compiler/controller.py
@@ -45,7 +45,7 @@
         try:
             if configfile is not None:
                 self._parse_conf(configfile)
-        except yaml.parser.ParserError as e:
+        except yaml.error.YAMLError as e:
             _log.exception("Configuration file %s contains malformed yaml: %s",
                            configfile, e)
             sys.exit(2)
diff --git a/puppet_compiler/prepare.py b/puppet_compiler/prepare.py
index 670c6bc..c63dfd1 100644
--- a/puppet_compiler/prepare.py
+++ b/puppet_compiler/prepare.py
@@ -36,14 +36,6 @@
         self.output_dir = FHS.output_dir
         self.git = Git()
 
-    def find_yaml(self, hostname):
-        """ Finds facts file for the current hostname """
-        facts_file = os.path.join(self.puppet_var, 'yaml', 'facts',
-                                  '{}.yaml'.format(hostname))
-        if os.path.isfile(facts_file):
-            return facts_file
-        return None
-
     def cleanup(self):
         """
         Remove the whole change tree.
@@ -216,14 +208,6 @@
 
     def _pull_rebase_origin(self, origin_branch):
         self.git.pull('--rebase', 'origin', origin_branch)
-
-    def _sh(command):
-        try:
-            subprocess.check_call(command, shell=True)
-        except subprocess.CalledProcessError as e:
-            _log.error("Command '%s' failed with exit code '%s'",
-                       e.cmd, e.returncode)
-            raise
 
 
 class Git():
diff --git a/puppet_compiler/tests/fixtures/test_config.yaml.invalid 
b/puppet_compiler/tests/fixtures/test_config.yaml.invalid
new file mode 100644
index 0000000..30e3d73
--- /dev/null
+++ b/puppet_compiler/tests/fixtures/test_config.yaml.invalid
@@ -0,0 +1,3 @@
+http_url: http://www.example.com/garbagehere
+test_non_existent:
+  - 'definitely': - 'maybe'
diff --git a/puppet_compiler/tests/test_controller.py 
b/puppet_compiler/tests/test_controller.py
index 7066e84..c3291e1 100644
--- a/puppet_compiler/tests/test_controller.py
+++ b/puppet_compiler/tests/test_controller.py
@@ -24,6 +24,9 @@
         self.assertEquals(len(c.config['test_non_existent']), 2)
         self.assertEquals(c.config['http_url'],
                           'http://www.example.com/garbagehere')
+        # This will log an error, but not raise an exception
+        controller.Controller('unexistent', 19, 224570, 'test.eqiad.wmnet', 
nthreads=2)
+        self.assertRaises(SystemExit, controller.Controller, filename + 
'.invalid', 1, 1, 'test.eqiad.wmnet')
 
     @mock.patch('puppet_compiler.worker.HostWorker.html_index')
     @mock.patch('puppet_compiler.worker.HostWorker.run_host')
@@ -39,6 +42,13 @@
         c.m.prepare.assert_called_once_with()
         c.m.refresh.assert_not_called()
         self.assertEquals(c.state.modes['change']['fail'], 
set(['test.eqiad.wmnet']))
+        c.m.refresh.reset_mocks()
+        c.config['puppet_src'] = '/src'
+        c.config['puppet_private'] = '/private'
+        mocker.return_value = (False, False, None)
+        c.run()
+        c.m.refresh.assert_has_calls([mock.call('/src'), 
mock.call('/private')])
+        self.assertEquals(c.state.modes['change']['noop'], 
set(['test.eqiad.wmnet']))
 
     def test_node_callback(self):
         c = controller.Controller(None, 19, 224570, 'test.eqiad.wmnet')
@@ -60,6 +70,17 @@
         c.on_node_compiled(response)
         self.assertIn('test2.eqiad.wmnet', c.state.modes['change']['noop'])
         self.assertEquals(c.count['change'], 7)
+        with mock.patch('puppet_compiler.presentation.html.Index') as mocker:
+            response = threads.Msg(
+                is_error=False, value=(False, False, None), args=None,
+                kwargs={
+                    'hostname': 'test2.eqiad.wmnet',
+                    'classes': (state.ChangeState, html.Index),
+                    'mode': 'change'}
+            )
+            c.count['change'] = 4
+            c.on_node_compiled(response)
+            mocker.assert_called_with(c.outdir, mode='change')
 
     def test_pick_hosts(self):
         # Initialize a simple controller
@@ -93,3 +114,12 @@
             c.pick_hosts('test.eqiad.wmnet,test.tools.eqiad.wmflabs')
 
         self.assertEqual(cm.exception.code, 2)
+
+    def test_success(self):
+        c = controller.Controller(None, 19, 224570, 'test.eqiad.wmnet')
+        # let's add just a successful run
+        c.count['change'] = 2
+        c.state.modes = {'change': {'noop': 1}}
+        self.assertTrue(c.success)
+        c.count['change'] = 0
+        self.assertTrue(c.success)
diff --git a/puppet_compiler/tests/test_directories.py 
b/puppet_compiler/tests/test_directories.py
index 8b1e651..6981330 100644
--- a/puppet_compiler/tests/test_directories.py
+++ b/puppet_compiler/tests/test_directories.py
@@ -39,3 +39,9 @@
         )
         # Now something we don't have
         self.assertRaises(ValueError, self.hostfiles.file_for, 'test', 'none')
+
+    def test_outfile_for(self):
+        self.assertEqual(
+            self.hostfiles.outfile_for('prod', 'catalog'),
+            
'/mnt/jenkins-workspace/output/19/test.example.com/prod.test.example.com.pson'
+        )
diff --git a/puppet_compiler/tests/test_hostworker.py 
b/puppet_compiler/tests/test_hostworker.py
index 1a6fd19..8405ec3 100644
--- a/puppet_compiler/tests/test_hostworker.py
+++ b/puppet_compiler/tests/test_hostworker.py
@@ -18,6 +18,15 @@
         self.assertEquals(self.hw.hostname, 'test.example.com')
         self.assertItemsEqual(['prod', 'change'], self.hw._envs)
 
+    @mock.patch('os.path.isfile')
+    def test_facts_file(self, isfile):
+        isfile.return_value = True
+        self.assertEqual(
+            self.hw.facts_file(),
+            '/var/lib/catalog-differ/puppet/yaml/facts/test.example.com.yaml')
+        isfile.return_value = False
+        self.assertIsNone(self.hw.facts_file())
+
     @mock.patch('puppet_compiler.puppet.compile')
     def test_compile_all(self, mocker):
         # Verify simple calls
@@ -71,9 +80,17 @@
         mock_makedirs.assert_called_with(self.hw._files.outdir, 0755)
         assert not mock_copy.called
         mock_isfile.return_value = True
-        mock_copy.assert_called_any(
-            
'/mnt/jenkins-workspace/19/production/catalogs/test.example.com.pson',
-            '/what/you/want/test.example.com/production.test.example.com.pson',
+        self.hw._get_diff = mock.MagicMock(return_value=False)
+        self.hw._make_output()
+        mock_copy.assert_called_with(
+            '/mnt/jenkins-workspace/19/change/catalogs/test.example.com.err',
+            
'/mnt/jenkins-workspace/output/19/test.example.com/change.test.example.com.err',
+        )
+        self.hw._get_diff = mock.MagicMock(return_value='test_value')
+        self.hw._make_output()
+        mock_copy.assert_called_with(
+            'test_value',
+            '/mnt/jenkins-workspace/output/19/test.example.com'
         )
 
     @mock.patch('os.path')
@@ -107,3 +124,6 @@
         self.hw._make_diff.reset_mock()
         self.assertEquals(self.hw.run_host(), (True, False, None))
         assert not self.hw._make_diff.called
+        # An exception writing the output doesn't make the payload fail
+        self.hw._make_output.side_effect = Exception('Boom!')
+        self.assertEquals(self.hw.run_host(), (True, False, None))
diff --git a/puppet_compiler/tests/test_prepare.py 
b/puppet_compiler/tests/test_prepare.py
index 7c602e4..5a74468 100644
--- a/puppet_compiler/tests/test_prepare.py
+++ b/puppet_compiler/tests/test_prepare.py
@@ -96,6 +96,9 @@
             mock.call(['git', 'submodule', 'update', '--init'])
         ]
         mocker.assert_has_calls(calls)
+        # Now test a change to another repository
+        self.m.change_id = 363216
+        self.assertRaises(RuntimeError, self.m._fetch_change)
 
     @mock.patch('subprocess.call')
     @mock.patch('os.chdir', return_value=None)
@@ -141,3 +144,25 @@
         exim_pub = os.path.join(self.m.prod_dir,
                                 'src/modules/privateexim')
         mock_symlink.assert_any_call(exim_priv, exim_pub)
+
+    @mock.patch('puppet_compiler.prepare.pushd')
+    def test_refresh(self, pushd):
+        self.m.git = mock.MagicMock()
+        self.m.refresh('/__test')
+        pushd.assert_called_with('/__test')
+        self.m.git.pull.assert_called_with('-q', '--rebase')
+
+    @mock.patch('puppet_compiler.prepare.pushd')
+    def test_prepare(self, pushd):
+        self.m._prepare_dir = mock.MagicMock()
+        self.m._fetch_change = mock.MagicMock()
+        self.m._copy_hiera = mock.MagicMock()
+        self.m._create_puppetconf = mock.MagicMock()
+        self.m.prepare()
+        for dirname in self.m.base_dir, self.m.prod_dir, self.m.change_dir:
+            assert os.path.isdir(dirname)
+        self.m._prepare_dir.assert_has_calls(
+            [mock.call(self.m.prod_dir)],
+            [mock.call(self.m.change_dir)],
+        )
+        assert self.m._fetch_change.called

-- 
To view, visit https://gerrit.wikimedia.org/r/363351
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Id04f2283df66f3742536367d814b9cccc898c7a7
Gerrit-PatchSet: 6
Gerrit-Project: operations/software/puppet-compiler
Gerrit-Branch: master
Gerrit-Owner: Giuseppe Lavagetto <glavage...@wikimedia.org>
Gerrit-Reviewer: Alexandros Kosiaris <akosia...@wikimedia.org>
Gerrit-Reviewer: Giuseppe Lavagetto <glavage...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to