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

[James Westby]

Turn the unpacking in to a check that the package is a valid click package.

Hi,

Previously if the unpack failed then there would be an oops and no
results reported to sca. This changes that so that if it fails then
it is reported as not being a valid click package.

Thanks,

James

[Matias Bordese (matiasb)]

(06:31:50 PM) matiasb: james_w: looks good, just one question, why are the changes in l.93-98 required?
(06:32:16 PM) james_w: hmm
(06:32:23 PM) james_w: I should probably extract that in to a method
(06:32:32 PM) james_w: the code under test does
(06:32:38 PM) james_w: with TempDir() as tempdir:
(06:32:43 PM) james_w: to create a temporary directory
(06:33:06 PM) james_w: and we need to fill that tempdir with stuff as part of mocking the download/extract methods
(06:33:14 PM) james_w: there are two ways we could do this:
(06:33:32 PM) james_w: 1. fill it as a side_effect of the extract mock, exactly like it would be for real
(06:33:55 PM) james_w: but this makes it impossible to assert that the correct path is passed to the download mock
(06:34:05 PM) james_w: (l.105)
(06:34:29 PM) james_w: so I chose 2. mock the TempDir() fixture to return a known directory that is pre-filled with the stuff we need
(06:34:47 PM) james_w: it's so ugly because it's mocking a return from a class that is a content manager that returns itself
(06:34:59 PM) james_w: I couldn't find a nicer way to mock it
(06:35:03 PM) james_w: suggestions welcome
(06:36:19 PM) james_w: (the previous approach to mocking it wasn't working right, as when __enter__ was called on the TempDir object we were setting in the mock, it created a new temporary directory, meaning the files we created then weren't found by the code under test)
(06:36:45 PM) james_w: at the very least I should make that a replace_tempdir method or something to make the intent clearer
(06:37:13 PM) matiasb: hmm... I see, sounds good; no brain for suggestions at this time of the day, with that change, works for me :)

review: Approve

[lp:~james-w/pkgme-service/review-click]

146. By James Westby on 2013-10-30

[r=matiasb] Turn the unpacking in to a check that the package is a valid click package.

=== modified file 'src/djpkgme/tasks.py'
--- src/djpkgme/tasks.py	2013-10-16 13:30:05 +0000
+++ src/djpkgme/tasks.py	2013-10-30 21:15:22 +0000
@@ -382,16 +382,27 @@
 
     def unpack_package(self, package_file, temp_dir):
         dest = os.path.join(temp_dir, 'unpacked')
-        subprocess.check_call(['dpkg-deb', '-R', package_file, dest])
-        return dest
+        passed = True
+        try:
+            subprocess.check_call(['dpkg-deb', '-R', package_file, dest])
+        except subprocess.CalledProcessError:
+            passed = False
+            dest = None
+        return dest, {
+            'description': 'Is a valid click package',
+            'passed': passed,
+        }
 
     def run(self, metadata, overrides, logger):
         working_dirs = []
         results = []
         with TempDir() as temp_dir:
             package_file = self.download_package(metadata, temp_dir.path, logger)
-            unpacked = self.unpack_package(package_file, temp_dir.path)
-            results.append(self.check_package_name_matches(metadata, unpacked))
+            unpacked, unpack_result = self.unpack_package(
+                package_file, temp_dir.path)
+            results.append(unpack_result)
+            if unpack_result['passed']:
+                results.append(self.check_package_name_matches(metadata, unpacked))
             return results
 
     def check_package_name_matches(self, metadata, package):

=== modified file 'src/djpkgme/tests/test_tasks.py'
--- src/djpkgme/tests/test_tasks.py	2013-10-16 13:30:05 +0000
+++ src/djpkgme/tests/test_tasks.py	2013-10-30 21:15:22 +0000
@@ -4,6 +4,7 @@
 import json
 import logging
 import os
+import subprocess
 import sys
 import tarfile
 import traceback
@@ -475,8 +476,24 @@
         package_file = 'some/package'
         temp_dir = 'temp/dir'
         unpack_path = os.path.join(temp_dir, 'unpacked')
-        returned_path = job.unpack_package(package_file, temp_dir)
+        returned_path, check = job.unpack_package(package_file, temp_dir)
         self.assertEqual(unpack_path, returned_path)
+        self.assertEqual(
+            check, dict(passed=True, description="Is a valid click package"))
+        mock_call.assert_called_with(
+            ['dpkg-deb', '-R', package_file, unpack_path])
+
+    @patch('djpkgme.tasks.subprocess.check_call')
+    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')
+        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"))
         mock_call.assert_called_with(
             ['dpkg-deb', '-R', package_file, unpack_path])
 
@@ -507,11 +524,25 @@
         result = job.check_package_name_matches(metadata, tempdir)
         self.assertEqual(False, result['passed'])
 
-    @patch('djpkgme.tasks.ReviewClickJob.unpack_package')
+    def make_tempdir_fixture_mock(self, path):
+        """Return a mock suitable for mocking the fixtures.TempDir class.
+
+        The mock will return the passed path as the tempdir path.
+
+        It also handles the result being used as a context manager.
+        """
+        tempdir_mock = mock.Mock()
+        path_holder = mock.Mock()
+        path_holder.path = path
+        tempdir_mock.__enter__ = mock.Mock(return_value=path_holder)
+        tempdir_mock.__exit__ = mock.Mock(return_value=False)
+        return tempdir_mock
+
+    @patch('djpkgme.tasks.subprocess.check_call')
     @patch('djpkgme.tasks.ReviewClickJob.download_package')
     @patch('djpkgme.tasks.TempDir')
     def test_integration(self, mock_tempdir, mock_download_package,
-                         mock_unpack_package):
+                         mock_call):
         tempdir = self.useFixture(TempDir())
         downloaded_path = 'some/path'
         unpacked_path = os.path.join(tempdir.path, 'unpacked')
@@ -521,15 +552,16 @@
         logger = logging.getLogger()
         self.make_manifest_file(dict(name=package_fullname), unpacked_path)
 
-        mock_tempdir.return_value = tempdir
+        mock_tempdir.return_value = self.make_tempdir_fixture_mock(
+            tempdir.path)
         mock_download_package.return_value = downloaded_path
-        mock_unpack_package.return_value = unpacked_path
 
         job = ReviewClickJob()
         results = job.run(metadata, {}, logger)
 
         mock_download_package.assert_called_with(
             metadata, tempdir.path, logger)
-        mock_unpack_package.assert_called_with(downloaded_path, tempdir.path)
-        self.assertEqual(1, len(results))
+        mock_call.assert_called_with(
+            ['dpkg-deb', '-R', downloaded_path, unpacked_path])
+        self.assertEqual(2, len(results))
         self.assertEqual(True, results[0]['passed'])