[syseepromd] Add unit tests; Refactor to allow for greater unit test coverage (sonic-net#156)

- Refactor syseepromd to eliminate infinite loops to allow for increased unit test coverage
- Refactor signal handler and ensure daemon always exits with non-zero exit code so that supervisor will restart it
- Add unit tests

Unit test coverage increases from 0% to 90%

Author: jleveque

sonic-syseepromd/pytest.ini

@@ -0,0 +1,2 @@
+[pytest]
+addopts = --cov=scripts --cov-report html --cov-report term --cov-report xml --junitxml=test-results.xml -vv

sonic-syseepromd/scripts/syseepromd

@@ -8,83 +8,100 @@
     With this daemon, show syseeprom CLI will be able to get data from state DB instead of access hw or cache.
 '''
 
-try:
-    import signal
-    import sys
-    import threading
+import signal
+import sys
+import threading
 
-    from sonic_py_common import daemon_base
-    from swsscommon import swsscommon
-except ImportError as e:
-    raise ImportError(str(e) + " - required module not found")
+from sonic_py_common import daemon_base
+from swsscommon import swsscommon
+
+
+# TODO: Once we no longer support Python 2, we can eliminate this and get the
+# name using the 'name' field (e.g., `signal.SIGINT.name`) starting with Python 3.5
+SIGNALS_TO_NAMES_DICT = dict((getattr(signal, n), n)
+                             for n in dir(signal) if n.startswith('SIG') and '_' not in n)
 
 PLATFORM_SPECIFIC_MODULE_NAME = 'eeprom'
 PLATFORM_SPECIFIC_CLASS_NAME = 'board'
 
 EEPROM_INFO_UPDATE_PERIOD_SECS = 60
 
-POST_EEPROM_SUCCESS = 0
+ERR_NONE = 0
 ERR_PLATFORM_NOT_SUPPORT = 1
 ERR_FAILED_EEPROM = 2
 ERR_FAILED_UPDATE_DB = 3
 ERR_INVALID_PARAMETER = 4
-ERR_EEPROMUTIL_LOAD = 5
+ERR_EEPROM_LOAD = 5
 
 EEPROM_TABLE_NAME = 'EEPROM_INFO'
+
 SYSLOG_IDENTIFIER = 'syseepromd'
 
+exit_code = 0
+
 
 class DaemonSyseeprom(daemon_base.DaemonBase):
-    def __init__(self, log_identifier):
-        super(DaemonSyseeprom, self).__init__(log_identifier)
+    def __init__(self):
+        super(DaemonSyseeprom, self).__init__(SYSLOG_IDENTIFIER)
+
+        # Set minimum logging level to INFO
+        self.set_min_log_priority_info()
 
         self.stop_event = threading.Event()
         self.eeprom = None
+        self.eeprom_tbl = None
 
-        state_db = daemon_base.db_connect("STATE_DB")
-        self.eeprom_tbl = swsscommon.Table(state_db, EEPROM_TABLE_NAME)
-        self.eepromtbl_keys = []
+        # First, try to load the new platform API
+        try:
+            import sonic_platform
+            self.eeprom = sonic_platform.platform.Platform().get_chassis().get_eeprom()
+        except Exception as e:
+            self.log_warning(
+                "Failed to load platform-specific eeprom from sonic_platform package due to {}. Trying deprecated plugin method ...".format(repr(e)))
 
-    def _wrapper_read_eeprom(self):
-        if self.eeprom is not None:
+            # If we didn't successfully load the class from the sonic_platform package, try loading the old plugin
             try:
-                return self.eeprom.read_eeprom()
-            except (NotImplementedError, IOError):
-                pass
+                self.eeprom = self.load_platform_util(PLATFORM_SPECIFIC_MODULE_NAME, PLATFORM_SPECIFIC_CLASS_NAME)
+            except Exception as e:
+                self.log_error("Failed to load platform-specific eeprom from deprecated plugin: {}".format(repr(e)))
 
-        try:
-            return self.eeprom.read_eeprom()
-        except IOError:
-            pass
+        if not self.eeprom:
+            sys.exit(ERR_EEPROM_LOAD)
 
-    def _wrapper_update_eeprom_db(self, eeprom):
-        if self.eeprom is not None:
-            try:
-                return self.eeprom.update_eeprom_db(eeprom)
-            except NotImplementedError:
-                pass
+        # Connect to STATE_DB
+        state_db = daemon_base.db_connect("STATE_DB")
+        self.eeprom_tbl = swsscommon.Table(state_db, EEPROM_TABLE_NAME)
+        self.eepromtbl_keys = []
 
-        return self.eeprom.update_eeprom_db(eeprom)
+        # Post system EEPROM info to state DB once at start-up
+        rc = self.post_eeprom_to_db()
+        if rc != ERR_NONE:
+            self.log_error("Failed to post system EEPROM info to database")
+
+    def __del__(self):
+        # Delete all the information from DB
+        self.clear_db()
 
     def post_eeprom_to_db(self):
-        eeprom = self._wrapper_read_eeprom()
-        if eeprom is None:
-            self.log_error("Failed to read eeprom")
+        eeprom_data = self.eeprom.read_eeprom()
+        if eeprom_data is None:
+            self.log_error("Failed to read EEPROM")
             return ERR_FAILED_EEPROM
 
-        err = self._wrapper_update_eeprom_db(eeprom)
+        err = self.eeprom.update_eeprom_db(eeprom_data)
         if err:
-            self.log_error("Failed to update eeprom info to database")
+            self.log_error("Failed to update EEPROM info in database")
             return ERR_FAILED_UPDATE_DB
 
         self.eepromtbl_keys = self.eeprom_tbl.getKeys()
 
-        return POST_EEPROM_SUCCESS
+        return ERR_NONE
 
     def clear_db(self):
-        keys = self.eeprom_tbl.getKeys()
-        for key in keys:
-            self.eeprom_tbl._del(key)
+        if self.eeprom_tbl:
+            keys = self.eeprom_tbl.getKeys()
+            for key in keys:
+                self.eeprom_tbl._del(key)
 
     def detect_eeprom_table_integrity(self):
         keys = self.eeprom_tbl.getKeys()
@@ -98,75 +115,55 @@ class DaemonSyseeprom(daemon_base.DaemonBase):
 
         return True
 
-    # Signal handler
+    # Override signal handler from DaemonBase
     def signal_handler(self, sig, frame):
-        if sig == signal.SIGHUP:
-            self.log_info("Caught SIGHUP - ignoring...")
-        elif sig == signal.SIGINT:
-            self.log_info("Caught SIGINT - exiting...")
-            self.stop_event.set()
-        elif sig == signal.SIGTERM:
-            self.log_info("Caught SIGTERM - exiting...")
+        FATAL_SIGNALS = [signal.SIGINT, signal.SIGTERM]
+        NONFATAL_SIGNALS = [signal.SIGHUP]
+
+        global exit_code
+
+        if sig in FATAL_SIGNALS:
+            self.log_info("Caught signal '{}' - exiting...".format(SIGNALS_TO_NAMES_DICT[sig]))
+            exit_code = 128 + sig  # Make sure we exit with a non-zero code so that supervisor will try to restart us
             self.stop_event.set()
+        elif sig in NONFATAL_SIGNALS:
+            self.log_info("Caught signal '{}' - ignoring...".format(SIGNALS_TO_NAMES_DICT[sig]))
         else:
-            self.log_warning("Caught unhandled signal '" + sig + "'")
+            self.log_warning("Caught unhandled signal '{}' - ignoring...".format(SIGNALS_TO_NAMES_DICT[sig]))
 
-    # Run daemon
+    # Main daemon logic
     def run(self):
-        self.log_info("Starting up...")
-
-        # First, try to load the new platform API
-        try:
-            import sonic_platform
-            self.chassis = sonic_platform.platform.Platform().get_chassis()
-            self.eeprom = self.chassis.get_eeprom()
-        except Exception as e:
-            self.log_warning("Failed to load data from eeprom using sonic_platform package due to {}, retrying using deprecated plugin method".format(repr(e)))
-
-        # If we didn't successfully load the class from the sonic_platform package, try loading the old plugin
-        if not self.eeprom:
-            try:
-                self.eeprom = self.load_platform_util(PLATFORM_SPECIFIC_MODULE_NAME, PLATFORM_SPECIFIC_CLASS_NAME)
-            except Exception as e:
-                self.log_error("Failed to load platform-specific eeprom implementation: {}".format(repr(e)))
-
-        if not self.eeprom:
-            sys.exit(ERR_EEPROMUTIL_LOAD)
-
-        # Connect to STATE_DB and post syseeprom info to state DB
-        rc = self.post_eeprom_to_db()
-        if rc != POST_EEPROM_SUCCESS:
-            self.log_error("Failed to post eeprom to database")
-
-        # Start main loop
-        self.log_info("Start daemon main loop")
-
-        while not self.stop_event.wait(EEPROM_INFO_UPDATE_PERIOD_SECS):
-            rc = self.detect_eeprom_table_integrity()
-            if not rc:
-                self.log_info("sys eeprom table was changed, need update")
-                self.clear_db()
-                rcs = self.post_eeprom_to_db()
-                if rcs != POST_EEPROM_SUCCESS:
-                    self.log_error("Failed to post eeprom to database")
-                    continue
-
-        self.log_info("Stop daemon main loop")
+        if self.stop_event.wait(EEPROM_INFO_UPDATE_PERIOD_SECS):
+            # We received a fatal signal
+            return False
 
-        # Delete all the information from DB and then exit
-        self.clear_db()
+        rc = self.detect_eeprom_table_integrity()
+        if not rc:
+            self.log_info("System EEPROM table was changed, needs update")
+            self.clear_db()
+            rcs = self.post_eeprom_to_db()
+            if rcs != ERR_NONE:
+                self.log_error("Failed to post EEPROM to database")
 
-        self.log_info("Shutting down...")
+        return True
 
 #
 # Main =========================================================================
 #
 
 
 def main():
-    syseepromd = DaemonSyseeprom(SYSLOG_IDENTIFIER)
-    syseepromd.run()
+    syseepromd = DaemonSyseeprom()
+
+    syseepromd.log_info("Starting up...")
+
+    while syseepromd.run():
+        pass
+
+    syseepromd.log_info("Shutting down...")
+
+    return exit_code
 
 
 if __name__ == '__main__':
-    main()
+    sys.exit(main())

sonic-syseepromd/setup.cfg

@@ -0,0 +1,2 @@
+[aliases]
+test=pytest

sonic-syseepromd/setup.py

@@ -16,6 +16,12 @@
     setup_requires=[
         'wheel'
     ],
+    tests_require=[
+        'mock>=2.0.0; python_version < "3.3"',
+        'pytest',
+        'pytest-cov',
+        'sonic_platform_common'
+    ],
     classifiers=[
         'Development Status :: 4 - Beta',
         'Environment :: No Input/Output (Daemon)',

sonic-syseepromd/tests/__init__.py

@@ -0,0 +1,6 @@
+"""
+    Mock implementation of sonic_platform package for unit testing
+"""
+
+from . import chassis
+from . import platform

sonic-syseepromd/tests/mocked_libs/sonic_platform/__init__.py

@@ -0,0 +1,21 @@
+"""
+    Mock implementation of sonic_platform package for unit testing
+"""
+
+# TODO: Clean this up once we no longer need to support Python 2
+import sys
+if sys.version_info.major == 3:
+    from unittest import mock
+else:
+    import mock
+
+from sonic_platform_base.chassis_base import ChassisBase
+
+
+class Chassis(ChassisBase):
+    def __init__(self):
+        ChassisBase.__init__(self)
+        self.eeprom = mock.MagicMock()
+
+    def get_eeprom(self):
+        return self.eeprom

sonic-syseepromd/tests/mocked_libs/sonic_platform/chassis.py

@@ -0,0 +1,12 @@
+"""
+    Mock implementation of sonic_platform package for unit testing
+"""
+
+from sonic_platform_base.platform_base import PlatformBase
+from sonic_platform.chassis import Chassis
+
+
+class Platform(PlatformBase):
+    def __init__(self):
+        PlatformBase.__init__(self)
+        self._chassis = Chassis()

sonic-syseepromd/tests/mocked_libs/sonic_platform/platform.py

@@ -0,0 +1,5 @@
+'''
+    Mock implementation of swsscommon package for unit testing
+'''
+
+from . import swsscommon

sonic-syseepromd/tests/mocked_libs/swsscommon/__init__.py

@@ -0,0 +1,51 @@
+'''
+    Mock implementation of swsscommon package for unit testing
+'''
+
+STATE_DB = ''
+
+
+class Table:
+    def __init__(self, db, table_name):
+        self.table_name = table_name
+        self.mock_dict = {}
+
+    def _del(self, key):
+        del self.mock_dict[key]
+        pass
+
+    def set(self, key, fvs):
+        self.mock_dict[key] = fvs.fv_dict
+        pass
+
+    def get(self, key):
+        if key in self.mock_dict:
+            return self.mock_dict[key]
+        return None
+
+
+class FieldValuePairs:
+    fv_dict = {}
+
+    def __init__(self, tuple_list):
+        if isinstance(tuple_list, list) and isinstance(tuple_list[0], tuple):
+            self.fv_dict = dict(tuple_list)
+
+    def __setitem__(self, key, kv_tuple):
+        self.fv_dict[kv_tuple[0]] = kv_tuple[1]
+
+    def __getitem__(self, key):
+        return self.fv_dict[key]
+
+    def __eq__(self, other):
+        if not isinstance(other, FieldValuePairs):
+            # don't attempt to compare against unrelated types
+            return NotImplemented
+
+        return self.fv_dict == other.fv_dict
+
+    def __repr__(self):
+        return repr(self.fv_dict)
+
+    def __str__(self):
+        return repr(self.fv_dict)