Repository: google/mobly

Issue:
Make `AndroidDevice.take_bug_report` more useful
Right now, `AndroidDevice.take_bug_report` is meant to be used in functions like `BaseTestClass.on_fail`, because it requires a test name and a test begin time.

There's no reason to lock the usage of `AndroidDevice.take_bug_report` like this. We should generalize the usage to make it work in more situations.

Discussion:

Patch:
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index 20204be..f55273d 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -55,6 +55,9 @@ KEY_SKIP_LOGCAT = 'skip_logcat'
 DEFAULT_VALUE_SKIP_LOGCAT = False
 SERVICE_NAME_LOGCAT = 'logcat'
 
+# Default name for bug reports taken without a specified test name.
+DEFAULT_BUG_REPORT_NAME = 'bugreport'
+
 # Default Timeout to wait for boot completion
 DEFAULT_TIMEOUT_BOOT_COMPLETION_SECOND = 15 * 60
 
@@ -392,7 +395,9 @@ def take_bug_reports(ads, test_name, begin_time, destination=None):
 
     def take_br(test_name, begin_time, ad, destination):
         ad.take_bug_report(
-            test_name, begin_time=begin_time, destination=destination)
+            test_name=test_name,
+            begin_time=begin_time,
+            destination=destination)
 
     args = [(test_name, begin_time, ad, destination) for ad in ads]
     utils.concurrent_exec(take_br, args)
@@ -824,7 +829,7 @@ class AndroidDevice(object):
         self.services.snippets.remove_snippet_client(name)
 
     def take_bug_report(self,
-                        test_name,
+                        test_name=None,
                         begin_time=None,
                         timeout=300,
                         destination=None):
@@ -832,6 +837,8 @@ class AndroidDevice(object):
 
         Args:
             test_name: Name of the test method that triggered this bug report.
+                If not set, then this will default to
+                android_device.DEFAULT_BUG_REPORT_NAME.
             begin_time: Timestamp of when the test started. If not set, then
                 this will default to the current time.
             timeout: float, the number of seconds to wait for bugreport to
@@ -843,6 +850,8 @@ class AndroidDevice(object):
           A string containing the absolute path to the bug report on the host
           machine.
         """
+        if test_name is None:
+            test_name = DEFAULT_BUG_REPORT_NAME
         if begin_time is None:
             epoch_time = utils.get_current_epoch_time()
             timestamp = mobly_logger.epoch_to_log_line_timestamp(epoch_time)


Test patch:
diff --git a/mobly/base_test.py b/mobly/base_test.py
index 91a0a4f..3a7f19f 100644
--- a/mobly/base_test.py
+++ b/mobly/base_test.py
@@ -169,8 +169,9 @@ class BaseTestClass(object):
             if name in self.user_params:
                 setattr(self, name, self.user_params[name])
             else:
-                logging.warning('Missing optional user param "%s" in '
-                                'configuration, continue.', name)
+                logging.warning(
+                    'Missing optional user param "%s" in '
+                    'configuration, continue.', name)
 
     def register_controller(self, module, required=True, min_number=1):
         """Loads a controller module and returns its loaded devices.
@@ -403,12 +404,12 @@ class BaseTestClass(object):
         if parent_token == stage_name:
             parent_token = self.TAG
         logging.debug(
-            TEST_STAGE_BEGIN_LOG_TEMPLATE.format(
-                parent_token=parent_token, child_token=stage_name))
+            TEST_STAGE_BEGIN_LOG_TEMPLATE.format(parent_token=parent_token,
+                                                 child_token=stage_name))
         yield
         logging.debug(
-            TEST_STAGE_END_LOG_TEMPLATE.format(
-                parent_token=parent_token, child_token=stage_name))
+            TEST_STAGE_END_LOG_TEMPLATE.format(parent_token=parent_token,
+                                               child_token=stage_name))
 
     def _setup_test(self, test_name):
         """Proxy function to guarantee the base implementation of setup_test is
@@ -588,10 +589,10 @@ class BaseTestClass(object):
                 try:
                     self._setup_test(test_name)
                 except signals.TestFailure as e:
-                    raise_with_traceback(
-                        signals.TestError(e.details, e.extras))
+                    raise_with_traceback(signals.TestError(
+                        e.details, e.extras))
                 test_method()
-            except signals.TestPass:
+            except (signals.TestPass, signals.TestAbortSignal):
                 raise
             except Exception:
                 logging.exception('Exception occurred in %s.',
@@ -620,7 +621,7 @@ class BaseTestClass(object):
         except signals.TestAbortSignal as e:
             # Abort signals, pass along.
             tr_record.test_fail(e)
-            raise e
+            raise
         except signals.TestPass as e:
             # Explicit test pass.
             tr_record.test_pass(e)
@@ -772,8 +773,8 @@ class BaseTestClass(object):
             elif test_name in self._generated_test_table:
                 test_method = self._generated_test_table[test_name]
             else:
-                raise Error('%s does not have test method %s.' % (self.TAG,
-                                                                  test_name))
+                raise Error('%s does not have test method %s.' %
+                            (self.TAG, test_name))
             test_methods.append((test_name, test_method))
         return test_methods
 
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index 53975aa..22944aa 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -224,11 +224,17 @@ class AndroidDeviceTest(unittest.TestCase):
         ads = mock_android_device.get_mock_ads(3)
         android_device.take_bug_reports(ads, 'test_something', 'sometime')
         ads[0].take_bug_report.assert_called_once_with(
-            'test_something', begin_time='sometime', destination=None)
+            test_name='test_something',
+            begin_time='sometime',
+            destination=None)
         ads[1].take_bug_report.assert_called_once_with(
-            'test_something', begin_time='sometime', destination=None)
+            test_name='test_something',
+            begin_time='sometime',
+            destination=None)
         ads[2].take_bug_report.assert_called_once_with(
-            'test_something', begin_time='sometime', destination=None)
+            test_name='test_something',
+            begin_time='sometime',
+            destination=None)
 
     # Tests for android_device.AndroidDevice class.
     # These tests mock out any interaction with the OS and real android device
@@ -320,7 +326,7 @@ class AndroidDeviceTest(unittest.TestCase):
         mock_serial = '1'
         ad = android_device.AndroidDevice(serial=mock_serial)
         output_path = ad.take_bug_report(
-            'test_something', begin_time='sometime')
+            test_name='test_something', begin_time='sometime')
         expected_path = os.path.join(
             logging.log_path, 'AndroidDevice%s' % ad.serial, 'BugReports')
         create_dir_mock.assert_called_with(expected_path)
@@ -344,7 +350,8 @@ class AndroidDeviceTest(unittest.TestCase):
         ad = android_device.AndroidDevice(serial=mock_serial)
         expected_msg = '.* Failed to take bugreport.'
         with self.assertRaisesRegex(android_device.Error, expected_msg):
-            ad.take_bug_report('test_something', begin_time='sometime')
+            ad.take_bug_report(
+                test_name='test_something', begin_time='sometime')
 
     @mock.patch(
         'mobly.controllers.android_device_lib.adb.AdbProxy',
@@ -355,7 +362,7 @@ class AndroidDeviceTest(unittest.TestCase):
     @mock.patch('mobly.utils.create_dir')
     @mock.patch('mobly.utils.get_current_epoch_time')
     @mock.patch('mobly.logger.epoch_to_log_line_timestamp')
-    def test_AndroidDevice_take_bug_report_without_begin_time(
+    def test_AndroidDevice_take_bug_report_without_args(
             self, epoch_to_log_line_timestamp_mock,
             get_current_epoch_time_mock, create_dir_mock, FastbootProxy,
             MockAdbProxy):
@@ -363,7 +370,33 @@ class AndroidDeviceTest(unittest.TestCase):
         epoch_to_log_line_timestamp_mock.return_value = '05-09 17:03:49.606'
         mock_serial = '1'
         ad = android_device.AndroidDevice(serial=mock_serial)
-        output_path = ad.take_bug_report('test_something')
+        output_path = ad.take_bug_report()
+        expected_path = os.path.join(
+            logging.log_path, 'AndroidDevice%s' % ad.serial, 'BugReports')
+        create_dir_mock.assert_called_with(expected_path)
+        epoch_to_log_line_timestamp_mock.assert_called_once_with(1557446629606)
+        self.assertEqual(output_path,
+                         os.path.join(expected_path,
+                                      'bugreport,05-09_17-03-49.606,1.zip'))
+
+    @mock.patch(
+        'mobly.controllers.android_device_lib.adb.AdbProxy',
+        return_value=mock_android_device.MockAdbProxy('1'))
+    @mock.patch(
+        'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+        return_value=mock_android_device.MockFastbootProxy('1'))
+    @mock.patch('mobly.utils.create_dir')
+    @mock.patch('mobly.utils.get_current_epoch_time')
+    @mock.patch('mobly.logger.epoch_to_log_line_timestamp')
+    def test_AndroidDevice_take_bug_report_with_only_test_name(
+            self, epoch_to_log_line_timestamp_mock,
+            get_current_epoch_time_mock, create_dir_mock, FastbootProxy,
+            MockAdbProxy):
+        get_current_epoch_time_mock.return_value = 1557446629606
+        epoch_to_log_line_timestamp_mock.return_value = '05-09 17:03:49.606'
+        mock_serial = '1'
+        ad = android_device.AndroidDevice(serial=mock_serial)
+        output_path = ad.take_bug_report(test_name='test_something')
         expected_path = os.path.join(
             logging.log_path, 'AndroidDevice%s' % ad.serial, 'BugReports')
         create_dir_mock.assert_called_with(expected_path)
@@ -380,7 +413,26 @@ class AndroidDeviceTest(unittest.TestCase):
         'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
         return_value=mock_android_device.MockFastbootProxy(1))
     @mock.patch('mobly.utils.create_dir')
-    def test_AndroidDevice_take_bug_report_with_positional_begin_time(
+    def test_AndroidDevice_take_bug_report_with_only_begin_time(
+            self, create_dir_mock, FastbootProxy, MockAdbProxy):
+        mock_serial = '1'
+        ad = android_device.AndroidDevice(serial=mock_serial)
+        output_path = ad.take_bug_report(begin_time='sometime')
+        expected_path = os.path.join(
+            logging.log_path, 'AndroidDevice%s' % ad.serial, 'BugReports')
+        create_dir_mock.assert_called_with(expected_path)
+        self.assertEqual(output_path,
+                         os.path.join(expected_path,
+                                      'bugreport,sometime,1.zip'))
+
+    @mock.patch(
+        'mobly.controllers.android_device_lib.adb.AdbProxy',
+        return_value=mock_android_device.MockAdbProxy(1))
+    @mock.patch(
+        'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
+        return_value=mock_android_device.MockFastbootProxy(1))
+    @mock.patch('mobly.utils.create_dir')
+    def test_AndroidDevice_take_bug_report_with_positional_args(
             self, create_dir_mock, FastbootProxy, MockAdbProxy):
         mock_serial = '1'
         ad = android_device.AndroidDevice(serial=mock_serial)
@@ -405,7 +457,9 @@ class AndroidDeviceTest(unittest.TestCase):
         ad = android_device.AndroidDevice(serial=mock_serial)
         dest = tempfile.gettempdir()
         output_path = ad.take_bug_report(
-            "test_something", begin_time="sometime", destination=dest)
+            test_name="test_something",
+            begin_time="sometime",
+            destination=dest)
         expected_path = os.path.join(dest)
         create_dir_mock.assert_called_with(expected_path)
         self.assertEqual(output_path,
@@ -428,7 +482,7 @@ class AndroidDeviceTest(unittest.TestCase):
         mock_serial = '1'
         ad = android_device.AndroidDevice(serial=mock_serial)
         output_path = ad.take_bug_report(
-            'test_something', begin_time='sometime')
+            test_name='test_something', begin_time='sometime')
         expected_path = os.path.join(
             logging.log_path, 'AndroidDevice%s' % ad.serial, 'BugReports')
         create_dir_mock.assert_called_with(expected_path)