From cc344983041e364483ff7c44e65bb6fa818049f3 Mon Sep 17 00:00:00 2001 From: Ang Li Date: Tue, 6 Oct 2026 06:33:47 +0000 Subject: [PATCH 1/2] Fix miscellaneous minor bugs in android_device_lib and test_runner * `SnippetClientV2._read_protocol_line`: remove the duplicate `self._server_start_stdout = []` reset that wiped output collected by the first protocol-line read. * `AdbProxy.connect`: pass `stderr=b''` (bytes) to `AdbError` so callers like `root()` that call `e.stderr.decode()` don't fail with `AttributeError`. * `SnippetManagementService`: pass `self._device` (not `self`) to `Error` (a `DeviceError` subclass), and drop the unused `self._is_alive` attribute. * `ServiceManager.list_live_services`: use a list comprehension instead of `for_each` so a pure query doesn't go through `expect_no_raises`. * `JsonRpcShellBase`: raise `Error` instead of `assert` (which is stripped under `python -O`). * `TestRunner.get_full_test_names`: fix broken error formatting (`Error('... %s ...', (...))` passed a 2-arg tuple to `Exception` instead of formatting the string). --- mobly/controllers/android_device_lib/adb.py | 2 +- .../android_device_lib/jsonrpc_shell_base.py | 3 ++- .../controllers/android_device_lib/service_manager.py | 10 ++++------ .../services/snippet_management_service.py | 6 ++---- .../android_device_lib/snippet_client_v2.py | 1 - mobly/test_runner.py | 4 +--- 6 files changed, 10 insertions(+), 16 deletions(-) diff --git a/mobly/controllers/android_device_lib/adb.py b/mobly/controllers/android_device_lib/adb.py index 35f59ea65..9e4a06d96 100644 --- a/mobly/controllers/android_device_lib/adb.py +++ b/mobly/controllers/android_device_lib/adb.py @@ -374,7 +374,7 @@ def connect(self, address) -> bytes: ) if PATTERN_ADB_CONNECT_SUCCESS.match(stdout.decode('utf-8')) is None: raise AdbError( - cmd=f'connect {address}', stdout=stdout, stderr='', ret_code=0 + cmd=f'connect {address}', stdout=stdout, stderr=b'', ret_code=0 ) return stdout diff --git a/mobly/controllers/android_device_lib/jsonrpc_shell_base.py b/mobly/controllers/android_device_lib/jsonrpc_shell_base.py index 1a2f304ff..f7454eb5c 100755 --- a/mobly/controllers/android_device_lib/jsonrpc_shell_base.py +++ b/mobly/controllers/android_device_lib/jsonrpc_shell_base.py @@ -66,7 +66,8 @@ def load_device(self, serial=None): if serial not in serials: raise Error('Device "%s" is not found by adb.' % serial) ads = android_device.get_instances([serial]) - assert len(ads) == 1 + if len(ads) != 1: + raise Error(f'Expected exactly one device for "{serial}", got {ads}.') self._ad = ads[0] def start_console(self): diff --git a/mobly/controllers/android_device_lib/service_manager.py b/mobly/controllers/android_device_lib/service_manager.py index 08fbaa13f..c10588231 100644 --- a/mobly/controllers/android_device_lib/service_manager.py +++ b/mobly/controllers/android_device_lib/service_manager.py @@ -150,13 +150,11 @@ def list_live_services(self): Returns: list of strings, the aliases of the services that are running. """ - aliases = [] - self.for_each( - lambda service: aliases.append(service.alias) + return [ + service.alias + for service in self._service_objects.values() if service.is_alive - else None - ) - return aliases + ] def create_output_excerpts_all(self, test_info): """Creates output excerpts from all services. diff --git a/mobly/controllers/android_device_lib/services/snippet_management_service.py b/mobly/controllers/android_device_lib/services/snippet_management_service.py index 2e1a604b8..9c1a9dea4 100644 --- a/mobly/controllers/android_device_lib/services/snippet_management_service.py +++ b/mobly/controllers/android_device_lib/services/snippet_management_service.py @@ -35,7 +35,6 @@ class SnippetManagementService(base_service.BaseService): def __init__(self, device, configs=None): del configs # Unused param. self._device = device - self._is_alive = False self._snippet_clients = {} super().__init__(device) @@ -75,7 +74,7 @@ class for supported configurations. # Should not load snippet with the same name more than once. if name in self._snippet_clients: raise Error( - self, + self._device, f'Name "{name}" is already registered with package' f' "{self._snippet_clients[name].package}" for user ID' f' {self._snippet_clients[name].user_id}, the same name' @@ -89,9 +88,8 @@ class for supported configurations. ) for snippet_name, client in self._snippet_clients.items(): if new_client.identifier == client.identifier: - del new_client raise Error( - self, + self._device, f'Snippet "{client.package}" has already been registered for user' f' id {client.user_id} under name "{snippet_name}". The same' ' package cannot be registered again for the same user.', diff --git a/mobly/controllers/android_device_lib/snippet_client_v2.py b/mobly/controllers/android_device_lib/snippet_client_v2.py index 34fd8ee66..c7257f2d6 100644 --- a/mobly/controllers/android_device_lib/snippet_client_v2.py +++ b/mobly/controllers/android_device_lib/snippet_client_v2.py @@ -406,7 +406,6 @@ def _read_protocol_line(self): errors.ServerStartError: If EOF is reached without any protocol lines being read. """ - self._server_start_stdout = [] while True: line = self._proc.stdout.readline().decode('utf-8') if not line: diff --git a/mobly/test_runner.py b/mobly/test_runner.py index e30bb246c..4b556f239 100644 --- a/mobly/test_runner.py +++ b/mobly/test_runner.py @@ -329,9 +329,7 @@ def get_full_test_names(self): tests_set = set(tests) for test_name in test_run_info.tests: if test_name not in tests_set: - raise Error( - 'Unknown test method: %s in class %s', (test_name, test.TAG) - ) + raise Error(f'Unknown test method: {test_name} in class {test.TAG}') test_names.append(f'{test.TAG}.{test_name}') else: test_names.extend([f'{test.TAG}.{n}' for n in tests]) From 7a0a9cdfe45f2baf6856247dcad18c5d5f718ed2 Mon Sep 17 00:00:00 2001 From: Ang Li Date: Wed, 7 Oct 2026 06:50:26 +0000 Subject: [PATCH 2/2] Add regression tests for each fix --- .../android_device_lib/adb_test.py | 8 +++++++ .../jsonrpc_shell_base_test.py | 14 +++++++++++ .../service_manager_test.py | 13 ++++++++++ .../snippet_management_service_test.py | 13 ++++++++++ .../snippet_client_v2_test.py | 24 +++++++++++++++++++ tests/mobly/test_runner_test.py | 13 ++++++++++ 6 files changed, 85 insertions(+) diff --git a/tests/mobly/controllers/android_device_lib/adb_test.py b/tests/mobly/controllers/android_device_lib/adb_test.py index 31d3ff86f..467fba88f 100755 --- a/tests/mobly/controllers/android_device_lib/adb_test.py +++ b/tests/mobly/controllers/android_device_lib/adb_test.py @@ -556,6 +556,14 @@ def test_connect_fail(self, mock_run_command): ): out = adb.AdbProxy().connect(mock_address) + @mock.patch('mobly.utils.run_command') + def test_connect_fail_error_fields_are_bytes(self, mock_run_command): + mock_run_command.return_value = (0, b'Connection refused\n', b'') + with self.assertRaises(adb.AdbError) as cm: + adb.AdbProxy().connect('localhost:1234') + self.assertIsInstance(cm.exception.stdout, bytes) + self.assertIsInstance(cm.exception.stderr, bytes) + def test_getprop(self): with mock.patch.object(adb.AdbProxy, '_exec_cmd') as mock_exec_cmd: mock_exec_cmd.return_value = b'blah' diff --git a/tests/mobly/controllers/android_device_lib/jsonrpc_shell_base_test.py b/tests/mobly/controllers/android_device_lib/jsonrpc_shell_base_test.py index ffc8f77d2..3588c6e60 100755 --- a/tests/mobly/controllers/android_device_lib/jsonrpc_shell_base_test.py +++ b/tests/mobly/controllers/android_device_lib/jsonrpc_shell_base_test.py @@ -47,6 +47,20 @@ def test_load_device_when_one_device( json_shell.load_device() self.assertEqual(json_shell._ad, mock_device) + @mock.patch.object(android_device, 'list_adb_devices') + @mock.patch.object(android_device, 'get_instances') + @mock.patch.object(os, 'environ', new={}) + def test_load_device_no_instance_raises_error( + self, mock_get_instances, mock_list_adb_devices + ): + mock_list_adb_devices.return_value = ['1234'] + mock_get_instances.return_value = [] + json_shell = jsonrpc_shell_base.JsonRpcShellBase() + with self.assertRaisesRegex( + jsonrpc_shell_base.Error, 'Expected exactly one device for "1234"' + ): + json_shell.load_device(serial='1234') + @mock.patch.object(android_device, 'list_adb_devices') @mock.patch.object(android_device, 'get_instances') @mock.patch.object(os, 'environ', new={'ANDROID_SERIAL': '1234'}) diff --git a/tests/mobly/controllers/android_device_lib/service_manager_test.py b/tests/mobly/controllers/android_device_lib/service_manager_test.py index 2d608d3dd..cee4bf8b5 100755 --- a/tests/mobly/controllers/android_device_lib/service_manager_test.py +++ b/tests/mobly/controllers/android_device_lib/service_manager_test.py @@ -432,6 +432,19 @@ def test_list_live_services(self): aliases = manager.list_live_services() self.assertEqual(aliases, []) + def test_list_live_services_does_not_record_expects_errors(self): + manager = service_manager.ServiceManager(mock.MagicMock()) + manager.register('mock_service1', MockService) + with mock.patch.object( + MockService, + 'is_alive', + new_callable=mock.PropertyMock, + side_effect=RuntimeError('boom'), + ): + with self.assertRaisesRegex(RuntimeError, 'boom'): + manager.list_live_services() + self.assertEqual(expects.recorder.error_count, 0) + def test_start_services(self): manager = service_manager.ServiceManager(mock.MagicMock()) manager.register('mock_service1', MockService, start_service=False) diff --git a/tests/mobly/controllers/android_device_lib/services/snippet_management_service_test.py b/tests/mobly/controllers/android_device_lib/services/snippet_management_service_test.py index f2af73250..cd0bd225f 100755 --- a/tests/mobly/controllers/android_device_lib/services/snippet_management_service_test.py +++ b/tests/mobly/controllers/android_device_lib/services/snippet_management_service_test.py @@ -136,6 +136,19 @@ def test_add_snippet_client_with_config(self, mock_class): package=mock.ANY, ad=mock.ANY, config=snippet_config ) + @mock.patch(SNIPPET_CLIENT_V2_CLASS_PATH) + def test_add_snippet_client_dup_name_error_names_device(self, _): + device = mock.MagicMock() + device.__repr__ = lambda _: '[AndroidDevice|serial123]' + manager = snippet_management_service.SnippetManagementService(device) + manager.add_snippet_client('foo', MOCK_PACKAGE) + with self.assertRaisesRegex( + snippet_management_service.Error, + r'^\[AndroidDevice\|serial123\]::Service Name' + r' "foo" is already registered', + ): + manager.add_snippet_client('foo', MOCK_PACKAGE + 'ha') + @mock.patch(SNIPPET_CLIENT_V2_CLASS_PATH) def test_add_snippet_client_dup_name(self, _): manager = snippet_management_service.SnippetManagementService( diff --git a/tests/mobly/controllers/android_device_lib/snippet_client_v2_test.py b/tests/mobly/controllers/android_device_lib/snippet_client_v2_test.py index 8b5fc4611..971b4ff01 100644 --- a/tests/mobly/controllers/android_device_lib/snippet_client_v2_test.py +++ b/tests/mobly/controllers/android_device_lib/snippet_client_v2_test.py @@ -825,6 +825,30 @@ def test_start_server_error_message_include_discarded_output( ): self.client.start_server() + @mock.patch( + 'mobly.controllers.android_device_lib.snippet_client_v2.' + 'utils.start_standing_subprocess' + ) + def test_start_server_error_message_keeps_output_before_start_line( + self, mock_start_standing_subprocess + ): + """Output discarded before and after SNIPPET START is both reported.""" + self._make_client() + self._mock_server_process_starting_response( + mock_start_standing_subprocess, + resp_lines=[ + b'junk before start\n', + b'SNIPPET START, PROTOCOL 1 0\n', + b'junk after start\n', + b'INSTRUMENTATION_RESULT: shortMsg=Process crashed.', + ], + ) + with self.assertRaisesRegex( + errors.ServerStartProtocolError, + r'junk before start\njunk after start', + ): + self.client.start_server() + @mock.patch( 'mobly.controllers.android_device_lib.snippet_client_v2.' 'utils.start_standing_subprocess' diff --git a/tests/mobly/test_runner_test.py b/tests/mobly/test_runner_test.py index b15314535..952469454 100755 --- a/tests/mobly/test_runner_test.py +++ b/tests/mobly/test_runner_test.py @@ -433,6 +433,19 @@ def test_get_full_test_names_test_list(self): self.assertIn('IntegrationTest.test_hello_world', results) self.assertEqual(len(results), 1) + def test_get_full_test_names_unknown_test(self): + config = self.base_mock_test_config.copy() + tr = test_runner.TestRunner(self.log_dir, self.testbed_name) + with tr.mobly_logger(): + tr.add_test_class( + config, integration_test.IntegrationTest, tests=['test_nope'] + ) + with self.assertRaisesRegex( + test_runner.Error, + '^Unknown test method: test_nope in class IntegrationTest$', + ): + tr.get_full_test_names() + def test_get_full_test_names_test_list_empty(self): """Verifies that calling get_test_names with empty test list works properly.""" config = self.base_mock_test_config.copy()