Merge lp:~james-w/pkgme-service/review-click into lp:pkgme-service

[James Westby]

Add an explanation when either of the click review checks fails.

Hi,

The commit message should explain this change.

Thanks,

James

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

=== modified file 'src/djpkgme/tasks.py'
--- src/djpkgme/tasks.py	2013-10-16 21:21:20 +0000
+++ src/djpkgme/tasks.py	2013-10-31 19:53:16 +0000
@@ -38,6 +38,13 @@
     )
 
 
+def indent(text, distance):
+    """Indent a block of text by `distance` spaces."""
+    output = distance * ' '
+    output += text.replace('\n', '\n' + distance * ' ')
+    return output
+
+
 class MissingFieldsError(Exception):
     """Raised when we're asked to do a job with missing data."""
 
@@ -382,16 +389,20 @@
 
     def unpack_package(self, package_file, temp_dir):
         dest = os.path.join(temp_dir, 'unpacked')
-        passed = True
+        result = {
+            'description': 'Is a valid click package',
+            'passed': True,
+        }
         try:
-            subprocess.check_call(['dpkg-deb', '-R', package_file, dest])
-        except subprocess.CalledProcessError:
-            passed = False
+            subprocess.check_output(['dpkg-deb', '-R', package_file, dest],
+                                    stderr=subprocess.STDOUT)
+        except subprocess.CalledProcessError, e:
+            result['passed'] = False
+            explanation = 'Extracting the package with dpkg failed:\n\n'
+            explanation += indent(e.output, 4)
+            result['explanation'] = explanation
             dest = None
-        return dest, {
-            'description': 'Is a valid click package',
-            'passed': passed,
-        }
+        return dest, result
 
     def run(self, metadata, overrides, logger):
         working_dirs = []
@@ -406,13 +417,18 @@
             return results
 
     def check_package_name_matches(self, metadata, package):
+        result = {
+            'description': 'Stated package name matches the manifest',
+            'passed': True,
+        }
         with open(os.path.join(package, 'DEBIAN/manifest')) as f:
             manifest_content = json.loads(f.read())
-            passed = manifest_content['name'] == metadata['package_fullname']
-        return {
-            'description': 'Stated package name matches the manifest',
-            'passed': passed,
-        }
+            if manifest_content['name'] != metadata['package_fullname']:
+                result['passed'] = False
+                result['explanation'] = '''Package name in the manifest: `{}`
+Package name in My Apps: `{}`
+'''.format(manifest_content['name'], metadata['package_fullname'])
+        return result
 
 
 class CauseOopsTask(Task):

=== modified file 'src/djpkgme/tests/test_tasks.py'
--- src/djpkgme/tests/test_tasks.py	2013-10-30 21:14:05 +0000
+++ src/djpkgme/tests/test_tasks.py	2013-10-31 19:53:16 +0000
@@ -34,6 +34,7 @@
     BuildPackageTask,
     CallbackError,
     get_url_for_file,
+    indent,
     PackageAppJob,
     prepare_icons,
     prepare_metadata,
@@ -50,6 +51,22 @@
 from djpkgme.tests.helpers import FakeResponse
 
 
+class TestIndent(TestCase):
+
+    def test_empty_string(self):
+        self.assertEqual('    ', indent('', 4))
+
+    def test_distance(self):
+        self.assertEqual('    ', indent('', 4))
+        self.assertEqual('     ', indent('', 5))
+
+    def test_multiline(self):
+        self.assertEqual('    a\n    b', indent('a\nb', 4))
+
+    def test_trailing_newline(self):
+        self.assertEqual('    a\n    ', indent('a\n', 4))
+
+
 class TestTranslateDict(TestCase):
 
     def test_default(self):
@@ -470,7 +487,7 @@
         mock_download_file.assert_called_with(metadata['package_url'],
             temp_dir)
 
-    @patch('djpkgme.tasks.subprocess.check_call')
+    @patch('djpkgme.tasks.subprocess.check_output')
     def test_unpack_package(self, mock_call):
         job = ReviewClickJob()
         package_file = 'some/package'
@@ -481,21 +498,29 @@
         self.assertEqual(
             check, dict(passed=True, description="Is a valid click package"))
         mock_call.assert_called_with(
-            ['dpkg-deb', '-R', package_file, unpack_path])
+            ['dpkg-deb', '-R', package_file, unpack_path],
+            stderr=subprocess.STDOUT)
 
-    @patch('djpkgme.tasks.subprocess.check_call')
+    @patch('djpkgme.tasks.subprocess.check_output')
     def test_unpack_package_fails(self, mock_call):
         job = ReviewClickJob()
         package_file = 'some/package'
         temp_dir = 'temp/dir'
         unpack_path = os.path.join(temp_dir, 'unpacked')
-        mock_call.side_effect = subprocess.CalledProcessError(1, 'command')
+        mock_call.side_effect = subprocess.CalledProcessError(1, 'command',
+            "Command failed\nwith some problem\n")
         returned_path, check = job.unpack_package(package_file, temp_dir)
         self.assertEqual(None, returned_path)
-        self.assertEqual(
-            check, dict(passed=False, description="Is a valid click package"))
+        expected_result = dict(passed=False, description="Is a valid click package")
+        expected_result['explanation'] = """Extracting the package with dpkg failed:
+
+    Command failed
+    with some problem
+    """
+        self.assertEqual(check, expected_result)
         mock_call.assert_called_with(
-            ['dpkg-deb', '-R', package_file, unpack_path])
+            ['dpkg-deb', '-R', package_file, unpack_path],
+            stderr=subprocess.STDOUT)
 
     def make_manifest_file(self, manifest_content, directory):
         debian_dir = os.path.join(directory, 'DEBIAN')
@@ -518,11 +543,16 @@
     def test_check_package_name_doesnt_match(self):
         job = ReviewClickJob()
         package_name = 'some.package'
+        other_package_name = 'some.other.package'
         metadata = dict(package_fullname=package_name)
         tempdir = self.useFixture(TempDir()).path
-        self.make_manifest_file(dict(name='some.other.package'), tempdir)
+        self.make_manifest_file(dict(name=other_package_name), tempdir)
         result = job.check_package_name_matches(metadata, tempdir)
         self.assertEqual(False, result['passed'])
+        explanation = '''Package name in the manifest: `{}`
+Package name in My Apps: `{}`
+'''.format(other_package_name, package_name)
+        self.assertEqual(explanation, result['explanation'])
 
     def make_tempdir_fixture_mock(self, path):
         """Return a mock suitable for mocking the fixtures.TempDir class.
@@ -538,7 +568,7 @@
         tempdir_mock.__exit__ = mock.Mock(return_value=False)
         return tempdir_mock
 
-    @patch('djpkgme.tasks.subprocess.check_call')
+    @patch('djpkgme.tasks.subprocess.check_output')
     @patch('djpkgme.tasks.ReviewClickJob.download_package')
     @patch('djpkgme.tasks.TempDir')
     def test_integration(self, mock_tempdir, mock_download_package,
@@ -562,6 +592,7 @@
         mock_download_package.assert_called_with(
             metadata, tempdir.path, logger)
         mock_call.assert_called_with(
-            ['dpkg-deb', '-R', downloaded_path, unpacked_path])
+            ['dpkg-deb', '-R', downloaded_path, unpacked_path],
+            stderr=subprocess.STDOUT)
         self.assertEqual(2, len(results))
         self.assertEqual(True, results[0]['passed'])