Merge ~jslarraz/review-tools:work-on-test-cases into review-tools:master

[Jorge Sancho Larraz]

[Jorge Sancho Larraz (jslarraz)]

Move check for container format to BaseContainer initialization and properly mock required functions: https://git.launchpad.net/~jslarraz/review-tools/commit/?id=9477ee6500544e63ed46ea19ab1486c8f4cf268c

Remove unneeded exception handling on error cases from tests classes (makes code cleaner and improves coverage): https://git.launchpad.net/~jslarraz/review-tools/commit/?id=acb74aab8dab0c2d0eedb354adda7bb0ea10e295

Add missing tests case for missing extension on container file name and mock _unpack_dir: https://git.launchpad.net/~jslarraz/review-tools/commit/?id=037aefd2f9dadec3b3734b1b2f368df8cba569b2

[Alex Murray (alexmurray)]

LGTM except one minor change needed to the comment in base_container.py

review: Needs Fixing

diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py
index 3c4555f..0759469 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -90,6 +90,9 @@ class BaseContainer:
         # Check max/min size if defined
         self.check_container_size()
 
+        # Check container has the expected format
+        self.check_container_format()
+
     @property
     def unpack_dir(self) -> str:
         """
@@ -143,7 +146,9 @@ class BaseContainer:
                 format is specific to the container type. This method needs to be
                 implemented by the specific subclasses.
         """
-        raise NotImplementedError("Must be implemented by subclasses")
+        raise NotImplementedError(
+            "Must be implemented by subclasses"
+        )  # pragma: no cover
 
     def get_container_size(self) -> int:
         """
@@ -188,7 +193,9 @@ class BaseContainer:
                 estimation method is specific to the container type. This method
                 needs to be implemented by the specific subclasses.
         """
-        raise NotImplementedError("Must be implemented by subclasses")
+        raise NotImplementedError(
+            "Must be implemented by subclasses"
+        )  # pragma: no cover
 
     def check_unpacked_size(self):
         """
@@ -243,7 +250,9 @@ class BaseContainer:
                 estimation method is specific to the container type. This method
                 needs to be implemented by the specific subclasses.
         """
-        raise NotImplementedError("Must be implemented by subclasses")
+        raise NotImplementedError(
+            "Must be implemented by subclasses"
+        )  # pragma: no cover
 
     def get_file(self, fn: str) -> bytes:
         """
diff --git a/reviewtools/containers/squashfs_container.py b/reviewtools/containers/squashfs_container.py
index 631fe76..4896bbb 100644
--- a/reviewtools/containers/squashfs_container.py
+++ b/reviewtools/containers/squashfs_container.py
@@ -10,17 +10,6 @@ class SquashfsContainer(BaseContainer):
     BaseContainer to handle functionality that is specific to Squashfs containers.
     """
 
-    def __init__(self, fn):
-        """
-        If SquashfsContainer is initialized with a container file, it will also
-        verify that the provided file is actually a squashfs, in addition to the
-        checks performed by the BaseContainer.
-        """
-        super().__init__(fn)
-
-        # Check is squash file
-        self.check_container_format()
-
     def check_container_format(self):
         """
         Checks that the container file has the expected format.
diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py
index 277be8f..d4e0726 100644
--- a/reviewtools/tests/containers/test_base_container.py
+++ b/reviewtools/tests/containers/test_base_container.py
@@ -1,10 +1,19 @@
 import os
 import shutil
 import unittest
-from tempfile import mkdtemp
+from unittest.mock import patch
+from tempfile import mkdtemp, mkstemp
 from reviewtools.containers.base_container import BaseContainer, ContainerException
 
 
+def _mock_check_container_format(self):
+    return True
+
+
+def _mock_calculate_unpacked_size(self):
+    return 10**3
+
+
 class TestBaseContainer(unittest.TestCase):
     """Tests for reviewtools.containers.base_container.BaseContainer"""
 
@@ -17,11 +26,33 @@ class TestBaseContainer(unittest.TestCase):
         cls.fn = cls._create_container(cls)
         cls.unpack_dir = cls._create_unpackdir(cls)
 
+        # Mock required classes
+        cls.patches = [
+            unittest.mock.patch(
+                "reviewtools.tests.containers.test_base_container.BaseContainer.check_container_format",
+                _mock_check_container_format,
+            ),
+            unittest.mock.patch(
+                "reviewtools.tests.containers.test_base_container.BaseContainer.calculate_unpacked_size",
+                _mock_calculate_unpacked_size,
+            ),
+            unittest.mock.patch(
+                "reviewtools.tests.containers.test_base_container.BaseContainer._unpack_dir",
+                cls.unpack_dir,
+            ),
+        ]
+        for patch in cls.patches:
+            patch.start()
+
     @classmethod
     def tearDownClass(cls):
         if os.path.exists(cls.tmp_dir):
             shutil.rmtree(cls.tmp_dir)
 
+        # Stop mocking
+        for patch in cls.patches:
+            patch.stop()
+
     def _create_container(self, container_size: int = 10000):
         fn = os.path.join(self.tmp_dir, "container.test")
         with open(fn, "w") as f:
@@ -49,112 +80,89 @@ map:
         return unpack_dir
 
     # Test class initialization
-    def test_initialization_container__happy(self):
-        try:
-            BaseContainer(self.fn)
-        except ContainerException:
-            self.fail(
-                "An unexpected error occurred during BaseContainer initialization with container file"
-            )
+    def test_initialization__happy(self):
+        BaseContainer(self.fn)
+
+    def test_initialization__missing_extension(self):
+        fd, plain_file = mkstemp()
+        with self.assertRaises(ContainerException) as context:
+            BaseContainer(plain_file)
+        self.assertEqual(
+            "container filename must include an extension", context.exception.value
+        )
+        os.unlink(plain_file)
 
     def test_initialization__missing_file(self):
         with self.assertRaises(ContainerException) as context:
             BaseContainer("/non/existing/path")
-            self.assertTrue(
-                "does not exists or has incorrect permissions"
-                in context.exception.value
-            )
+        self.assertTrue("Could not find " in context.exception.value)
 
     # Test deletion
     def test_check_cleanup(self):
-        try:
-            container = BaseContainer(self.fn)
-            tmp_dir = container.tmp_dir
-            del container
-            self.assertFalse(os.path.exists(tmp_dir))
-        except Exception:
-            self.fail("An unexpected error occurred during deletion test")
+        container = BaseContainer(self.fn)
+        tmp_dir = container.tmp_dir
+        del container
+        self.assertFalse(os.path.exists(tmp_dir))
 
     # Test check container size
     def test_check_container_size__happy(self):
         container = BaseContainer(self.fn)
         container._min_packed_size = 0
         container._max_packed_size = 10**6
-        try:
-            container.check_container_size()
-        except ContainerException:
-            self.fail("An unexpected error occurred while checking file size")
+        container.check_container_size()
 
     def test_check_container_size__too_big(self):
         container = BaseContainer(self.fn)
         container._max_packed_size = 1
         with self.assertRaises(ContainerException) as context:
             container.check_container_size()
-            self.assertTrue("container file is too large " in context.exception.value)
+        self.assertTrue("container file is too large " in context.exception.value)
 
     def test_check_container_size__too_small(self):
         container = BaseContainer(self.fn)
         container._min_packed_size = 10**6
         with self.assertRaises(ContainerException) as context:
             container.check_container_size()
-            self.assertTrue("container file is too small " in context.exception.value)
+        self.assertTrue("container file is too small " in context.exception.value)
 
     # Test check unpacked size
-    def _mock_calculate_unpacked_size(self):
-        return 10**3
-
     def test_check_unpacked_size__happy(self):
         container = BaseContainer(self.fn)
-        container.calculate_unpacked_size = self._mock_calculate_unpacked_size
         container._min_unpacked_size = 0
         container._max_unpacked_size = 10**6
-        try:
-            container.check_unpacked_size()
-        except ContainerException:
-            self.fail("An unexpected error occurred while checking file size")
+        container.check_unpacked_size()
 
     def test_check_unpacked_size__too_big(self):
         container = BaseContainer(self.fn)
-        container.calculate_unpacked_size = self._mock_calculate_unpacked_size
         container._max_unpacked_size = 1
         with self.assertRaises(ContainerException) as context:
             container.check_unpacked_size()
-            self.assertTrue("container file is too large " in context.exception.value)
+        self.assertTrue("unpacked directory is too large " in context.exception.value)
 
     def test_check_unpacked_size__too_small(self):
         container = BaseContainer(self.fn)
-        container.calculate_unpacked_size = self._mock_calculate_unpacked_size
         container._min_unpacked_size = 10**6
         with self.assertRaises(ContainerException) as context:
             container.check_unpacked_size()
-            self.assertTrue("container file is too small " in context.exception.value)
+        self.assertTrue("unpacked directory is too small " in context.exception.value)
 
     # Test get file
     def test_get_file__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            snap_yaml = container.get_file("meta/icon.png")
-            self.assertEqual(snap_yaml.decode(), "Test")
-        except ContainerException:
-            self.fail("An unexpected error occurred while getting meta/icon.png")
+        container = BaseContainer(self.fn)
+        snap_yaml = container.get_file("meta/icon.png")
+        self.assertEqual(snap_yaml.decode(), "Test")
 
     def test_get_file__missing_file(self):
+        container = BaseContainer(self.fn)
         with self.assertRaises(ContainerException) as context:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
             container.get_file("non/existent/file")
-            self.assertTrue("Could not find" in context.exception.value)
+        self.assertTrue("Could not find" in context.exception.value)
 
     # Test get yaml
     def test_get_file_as_yaml__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            snap_yaml = container.get_file_as_yaml("meta/snap.yaml")
-            self.assertEqual(snap_yaml, {"map": {"key": "value"}})
-        except ContainerException:
-            self.fail("An unexpected error occurred while getting meta/snap.yaml")
+        container = BaseContainer(self.fn)
+        snap_yaml = container.get_file_as_yaml("meta/snap.yaml")
+        self.assertEqual(snap_yaml, {"map": {"key": "value"}})
 
     def test_get_file_as_yaml__dup_keys(self):
         # Write the file to test
@@ -169,7 +177,6 @@ map:
 
         with self.assertRaises(ContainerException) as context:
             container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
             container.get_file_as_yaml("test.yaml")
             self.assertTrue('found duplicate key "key"' in context.exception.value)
 
@@ -188,7 +195,6 @@ map:
             )
 
         container = BaseContainer(self.fn)
-        container._unpack_dir = self.unpack_dir
         container.get_file_as_yaml("test.yaml", allow_duplicate_keys=True)
 
         # Clean the tested file
@@ -204,7 +210,6 @@ mode: 0777
             )
 
         container = BaseContainer(self.fn)
-        container._unpack_dir = self.unpack_dir
         test_yaml = container.get_file_as_yaml("test.yaml", yaml_version="1.1")
         self.assertEqual(test_yaml["mode"], 511)
 
@@ -221,7 +226,6 @@ mode: 0777
             )
 
         container = BaseContainer(self.fn)
-        container._unpack_dir = self.unpack_dir
         test_yaml = container.get_file_as_yaml("test.yaml")
         self.assertEqual(test_yaml["mode"], 777)
 
@@ -230,76 +234,49 @@ mode: 0777
 
     # Test get files list
     def test_get_files_list__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            self.assertCountEqual(
-                container.get_files_list(),
-                self.expected_files,
-            )
-        except ContainerException:
-            self.fail("An unexpected error occurred while getting files list")
+        container = BaseContainer(self.fn)
+        self.assertCountEqual(
+            container.get_files_list(),
+            self.expected_files,
+        )
 
     def test_get_files_list_abs__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            self.assertCountEqual(
-                container.get_files_list(abs=True),
-                [
-                    os.path.join(container.unpack_dir, file)
-                    for file in self.expected_files
-                ],
-            )
-        except ContainerException:
-            self.fail("An unexpected error occurred while getting files list")
+        container = BaseContainer(self.fn)
+        self.assertCountEqual(
+            container.get_files_list(abs=True),
+            [os.path.join(container.unpack_dir, file) for file in self.expected_files],
+        )
 
     def test_files__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            self.assertCountEqual(
-                container.files,
-                self.expected_files,
-            )
-        except ContainerException:
-            self.fail("An unexpected error occurred while getting files list")
+        container = BaseContainer(self.fn)
+        self.assertCountEqual(
+            container.files,
+            self.expected_files,
+        )
 
     # Test get binaries list
     def test_get_compiled_binaries_list__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            self.assertCountEqual(
-                container.get_compiled_binaries_list(), self.expected_binaries
-            )
-        except ContainerException:
-            self.fail(
-                "An unexpected error occurred while getting compiled binaries list"
-            )
+        container = BaseContainer(self.fn)
+        self.assertCountEqual(
+            container.get_compiled_binaries_list(), self.expected_binaries
+        )
 
     def test_get_compiled_binaries_list_abs__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            self.assertCountEqual(
-                container.get_compiled_binaries_list(abs=True),
-                [
-                    os.path.join(container.unpack_dir, file)
-                    for file in self.expected_binaries
-                ],
-            )
-        except ContainerException:
-            self.fail(
-                "An unexpected error occurred while getting compiled binaries list"
-            )
+        container = BaseContainer(self.fn)
+        self.assertCountEqual(
+            container.get_compiled_binaries_list(abs=True),
+            [
+                os.path.join(container.unpack_dir, file)
+                for file in self.expected_binaries
+            ],
+        )
 
     def test_bin_files__happy(self):
-        try:
-            container = BaseContainer(self.fn)
-            container._unpack_dir = self.unpack_dir
-            self.assertCountEqual(container.bin_files, self.expected_binaries)
-        except ContainerException:
-            self.fail(
-                "An unexpected error occurred while getting compiled binaries list"
-            )
+        container = BaseContainer(self.fn)
+        self.assertCountEqual(container.bin_files, self.expected_binaries)
+
+
+class TestContainerException(unittest.TestCase):
+    def test_exception__happy(self):
+        e = ContainerException("Test exception")
+        self.assertEqual(str(e), "Test exception")
diff --git a/reviewtools/tests/containers/test_squashfs_container.py b/reviewtools/tests/containers/test_squashfs_container.py
index 7d19a77..3241f23 100644
--- a/reviewtools/tests/containers/test_squashfs_container.py
+++ b/reviewtools/tests/containers/test_squashfs_container.py
@@ -22,52 +22,38 @@ class TestSquashfsContainer(unittest.TestCase):
 
     #  Test initialization
     def test_check_initialization__happy(self):
-        try:
-            SquashfsContainer(self.fn)
-        except Exception:
-            self.fail(
-                "An unexpected error occurred during SquashfsContainer initialization with container file"
-            )
+        SquashfsContainer(self.fn)
 
     def test_check_initialization__invalid_format(self):
+        fd, plain_file = mkstemp(suffix=".snap")
         with self.assertRaises(ContainerException) as context:
-            fd, plain_file = mkstemp()
             SquashfsContainer(plain_file)
-            self.assertEqual(
-                "unsupported package format (not squashfs)", context.exception.value
-            )
-            os.unlink(plain_file)
+        self.assertEqual(
+            "Unsupported package format (not squashfs)", context.exception.value
+        )
+        os.unlink(plain_file)
 
     # Test check format
     def test_calculate_unpacked_size__happy(self):
-        try:
-            # unpacked size calculated is a bit smaller than actual size as it does not consider that
-            # folders require at least one complete block
-            container = SquashfsContainer(self.fn)
-            size = container.calculate_unpacked_size()
-            self.assertTrue(isinstance(size, int))
-        except Exception:
-            self.fail("An unexpected error occurred during unpacked size calculation")
+        # unpacked size calculated is a bit smaller than actual size as it does not consider that
+        # folders require at least one complete block
+        container = SquashfsContainer(self.fn)
+        size = container.calculate_unpacked_size()
+        self.assertTrue(isinstance(size, int))
 
     # Test unpack
     def test_unpack__happy(self):
-        try:
-            container = SquashfsContainer(self.fn)
-            container.unpack()
-        except Exception:
-            self.fail("An unexpected error occurred during unpack")
+        container = SquashfsContainer(self.fn)
+        container.unpack()
 
     def test_unpack__force(self):
-        try:
-            container = SquashfsContainer(self.fn)
-            container.unpack()
-            container.unpack(force=True)
-        except Exception:
-            self.fail("An unexpected error occurred during unpack")
+        container = SquashfsContainer(self.fn)
+        container.unpack()
+        container.unpack(force=True)
 
     def test_unpack__no_force(self):
+        container = SquashfsContainer(self.fn)
+        container.unpack()
         with self.assertRaises(ContainerException) as context:
-            container = SquashfsContainer(self.fn)
-            container.unpack()
             container.unpack()
-            self.assertTrue(" exists. Aborting." in context.exception.value)
+        self.assertTrue(" exists. Aborting." in context.exception.value)