Task SW-111: Process review

LCU/checkhardware/check_hardware.py

@@ -308,6 +308,14 @@ def stop_test_signal_and_exit(cmd, *optargs):
     :param optargs: The intercepted POSIX signal
     """
     stop_test_signal(cmd)
+    exit_without_triggering_handler(cmd, *optargs)
+
+
+def exit_without_triggering_handler(cmd, *optargs):
+    """
+    :param cmd: The command to stop the test signal
+    :param optargs: The intercepted POSIX signal
+    """
 
     # try to get correct return code
     logger.info('Now exiting.')
@@ -367,7 +375,7 @@ def safely_start_test_signal(start_cmd, stop_cmd):
     try:
         check_call(start_cmd, shell=True)
     except CalledProcessError as ex:
-        logger.error(("Could not start the test signal! Non-zero return code from start_cmd (%s)." % start_cmd), ex)
+        logger.error("Could not start the test signal! Non-zero return code from start_cmd (%s)." % start_cmd, ex)
         raise
 
     # set things up sp signal is stopped when check_hardware terminates
@@ -403,21 +411,20 @@ def safely_start_test_signal_from_ParameterSet(settings):
         logger.info('No test signal settings found.')
 
 
-def wait_for_test_signal_status(status_cmd, status, timeout=30):
+def wait_for_test_signal_status(status_cmd, status, retry_limit=30):
     """
     :param status_cmd: command to get test signal status
     :param status: the command output to wait for
-    :param timeout: raise RunTimeError after this many seconds
+    :param retry_limit: raise RunTimeError after this many status_cmd that did not return status
     """
     logger.info("Waiting for '%s' to return '%s'" % (status_cmd, status))
     out = None
-    for _ in range(timeout):
+    for _ in range(retry_limit):
         out = check_output(status_cmd, shell=True)
         out = out.strip()
-        for line in out.split('\n'):
-            if line.strip() == status:
-                logger.info("Status ok.")
-                return status
+        if out == status:
+            logger.info("Status ok.")
+            return status
         else:
             logger.info('Wrong status: %s != %s. Try again...'% (out, status))
         time.sleep(1)

LCU/checkhardware/checkhardware_lib/__init__.py

@@ -3,7 +3,7 @@
 
 from general import *
 from lofar import *
-from settings import TestSettings
+from settings import TestSettings, ParameterSet
 from db import DB, db_version
 from reporting import make_report
 from spu import SPU

LCU/checkhardware/test/t_check_hardware.py

@@ -25,7 +25,7 @@ with patch.dict('sys.modules', **{
 }):
     # import these here so we can before mock out checks on station environment
     import lofar.lcu.checkhardware.check_hardware as check_hardware
-    from lofar.lcu.checkhardware.checkhardware_lib import TestSettings
+    from lofar.lcu.checkhardware.checkhardware_lib import TestSettings, ParameterSet
     check_hardware.logger = logger   # override logger to handle logging output here
 
 
@@ -39,6 +39,55 @@ class TestCheckHardware(unittest.TestCase):
         # we don't want to actually call anything
         check_hardware.check_call = MagicMock()
 
+        self.elem_settings_no_testsignal = ParameterSet()
+        self.elem_settings_no_testsignal.parset = {'spurious': {'min-peak-pwr': '3.0',
+                                                                'passband': '1:511'},
+                                                   'rf': {'negative-deviation': '-24.0',
+                                                          'subbands': '105',
+                                                          'min-sb-pwr': '65.0',
+                                                          'positive-deviation': '12.0'},
+                                                   'noise': {'negative-deviation': '-3.0',
+                                                             'max-difference': '1.5',
+                                                             'positive-deviation': '1.5',
+                                                             'passband': '1:511'},
+                                                   'oscillation': {'min-peak-pwr': '6.0',
+                                                                   'passband': '1:511'}}
+
+        self.elem_settings_testsignal = ParameterSet()
+        self.elem_settings_testsignal.parset = {'spurious': {'min-peak-pwr': '3.0',
+                                                             'passband': '1:511'},
+                                                'rf': {'negative-deviation': '-24.0',
+                                                       'subbands': '105',
+                                                       'min-sb-pwr': '65.0',
+                                                       'positive-deviation': '12.0'},
+                                                'noise': {'negative-deviation': '-3.0',
+                                                          'max-difference': '1.5',
+                                                          'positive-deviation': '1.5',
+                                                          'passband': '1:511'},
+                                                'testsignal': {'start-cmd': 'echo set config 56.0 -10 | nc ncu 8093',
+                                                               'stop-cmd': 'echo bye | nc ncu 8093'},
+                                                'oscillation': {'min-peak-pwr': '6.0',
+                                                                'passband': '1:511'}}
+
+        self.elem_settings_testsignal_with_status = ParameterSet()
+        self.elem_settings_testsignal_with_status.parset = {'spurious': {'min-peak-pwr': '3.0',
+                                                                         'passband': '1:511'},
+                                                            'rf': {'negative-deviation': '-24.0',
+                                                                   'subbands': '105',
+                                                                   'min-sb-pwr': '65.0',
+                                                                   'positive-deviation': '12.0'},
+                                                            'noise': {'negative-deviation': '-3.0',
+                                                                      'max-difference': '1.5',
+                                                                      'positive-deviation': '1.5',
+                                                                      'passband': '1:511'},
+                                                            'testsignal': {'start-cmd': 'echo set config 56.0 -10 | nc ncu 8093',
+                                                                           'stop-cmd': 'echo bye | nc ncu 8093',
+                                                                           'status-cmd': "echo 'Frequency: 56 MHz Power level: -10 dBm RF: ON'",
+                                                                           'ok-status': "Frequency: 56 MHz Power level: -10 dBm RF: ON"},
+                                                            'oscillation': {'min-peak-pwr': '6.0',
+                                                                            'passband': '1:511'}}
+
+
     def test_safely_start_test_signal(self):
         """ Verify that the provided command is executed and handlers are registered correctly"""
 
@@ -60,26 +109,77 @@ class TestCheckHardware(unittest.TestCase):
             check_hardware.register_signal_handlers.assert_called_with(stop_cmd)
             check_hardware.start_watchdog_daemon.assert_called_with(os.getpid(), stop_cmd)
 
-    def test_safely_start_test_signal_from_ParameterSet(self):
-        """ Verify that the commands from ParameterSet are passed on to safely_start_test_signal"""
+    def test_safely_start_test_signal_logs_and_reraises_CalledProcessError(self):
+        """ Verify that the provided command is executed and handlers are registered correctly"""
 
-        start_cmd = 'echo set config 56.0 -10 | nc ncu 8093'
-        stop_cmd = 'echo bye | nc ncu 8093'
+        # test value
+        start_cmd = 'echo "Start the signal!"'
+        stop_cmd = 'echo "Stop the signal!"'
+
+        # setup test
+        with patch.object(check_hardware, 'register_exit_handler'), \
+             patch.object(check_hardware, 'register_signal_handlers'), \
+             patch.object(check_hardware, 'start_watchdog_daemon'), \
+             patch.object(check_hardware, 'check_call', MagicMock(side_effect=subprocess.CalledProcessError('', ''))), \
+             patch.object(check_hardware.logger, 'error'):
+
+            with self.assertRaises(subprocess.CalledProcessError):
+                # trigger action
+                check_hardware.safely_start_test_signal(start_cmd, stop_cmd)
+
+            # assert correct behavior
+            check_hardware.logger.error.assert_called()
+
+    def test_safely_start_test_signal_from_ParameterSet_turns_signal_and_waits_for_status_correctly(self):
+        """ Verify that the commands from ParameterSet are passed on to safely_start_test_signal and wait_for_test_signal_status"""
 
         # test value
-        f = os.environ.get('srcdir') + '/test-check_hardware.conf'
-        settings = TestSettings(filename=f)
-        elem_settings = settings.group('rcumode.5.element')
+        start_cmd = 'echo set config 56.0 -10 | nc ncu 8093'
+        stop_cmd = 'echo bye | nc ncu 8093'
+        expected_status_cmd = "echo 'Frequency: 56 MHz Power level: -10 dBm RF: ON'"
+        expected_ok_status = "Frequency: 56 MHz Power level: -10 dBm RF: ON"
 
         # setup test
-        with patch.object(check_hardware, 'safely_start_test_signal'):
+        with patch.object(check_hardware, 'safely_start_test_signal'), \
+             patch.object(check_hardware, 'wait_for_test_signal_status'):
 
             # trigger action
-            check_hardware.safely_start_test_signal_from_ParameterSet(elem_settings)
+            check_hardware.safely_start_test_signal_from_ParameterSet(self.elem_settings_testsignal_with_status)
 
             # assert correct behavior
             check_hardware.safely_start_test_signal.assert_called_with(start_cmd, stop_cmd)
+            check_hardware.wait_for_test_signal_status.assert_called_with(expected_status_cmd, expected_ok_status)
+
+    def test_safely_start_test_signal_from_ParameterSet_does_nothing_when_no_stationsignal_keys_in_ParameterSet(self):
+        """ Verify that the commands from ParameterSet are passed on to safely_start_test_signal and wait_for_test_signal_status is not called"""
+
+        # setup test
+        with patch.object(check_hardware, 'safely_start_test_signal'), \
+             patch.object(check_hardware, 'wait_for_test_signal_status'):
+
+            # trigger action
+            check_hardware.safely_start_test_signal_from_ParameterSet(self.elem_settings_no_testsignal)
+
+            # assert correct behavior
+            check_hardware.safely_start_test_signal.assert_not_called()
+            check_hardware.wait_for_test_signal_status.assert_not_called()
+
+    def test_safely_start_test_signal_from_ParameterSet_only_starts_signal_when_no_status_keys_in_ParameterSet(self):
+        """ Verify that safely_start_test_signal and wait_for_test_signal_status are not called"""
+
+        # test value
+        start_cmd = 'echo set config 56.0 -10 | nc ncu 8093'
+        stop_cmd = 'echo bye | nc ncu 8093'
 
+        # setup test
+        with patch.object(check_hardware, 'safely_start_test_signal'), \
+             patch.object(check_hardware, 'wait_for_test_signal_status'):
+            # trigger action
+            check_hardware.safely_start_test_signal_from_ParameterSet(self.elem_settings_testsignal)
+
+            # assert correct behavior
+            check_hardware.safely_start_test_signal.assert_called_with(start_cmd, stop_cmd)
+            check_hardware.wait_for_test_signal_status.assert_not_called()
 
     def test_stop_test_signal(self):
         """ Verify that the provided command is executed """
@@ -94,7 +194,20 @@ class TestCheckHardware(unittest.TestCase):
         os._exit.assert_not_called()
         check_hardware.check_call.assert_called_with(cmd, shell=True)   # command is executed
 
-    def test_stop_test_signal_and_exit(self):
+    def test_stop_test_signal_and_exit_defaults_to_code_1(self):
+        """ Verify that the provided command is executed and os._exit is called with correct return code """
+
+        # test value
+        cmd = 'echo "Stop the signal! 2"'
+
+        # trigger action
+        check_hardware.stop_test_signal_and_exit(cmd)
+
+        # assert correct behavior
+        os._exit.assert_called_with(1)                        # exit code correct
+        check_hardware.check_call.assert_called_with(cmd, shell=True)   # command is executed
+
+    def test_stop_test_signal_and_exit_handles_signal_correctly(self):
         """ Verify that the provided command is executed and os._exit is called with correct return code """
 
         # test value
@@ -113,10 +226,11 @@ class TestCheckHardware(unittest.TestCase):
 
         # test value
         status_cmd = 'mockme'
-        responses = ['ne', 'garbage\n ja \n moregarbage', 'ne']
+        responses = ['ne', 'ja\n', 'ne']
         waitfor = 'ja'
 
-        with patch.object(check_hardware, 'check_output', MagicMock(side_effect=responses)):
+        with patch.object(check_hardware, 'check_output', MagicMock(side_effect=responses)),\
+             patch('time.sleep'):
 
             # trigger action
             check_hardware.wait_for_test_signal_status(status_cmd, waitfor)
@@ -125,6 +239,29 @@ class TestCheckHardware(unittest.TestCase):
             check_hardware.check_output.called_with(status_cmd, shell=True)   # command is executed
             self.assertEqual(check_hardware.check_output.call_count, 2)
 
+    def test_wait_for_test_signal_status_raises_RuntimeError_when_retry_limit_reached(self):
+        """ Verify that the provided command is executed and os._exit is called with correct return code """
+
+        # test value
+        limit=15
+        status_cmd = 'mockme'
+        responses = ['ne'] * limit  # only 30 are read
+        responses.append('ja')
+        waitfor = 'ja'
+
+        with patch.object(check_hardware, 'check_output', MagicMock(side_effect=responses)),\
+             patch('time.sleep'):
+
+            # trigger action
+
+            with self.assertRaises(RuntimeError):
+                check_hardware.wait_for_test_signal_status(status_cmd, waitfor, retry_limit=limit)
+
+            # assert correct behavior
+            check_hardware.check_output.called_with(status_cmd, shell=True)   # command is executed
+            self.assertEqual(check_hardware.check_output.call_count, limit)
+
+
     def test_register_signal_handlers_stops_test_signal_on_POSIX_signal(self):
         """ Verify that the provided command is executed and os._exit is called with correct return code """
 
@@ -208,14 +345,9 @@ class TestCheckHardware(unittest.TestCase):
     # FIXME: Move this to t_settings once that exists
     def test_settings_parset_raises_KeyError_when_accessing_missing_key(self):
 
-        # read settings
-        f = os.environ.get('srcdir')+'/test-check_hardware.conf'
-        settings = TestSettings(filename=f)
-        elem_settings = settings.group('rcumode.6.element')
-
         # assert KeyError if setting not there
         with self.assertRaises(KeyError):
-            logger.info(elem_settings.parset['testsignal']['status-cmd'])
+            logger.info(self.elem_settings_no_testsignal.parset['testsignal']['status-cmd'])
 
 
     # FIXME: Move this to t_settings once that exists
@@ -248,8 +380,11 @@ class TestCheckHardware(unittest.TestCase):
         # test values
         expected_start_cmd = "echo set config 56.0 -10 | nc ncu 8093"
         expected_stop_cmd = "echo bye | nc ncu 8093"
+        expected_status_cmd = "echo 'Frequency: 56 MHz Power level: -10 dBm RF: ON'"
+        expected_ok_status = "Frequency: 56 MHz Power level: -10 dBm RF: ON"
 
         # setup tests
+        # todo: mock the ParameterSet instead, once the imports are resolved and this can be done straight-forward
         check_hardware.conf_file = r'test-check_hardware.conf'
         check_hardware.confpath = os.environ.get('srcdir')+'/'
 
@@ -258,6 +393,7 @@ class TestCheckHardware(unittest.TestCase):
         # FIXME: why is all this actually necessary when I only run an element test?
         with patch.object(check_hardware, 'read_station_config', MagicMock(return_value=(1, 1, 1, 1, 1, 1, 1))), \
              patch.object(check_hardware, 'safely_start_test_signal'), \
+             patch.object(check_hardware, 'wait_for_test_signal_status'), \
              patch.object(check_hardware, 'swlevel', MagicMock(return_value=(5, None))), \
              patch.object(check_hardware, 'rspctl'), \
              patch.object(check_hardware, 'RSP'), \
@@ -281,6 +417,7 @@ class TestCheckHardware(unittest.TestCase):
                 check_hardware.main() # Warning: Something acts as a fork bomb when mocks are not setup properly!
 
             check_hardware.safely_start_test_signal.assert_called_with(expected_start_cmd, expected_stop_cmd)
+            check_hardware.wait_for_test_signal_status.assert_called_with(expected_status_cmd, expected_ok_status)