Repository: google/mobly

Issue:
Errors in `teardown_class` are not properly recorded
If Mobly encounters an error in the cleanup stage of `teardown_class`, the error would not be recorded anywhere except the cli output, which makes debugging difficult.

This points to a bigger problem: the errors occurred in base test and runner themselves are not clearly handled.

Discussion:

Patch:
diff --git a/mobly/controller_manager.py b/mobly/controller_manager.py
index 75370dd..775ad3c 100644
--- a/mobly/controller_manager.py
+++ b/mobly/controller_manager.py
@@ -17,6 +17,7 @@ import copy
 import logging
 import yaml
 
+from mobly import expects
 from mobly import records
 from mobly import signals
 
@@ -152,10 +153,9 @@ class ControllerManager(object):
         # logging them.
         for name, module in self._controller_modules.items():
             logging.debug('Destroying %s.', name)
-            try:
+            with expects.expect_no_raises(
+                    'Exception occurred destroying %s.' % name):
                 module.destroy(self._controller_objects[name])
-            except:
-                logging.exception('Exception occurred destroying %s.', name)
         self._controller_objects = collections.OrderedDict()
         self._controller_modules = {}
 
@@ -204,8 +204,11 @@ class ControllerManager(object):
         """
         info_records = []
         for controller_module_name in self._controller_objects.keys():
-            record = self._create_controller_info_record(
-                controller_module_name)
-            if record:
-                info_records.append(record)
+            with expects.expect_no_raises(
+                    'Failed to collect controller info from %s' %
+                    controller_module_name):
+                record = self._create_controller_info_record(
+                    controller_module_name)
+                if record:
+                    info_records.append(record)
         return info_records


Test patch:
diff --git a/mobly/base_test.py b/mobly/base_test.py
index 8ea90d3..2add2f2 100644
--- a/mobly/base_test.py
+++ b/mobly/base_test.py
@@ -41,6 +41,7 @@ STAGE_NAME_SETUP_CLASS = 'setup_class'
 STAGE_NAME_SETUP_TEST = 'setup_test'
 STAGE_NAME_TEARDOWN_TEST = 'teardown_test'
 STAGE_NAME_TEARDOWN_CLASS = 'teardown_class'
+STAGE_NAME_CLEAN_UP = 'clean_up'
 
 
 class Error(Exception):
@@ -374,9 +375,7 @@ class BaseTestClass(object):
                 self.summary_writer.dump(record.to_dict(),
                                          records.TestSummaryEntryType.RECORD)
         finally:
-            # Write controller info and summary to summary file.
-            self._record_controller_info()
-            self._controller_manager.unregister_controllers()
+            self._clean_up()
 
     def teardown_class(self):
         """Teardown function that will be called after all the selected tests in
@@ -846,10 +845,36 @@ class BaseTestClass(object):
             logging.info('Summary for test class %s: %s', self.TAG,
                          self.results.summary_str())
 
+    def _clean_up(self):
+        """The final stage of a test class execution."""
+        stage_name = STAGE_NAME_CLEAN_UP
+        record = records.TestResultRecord(stage_name, self.TAG)
+        record.test_begin()
+        self.current_test_info = runtime_test_info.RuntimeTestInfo(
+            stage_name, self.log_path, record)
+        expects.recorder.reset_internal_states(record)
+        with self._log_test_stage(stage_name):
+            # Write controller info and summary to summary file.
+            self._record_controller_info()
+            self._controller_manager.unregister_controllers()
+            if expects.recorder.has_error:
+                record.test_error()
+                record.update_record()
+                self.results.add_class_error(record)
+                self.summary_writer.dump(record.to_dict(),
+                                         records.TestSummaryEntryType.RECORD)
+
     def clean_up(self):
-        """A function that is executed upon completion of all tests selected in
+        """.. deprecated:: 1.8.1
+
+        Use `teardown_class` instead.
+
+        A function that is executed upon completion of all tests selected in
         the test class.
 
         This function should clean up objects initialized in the constructor by
         user.
+
+        Generally this should not be used as nothing should be instantiated
+        from the constructor of a test class.
         """
diff --git a/tests/mobly/base_test_test.py b/tests/mobly/base_test_test.py
index 1183133..8d77017 100755
--- a/tests/mobly/base_test_test.py
+++ b/tests/mobly/base_test_test.py
@@ -2001,6 +2001,53 @@ class BaseTestTest(unittest.TestCase):
             }
         }])
 
+    def test_record_controller_info_fail(self):
+        mock_test_config = self.mock_test_cls_configs.copy()
+        mock_ctrlr_config_name = mock_controller.MOBLY_CONTROLLER_CONFIG_NAME
+        mock_ctrlr_2_config_name = mock_second_controller.MOBLY_CONTROLLER_CONFIG_NAME
+        my_config = [{'serial': 'xxxx', 'magic': 'Magic'}]
+        mock_test_config.controller_configs[mock_ctrlr_config_name] = my_config
+        mock_test_config.controller_configs[
+            mock_ctrlr_2_config_name] = copy.copy(my_config)
+
+        class ControllerInfoTest(base_test.BaseTestClass):
+            """Registers two different controller types and modifies controller
+            info at runtime.
+            """
+
+            def setup_class(self):
+                device = self.register_controller(mock_controller)[0]
+                device.who_am_i = mock.MagicMock()
+                device.who_am_i.side_effect = Exception('Some failure')
+                second_controller = self.register_controller(
+                    mock_second_controller)[0]
+                # This should appear in recorded controller info.
+                second_controller.set_magic('haha')
+
+            def test_func(self):
+                pass
+
+        bt_cls = ControllerInfoTest(mock_test_config)
+        bt_cls.run()
+        info = bt_cls.results.controller_info[0]
+        self.assertEqual(len(bt_cls.results.controller_info), 1)
+        self.assertEqual(info.test_class, 'ControllerInfoTest')
+        self.assertEqual(info.controller_name, 'AnotherMagicDevice')
+        self.assertEqual(info.controller_info, [{
+            'MyOtherMagic': {
+                'magic': 'Magic',
+                'extra_magic': 'haha'
+            }
+        }])
+        record = bt_cls.results.error[0]
+        print(record.to_dict())
+        self.assertEqual(record.test_name, 'clean_up')
+        self.assertIsNotNone(record.begin_time)
+        self.assertIsNotNone(record.end_time)
+        expected_msg = ('Failed to collect controller info from '
+                        'mock_controller: Some failure')
+        self.assertEqual(record.details, expected_msg)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tests/mobly/controller_manager_test.py b/tests/mobly/controller_manager_test.py
index 7c9f1b6..4bfc4ca 100755
--- a/tests/mobly/controller_manager_test.py
+++ b/tests/mobly/controller_manager_test.py
@@ -150,6 +150,16 @@ class ControllerManagerTest(unittest.TestCase):
         self.assertEqual(record.test_class, 'SomeClass')
         self.assertEqual(record.controller_name, 'MagicDevice')
 
+    @mock.patch('tests.lib.mock_controller.get_info')
+    def test_get_controller_info_records_error(self, mock_get_info_func):
+        mock_get_info_func.side_effect = Exception('Record info failed.')
+        mock_ctrlr_config_name = mock_controller.MOBLY_CONTROLLER_CONFIG_NAME
+        controller_configs = {mock_ctrlr_config_name: ['magic1', 'magic2']}
+        c_manager = controller_manager.ControllerManager(
+            'SomeClass', controller_configs)
+        c_manager.register_controller(mock_controller)
+        self.assertFalse(c_manager.get_controller_info_records())
+
     def test_get_controller_info_records(self):
         mock_ctrlr_config_name = mock_controller.MOBLY_CONTROLLER_CONFIG_NAME
         controller_configs = {mock_ctrlr_config_name: ['magic1', 'magic2']}
@@ -189,6 +199,18 @@ class ControllerManagerTest(unittest.TestCase):
         self.assertFalse(c_manager._controller_objects)
         self.assertFalse(c_manager._controller_modules)
 
+    @mock.patch('tests.lib.mock_controller.destroy')
+    def test_unregister_controller_error(self, mock_destroy_func):
+        mock_ctrlr_config_name = mock_controller.MOBLY_CONTROLLER_CONFIG_NAME
+        controller_configs = {mock_ctrlr_config_name: ['magic1', 'magic2']}
+        c_manager = controller_manager.ControllerManager(
+            'SomeClass', controller_configs)
+        c_manager.register_controller(mock_controller)
+        mock_destroy_func.side_effect = Exception('Failed in destroy.')
+        c_manager.unregister_controllers()
+        self.assertFalse(c_manager._controller_objects)
+        self.assertFalse(c_manager._controller_modules)
+
     @mock.patch('tests.lib.mock_controller.destroy')
     def test_unregister_controller_without_registration(
             self, mock_destroy_func):