Merge ~jslarraz/review-tools:add-container-exceptions-and-tests-framework into review-tools:master

[Jorge Sancho Larraz]

Add container exception and tests framework

[Nishit Majithia (0xnishit)]

thanks for adding tests and handling error more standardize
lgtm!!

review: Approve

[Alex Murray (alexmurray)]

Thanks for this - one comment though, normally exceptions have a class or type - so I would expect this to be called 'InvalidContainerExtension' say rather than just 'ContainerException' - or perhaps 'BaseContainerException' - just wondering whether you think the name should be changed?

diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py
index a3f5f4a..a9e0f41 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -3,7 +3,25 @@ import shutil
 import magic
 from tempfile import mkdtemp
 
-from reviewtools.common import MKDTEMP_PREFIX, MKDTEMP_DIR, error, debug, unpack_pkg, recursive_rm, open_file_read
+from reviewtools.common import (
+    MKDTEMP_PREFIX,
+    MKDTEMP_DIR,
+    debug,
+    unpack_pkg,
+    recursive_rm,
+    open_file_read,
+)
+
+
+class ContainerException(Exception):
+    """This class represents exceptions"""
+
+    def __init__(self, value):
+        self.value = value
+
+    def __str__(self):
+        return str(self.value)
+
 
 class BaseContainer:
     """
@@ -31,14 +49,14 @@ class BaseContainer:
         self.tmp_dir = mkdtemp(prefix=MKDTEMP_PREFIX, dir=MKDTEMP_DIR)
         if os.path.isfile(fn):
             if os.path.basename(fn).count(".") == 0:
-                error("container filename must include an extension")
+                raise ContainerException("container filename must include an extension")
             self.filename = os.path.join(
                 self.tmp_dir,
                 os.path.basename(fn),
             )
             shutil.copyfile(fn, self.filename)
         else:
-            error("Could not find '%s'" % fn)
+            raise ContainerException("Could not find '%s'" % fn)
 
         self.unpack_dir = ".".join(self.filename.split(".")[:-1])
         unpack_pkg(fn, dest=self.unpack_dir)
@@ -62,11 +80,11 @@ class BaseContainer:
     def _extract_file(self, fn):
         """Extract file"""
         if not fn.startswith("/"):
-            error("_extract_file() expects absolute path")
+            raise ContainerException("_extract_file() expects absolute path")
         rel = os.path.relpath(fn, self.unpack_dir)
 
         if not os.path.isfile(fn):
-            error("Could not find '%s'" % rel)
+            raise ContainerException("Could not find '%s'" % rel)
         return open_file_read(fn)
 
     def get_files_list(self):
@@ -91,11 +109,10 @@ class BaseContainer:
                 continue
 
             if (
-                    res in self.magic_binary_file_descriptions
-                    and not i.endswith(".mo")  # check is not a message catalog
-                    and i not in bin_files
+                res in self.magic_binary_file_descriptions
+                and not i.endswith(".mo")  # check is not a message catalog
+                and i not in bin_files
             ):
                 bin_files.append(i)
 
         return bin_files
-
diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py
index ca8960f..9bdb556 100644
--- a/reviewtools/sr_common.py
+++ b/reviewtools/sr_common.py
@@ -30,7 +30,7 @@ from reviewtools.common import (
     unsquashfs_lln_parse,
     verify_type,
 )
-from reviewtools.containers.base_container import BaseContainer
+from reviewtools.containers.base_container import BaseContainer, ContainerException
 from reviewtools.overrides import interfaces_attribs_addons
 
 
@@ -417,7 +417,10 @@ class SnapReview(ReviewBase):
             return
         ReviewBase.__init__(self, review_type, overrides=overrides)
 
-        self.snap = BaseContainer(fn)
+        try:
+            self.snap = BaseContainer(fn)
+        except ContainerException as e:
+            error(str(e))
         # TODO: update code to directly use self.snap.xxx instead of assigning here
         self.pkg_filename = self.snap.filename
         self.unpack_dir = self.snap.unpack_dir
diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py
index 40990ad..518e9a2 100644
--- a/reviewtools/sr_lint.py
+++ b/reviewtools/sr_lint.py
@@ -16,9 +16,11 @@
 
 from __future__ import print_function
 from reviewtools.schemas.schema_validator import validate
+from reviewtools.containers.base_container import ContainerException
 from reviewtools.sr_common import SnapReview
 from reviewtools.common import (
     find_external_symlinks,
+    error,
 )
 from reviewtools.overrides import (
     redflagged_snap_types_overrides,
@@ -1979,7 +1981,10 @@ class SnapReviewLint(SnapReview):
             else:
                 appnames.append("%s.%s" % (self.snap_yaml["name"], app))
 
-        fh = self.snap._extract_file(fn)
+        try:
+            fh = self.snap._extract_file(fn)
+        except ContainerException as e:
+            error(str(e))
         lines = fh.readlines()
         fh.close()
 
diff --git a/reviewtools/tests/containers/__init__.py b/reviewtools/tests/containers/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/reviewtools/tests/containers/__init__.py
diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py
new file mode 100644
index 0000000..4d0563f
--- /dev/null
+++ b/reviewtools/tests/containers/test_base_container.py
@@ -0,0 +1,39 @@
+import os
+import shutil
+import unittest
+from tempfile import mkdtemp
+from reviewtools.containers.base_container import BaseContainer, ContainerException
+from reviewtools.tests.utils import make_snap2
+
+
+class TestBaseContainer(unittest.TestCase):
+    """Tests for reviewtools.containers.base_container.BaseContainer"""
+
+    @classmethod
+    def setUpClass(cls):
+        cls.tmp_dir = mkdtemp()
+        cls.fn = make_snap2(
+            output_dir=cls.tmp_dir,
+        )
+
+    @classmethod
+    def tearDownClass(cls):
+        if os.path.exists(cls.tmp_dir):
+            shutil.rmtree(cls.tmp_dir)
+
+    # Test class initialization
+    def test_initialization_container__happy(self):
+        try:
+            BaseContainer(self.fn)
+        except Exception:
+            self.fail(
+                "An unexpected error occurred during BaseContainer initialization with container 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
+            )