Simplify service interface and post-service clean ups. (#515)
* Disallow changing config in `start`.
* Update logcat service to conform to the new interface.
* Give the option to not start service upon registration.
diff --git a/mobly/controllers/android_device.py b/mobly/controllers/android_device.py
index 430ece9..178fd7b 100644
--- a/mobly/controllers/android_device.py
+++ b/mobly/controllers/android_device.py
@@ -54,6 +54,7 @@
# Default is False.
KEY_SKIP_LOGCAT = 'skip_logcat'
DEFAULT_VALUE_SKIP_LOGCAT = False
+SERVICE_NAME_LOGCAT = 'logcat'
# Default Timeout to wait for boot completion
DEFAULT_TIMEOUT_BOOT_COMPLETION_SECOND = 15 * 60
@@ -135,11 +136,11 @@
running_ads = []
for ad in ads:
running_ads.append(ad)
- skip_logcat = getattr(ad, KEY_SKIP_LOGCAT, DEFAULT_VALUE_SKIP_LOGCAT)
- if skip_logcat:
- continue
+ start_logcat = not getattr(ad, KEY_SKIP_LOGCAT,
+ DEFAULT_VALUE_SKIP_LOGCAT)
try:
- ad.start_services()
+ ad.services.register(
+ SERVICE_NAME_LOGCAT, logcat.Logcat, start_service=start_logcat)
except Exception:
is_required = getattr(ad, KEY_DEVICE_REQUIRED,
DEFAULT_VALUE_DEVICE_REQUIRED)
@@ -449,7 +450,7 @@
@property
def adb_logcat_file_path(self):
- if self.services.has_service_by_name('logcat'):
+ if self.services.has_service_by_name(SERVICE_NAME_LOGCAT):
return self.services.logcat.adb_logcat_file_path
@property
@@ -629,8 +630,6 @@
Starts long running services on the android device, like adb logcat
capture.
"""
- configs = logcat.Config(clear_log=clear_log)
- self.services.register('logcat', logcat.Logcat, configs)
self.services.start_all()
def start_adb_logcat(self, clear_log=True):
@@ -638,8 +637,7 @@
Use `self.services.logcat.start` instead.
"""
- configs = logcat.Config(clear_log=clear_log)
- self.services.logcat.start(configs)
+ self.services.logcat.start()
def stop_adb_logcat(self):
""".. deprecated:: 1.8
diff --git a/mobly/controllers/android_device_lib/jsonrpc_shell_base.py b/mobly/controllers/android_device_lib/jsonrpc_shell_base.py
index e596d8c..162813a 100755
--- a/mobly/controllers/android_device_lib/jsonrpc_shell_base.py
+++ b/mobly/controllers/android_device_lib/jsonrpc_shell_base.py
@@ -77,7 +77,7 @@
code.interact(banner=console_banner, local=console_env)
# Tear everything down
- self._ad.stop_services()
+ self._ad.services.stop_all()
def main(self, serial=None):
try:
diff --git a/mobly/controllers/android_device_lib/service_manager.py b/mobly/controllers/android_device_lib/service_manager.py
index bb65be8..0d93ad5 100644
--- a/mobly/controllers/android_device_lib/service_manager.py
+++ b/mobly/controllers/android_device_lib/service_manager.py
@@ -56,7 +56,7 @@
return True
return False
- def register(self, alias, service_class, configs=None):
+ def register(self, alias, service_class, configs=None, start_service=True):
"""Registers a service.
This will create a service instance, starts the service, and adds the
@@ -67,6 +67,8 @@
service_class: class, the service class to instantiate.
configs: (optional) config object to pass to the service class's
constructor.
+ start_service: bool, whether to start the service instance or not.
+ Default is True.
"""
if not inspect.isclass(service_class):
raise Error(self._device, '"%s" is not a class!' % service_class)
@@ -79,7 +81,8 @@
self._device,
'A service is already registered with alias "%s".' % alias)
service_obj = service_class(self._device, configs)
- service_obj.start()
+ if start_service:
+ service_obj.start()
self._service_objects[alias] = service_obj
def unregister(self, alias):
diff --git a/mobly/controllers/android_device_lib/services/base_service.py b/mobly/controllers/android_device_lib/services/base_service.py
index 1806ae2..14380df 100644
--- a/mobly/controllers/android_device_lib/services/base_service.py
+++ b/mobly/controllers/android_device_lib/services/base_service.py
@@ -23,6 +23,10 @@
def __init__(self, device, configs=None):
"""Constructor of the class.
+ The constructor is the only place to pass in a config. If you need to
+ change the config later, you should unregister the service instance
+ from `ServiceManager` and register again with the new config.
+
Args:
device: the device object this service is associated with.
config: optional configuration defined by the author of the service
@@ -36,12 +40,8 @@
"""True if the service is active; False otherwise."""
raise NotImplementedError('"is_alive" is a required service property.')
- def start(self, configs=None):
- """Starts the service.
-
- Args:
- configs: optional configs to be passed for startup.
- """
+ def start(self):
+ """Starts the service."""
raise NotImplementedError('"start" is a required service method.')
def stop(self):
@@ -84,4 +84,4 @@
If not implemented, we assume the service is not sensitive to device
disconnect, and `start` will be called by default.
"""
- self.start(configs=self._configs)
+ self.start()
diff --git a/mobly/controllers/android_device_lib/services/logcat.py b/mobly/controllers/android_device_lib/services/logcat.py
index 9549c75..f1b021f 100644
--- a/mobly/controllers/android_device_lib/services/logcat.py
+++ b/mobly/controllers/android_device_lib/services/logcat.py
@@ -34,11 +34,17 @@
Attributes:
clear_log: bool, clears the logcat before collection if True.
logcat_params: string, extra params to be added to logcat command.
+ output_file_path: string, the path on the host to write the log file
+ to. The service will automatically generate one if not specified.
"""
- def __init__(self, params=None, clear_log=True):
+ def __init__(self,
+ logcat_params=None,
+ clear_log=True,
+ output_file_path=None):
self.clear_log = clear_log
- self.logcat_params = params if params else ''
+ self.logcat_params = logcat_params if logcat_params else ''
+ self.output_file_path = output_file_path
class Logcat(base_service.BaseService):
@@ -87,7 +93,7 @@
return True if self._adb_logcat_process else False
def clear_adb_log(self):
- # Clears cached adb content.
+ """Clears cached adb content."""
try:
self._ad.adb.logcat('-c')
except adb.AdbError as e:
@@ -150,30 +156,40 @@
if in_range:
break
- def start(self, configs=None):
- """Starts a standing adb logcat collection.
+ def _assert_not_running(self):
+ """Asserts the logcat service is not running.
- The collection runs in a separate subprocess and saves logs in a file.
-
- Args:
- configs: Conifg object.
+ Raises:
+ Error, if the logcat service is running.
"""
- if self._adb_logcat_process:
+ if self.is_alive:
raise Error(
self._ad,
'Logcat thread is already running, cannot start another one.')
- configs = configs if configs else self._configs
- if configs.clear_log:
- self.clear_adb_log()
+ def start(self):
+ """Starts a standing adb logcat collection.
+
+ The collection runs in a separate subprocess and saves logs in a file.
+ """
+ self._assert_not_running()
+ if self._configs.clear_log:
+ self.clear_adb_log()
+ self._start()
+
+ def _start(self):
+ """The actual logic of starting logcat."""
self._enable_logpersist()
- f_name = 'adblog,%s,%s.txt' % (self._ad.model,
- self._ad._normalized_serial)
utils.create_dir(self._ad.log_path)
- logcat_file_path = os.path.join(self._ad.log_path, f_name)
+ logcat_file_path = self._configs.output_file_path
+ if not logcat_file_path:
+ f_name = 'adblog,%s,%s.txt' % (self._ad.model,
+ self._ad._normalized_serial)
+ logcat_file_path = os.path.join(self._ad.log_path, f_name)
cmd = '"%s" -s %s logcat -v threadtime %s >> "%s"' % (
- adb.ADB, self._ad.serial, configs.logcat_params, logcat_file_path)
+ adb.ADB, self._ad.serial, self._configs.logcat_params,
+ logcat_file_path)
process = utils.start_standing_subprocess(cmd, shell=True)
self._adb_logcat_process = process
self.adb_logcat_file_path = logcat_file_path
@@ -191,20 +207,15 @@
def pause(self):
"""Pauses logcat for usb disconnect."""
self.stop()
- # Clears cached adb content, so that the next time start_adb_logcat()
- # won't produce duplicated logs to log file.
+ # Clears cached adb content, so that the next time logcat is started,
+ # we won't produce duplicated logs to log file.
# This helps disconnection that caused by, e.g., USB off; at the
# cost of losing logs at disconnection caused by reboot.
self.clear_adb_log()
def resume(self):
- """Resumes a paused logcat service.
-
- Args:
- configs: Not used.
- """
- # Do not clear device log at this time. Otherwise the log during USB
- # disconnection will be lost.
- resume_configs = copy.copy(self._configs)
- resume_configs.clear_log = False
- self.start(resume_configs)
+ """Resumes a paused logcat service."""
+ self._assert_not_running()
+ # Not clearing the log regardless of the config when resuming.
+ # Otherwise the logs during the paused time will be lost.
+ self._start()
diff --git a/tests/mobly/controllers/android_device_lib/services/logcat_test.py b/tests/mobly/controllers/android_device_lib/services/logcat_test.py
index 1871c43..2cf6923 100755
--- a/tests/mobly/controllers/android_device_lib/services/logcat_test.py
+++ b/tests/mobly/controllers/android_device_lib/services/logcat_test.py
@@ -88,9 +88,8 @@
@mock.patch(
'mobly.utils.start_standing_subprocess', return_value='process')
@mock.patch('mobly.utils.stop_standing_subprocess')
- def test_logcat_service_start_and_stop(self, stop_proc_mock,
- start_proc_mock, creat_dir_mock,
- FastbootProxy, MockAdbProxy):
+ def test_start_and_stop(self, stop_proc_mock, start_proc_mock,
+ create_dir_mock, FastbootProxy, MockAdbProxy):
"""Verifies the steps of collecting adb logcat on an AndroidDevice
object, including various function calls and the expected behaviors of
the calls.
@@ -104,7 +103,7 @@
expected_log_path = os.path.join(logging.log_path,
'AndroidDevice%s' % ad.serial,
'adblog,fakemodel,%s.txt' % ad.serial)
- creat_dir_mock.assert_called_with(os.path.dirname(expected_log_path))
+ create_dir_mock.assert_called_with(os.path.dirname(expected_log_path))
adb_cmd = '"adb" -s %s logcat -v threadtime >> %s'
start_proc_mock.assert_called_with(
adb_cmd % (ad.serial, '"%s"' % expected_log_path), shell=True)
@@ -136,12 +135,12 @@
@mock.patch(
'mobly.controllers.android_device_lib.services.logcat.Logcat.clear_adb_log',
return_value=mock_android_device.MockAdbProxy('1'))
- def test_logcat_service_pause_and_resume(
- self, clear_adb_mock, stop_proc_mock, start_proc_mock,
- creat_dir_mock, FastbootProxy, MockAdbProxy):
+ def test_pause_and_resume(self, clear_adb_mock, stop_proc_mock,
+ start_proc_mock, create_dir_mock, FastbootProxy,
+ MockAdbProxy):
mock_serial = '1'
ad = android_device.AndroidDevice(serial=mock_serial)
- logcat_service = logcat.Logcat(ad)
+ logcat_service = logcat.Logcat(ad, logcat.Config(clear_log=True))
logcat_service.start()
clear_adb_mock.assert_called_once_with()
self.assertTrue(logcat_service.is_alive)
@@ -164,9 +163,9 @@
@mock.patch(
'mobly.utils.start_standing_subprocess', return_value='process')
@mock.patch('mobly.utils.stop_standing_subprocess')
- def test_logcat_service_take_logcat_with_extra_params(
- self, stop_proc_mock, start_proc_mock, creat_dir_mock,
- FastbootProxy, MockAdbProxy):
+ def test_take_logcat_with_extra_params(self, stop_proc_mock,
+ start_proc_mock, create_dir_mock,
+ FastbootProxy, MockAdbProxy):
"""Verifies the steps of collecting adb logcat on an AndroidDevice
object, including various function calls and the expected behaviors of
the calls.
@@ -182,7 +181,7 @@
expected_log_path = os.path.join(logging.log_path,
'AndroidDevice%s' % ad.serial,
'adblog,fakemodel,%s.txt' % ad.serial)
- creat_dir_mock.assert_called_with(os.path.dirname(expected_log_path))
+ create_dir_mock.assert_called_with(os.path.dirname(expected_log_path))
adb_cmd = '"adb" -s %s logcat -v threadtime -b radio >> %s'
start_proc_mock.assert_called_with(
adb_cmd % (ad.serial, '"%s"' % expected_log_path), shell=True)
@@ -195,44 +194,7 @@
@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.start_standing_subprocess', return_value='process')
- @mock.patch('mobly.utils.stop_standing_subprocess')
- def test_logcat_service_take_logcat_with_logcat_params_override_in_start(
- self, stop_proc_mock, start_proc_mock, creat_dir_mock,
- FastbootProxy, MockAdbProxy):
- """Verifies the steps of collecting adb logcat on an AndroidDevice
- object, including various function calls and the expected behaviors of
- the calls.
- """
- mock_serial = '1'
- ad = android_device.AndroidDevice(serial=mock_serial)
- configs = logcat.Config()
- configs.logcat_params = '-b radio'
- logcat_service = logcat.Logcat(ad, configs)
- new_configs = logcat.Config()
- new_configs.logcat_params = '-b something_else'
- logcat_service.start(configs=new_configs)
- # Verify start did the correct operations.
- self.assertTrue(logcat_service._adb_logcat_process)
- expected_log_path = os.path.join(logging.log_path,
- 'AndroidDevice%s' % ad.serial,
- 'adblog,fakemodel,%s.txt' % ad.serial)
- creat_dir_mock.assert_called_with(os.path.dirname(expected_log_path))
- adb_cmd = '"adb" -s %s logcat -v threadtime -b something_else >> %s'
- start_proc_mock.assert_called_with(
- adb_cmd % (ad.serial, '"%s"' % expected_log_path), shell=True)
- self.assertEqual(logcat_service.adb_logcat_file_path,
- expected_log_path)
-
- @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'))
- def test_logcat_service_instantiation(self, MockFastboot, MockAdbProxy):
+ def test_instantiation(self, MockFastboot, MockAdbProxy):
"""Verifies the AndroidDevice object's basic attributes are correctly
set after instantiation.
"""
@@ -254,9 +216,8 @@
@mock.patch(
'mobly.logger.get_log_line_timestamp',
return_value=MOCK_ADB_LOGCAT_END_TIME)
- def test_logcat_service_cat_adb_log(self, mock_timestamp_getter,
- stop_proc_mock, start_proc_mock,
- FastbootProxy, MockAdbProxy):
+ def test_cat_adb_log(self, mock_timestamp_getter, stop_proc_mock,
+ start_proc_mock, FastbootProxy, MockAdbProxy):
"""Verifies that AndroidDevice.cat_adb_log loads the correct adb log
file, locates the correct adb log lines within the given time range,
and writes the lines to the correct output file.
@@ -300,9 +261,9 @@
@mock.patch(
'mobly.logger.get_log_line_timestamp',
return_value=MOCK_ADB_LOGCAT_END_TIME)
- def test_logcat_service_cat_adb_log_with_unicode(
- self, mock_timestamp_getter, stop_proc_mock, start_proc_mock,
- FastbootProxy, MockAdbProxy):
+ def test_cat_adb_log_with_unicode(self, mock_timestamp_getter,
+ stop_proc_mock, start_proc_mock,
+ FastbootProxy, MockAdbProxy):
"""Verifies that AndroidDevice.cat_adb_log loads the correct adb log
file, locates the correct adb log lines within the given time range,
and writes the lines to the correct output file.
@@ -341,8 +302,8 @@
@mock.patch(
'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
return_value=mock_android_device.MockFastbootProxy('1'))
- def test_logcat_service__enable_logpersist_with_logpersist(
- self, MockFastboot, MockAdbProxy):
+ def test__enable_logpersist_with_logpersist(self, MockFastboot,
+ MockAdbProxy):
mock_serial = '1'
mock_adb_proxy = MockAdbProxy.return_value
# Set getprop to return '1' to indicate the device is rootable.
@@ -364,7 +325,7 @@
@mock.patch(
'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
return_value=mock_android_device.MockFastbootProxy('1'))
- def test_logcat_service__enable_logpersist_with_missing_all_logpersist(
+ def test__enable_logpersist_with_missing_all_logpersist(
self, MockFastboot, MockAdbProxy):
def adb_shell_helper(command):
if command == 'logpersist.start':
@@ -391,7 +352,7 @@
@mock.patch(
'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
return_value=mock_android_device.MockFastbootProxy('1'))
- def test_logcat_service__enable_logpersist_with_missing_logpersist_stop(
+ def test__enable_logpersist_with_missing_logpersist_stop(
self, MockFastboot, MockAdbProxy):
def adb_shell_helper(command):
if command == 'logpersist.stop --clear':
@@ -414,7 +375,7 @@
'mobly.controllers.android_device_lib.adb.AdbProxy',
return_value=mock.MagicMock())
@mock.patch('mobly.utils.stop_standing_subprocess')
- def test_logcat_service__enable_logpersist_with_missing_logpersist_start(
+ def test__enable_logpersist_with_missing_logpersist_start(
self, MockFastboot, MockAdbProxy):
def adb_shell_helper(command):
if command == 'logpersist.start':
@@ -437,7 +398,7 @@
@mock.patch(
'mobly.controllers.android_device_lib.fastboot.FastbootProxy',
return_value=mock_android_device.MockFastbootProxy('1'))
- def test_logcat_service_clear_adb_log(self, MockFastboot, MockAdbProxy):
+ def test_clear_adb_log(self, MockFastboot, MockAdbProxy):
mock_serial = '1'
ad = android_device.AndroidDevice(serial=mock_serial)
ad.adb.logcat = mock.MagicMock()
diff --git a/tests/mobly/controllers/android_device_test.py b/tests/mobly/controllers/android_device_test.py
index 818d10c..f83c56f 100755
--- a/tests/mobly/controllers/android_device_test.py
+++ b/tests/mobly/controllers/android_device_test.py
@@ -198,11 +198,11 @@
"""
msg = 'Some error happened.'
ads = mock_android_device.get_mock_ads(3)
- ads[0].start_services = mock.MagicMock()
+ ads[0].services.register = mock.MagicMock()
ads[0].stop_services = mock.MagicMock()
- ads[1].start_services = mock.MagicMock()
+ ads[1].services.register = mock.MagicMock()
ads[1].stop_services = mock.MagicMock()
- ads[2].start_services = mock.MagicMock(
+ ads[2].services.register = mock.MagicMock(
side_effect=android_device.Error(msg))
ads[2].stop_services = mock.MagicMock()
with self.assertRaisesRegex(android_device.Error, msg):
@@ -213,9 +213,9 @@
def test_start_services_on_ads_skip_logcat(self):
ads = mock_android_device.get_mock_ads(3)
- ads[0].start_services = mock.MagicMock()
- ads[1].start_services = mock.MagicMock()
- ads[2].start_services = mock.MagicMock(
+ ads[0].services.logcat.start = mock.MagicMock()
+ ads[1].services.logcat.start = mock.MagicMock()
+ ads[2].services.logcat.start = mock.MagicMock(
side_effect=Exception('Should not have called this.'))
ads[2].skip_logcat = True
android_device._start_services_on_ads(ads)
@@ -381,8 +381,6 @@
start_proc_mock, FastbootProxy,
MockAdbProxy):
ad = android_device.AndroidDevice(serial='1')
- ad.start_services()
- ad.services.unregister('logcat')
old_path = ad.log_path
new_log_path = tempfile.mkdtemp()
ad.log_path = new_log_path
@@ -438,7 +436,7 @@
self, stop_proc_mock, start_proc_mock, creat_dir_mock,
FastbootProxy, MockAdbProxy):
ad = android_device.AndroidDevice(serial='1')
- ad.start_services()
+ ad.services.register('logcat', logcat.Logcat)
new_log_path = tempfile.mkdtemp()
expected_msg = '.* Cannot change `log_path` when there is service running.'
with self.assertRaisesRegex(android_device.Error, expected_msg):
@@ -500,7 +498,7 @@
self, stop_proc_mock, start_proc_mock, creat_dir_mock,
FastbootProxy, MockAdbProxy):
ad = android_device.AndroidDevice(serial='1')
- ad.start_services()
+ ad.services.register('logcat', logcat.Logcat)
expected_msg = '.* Cannot change device serial number when there is service running.'
with self.assertRaisesRegex(android_device.Error, expected_msg):
ad.update_serial('2')
