Repository: google/mobly

Issue:
Better timeout message in CallbackHandler
Right now the `CallbackHandler`'s timeout msg only includes the `callback_id`, which is an implementation detail that doesn't make too much sense for the user. We should add the name of the call where the event was supposed to originate from.

Discussion:
dthkao: 



Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 74 at r1](https://reviewable.io:443/reviews/google/mobly/135#-Ke4ob0DXHPzYW-rBks6:-Ke4ob0DXHPzYW-rBks7:b6v54a8) ([raw file](https://github.com/google/mobly/blob/17f126533ea9c8c2e19c8379964a14ea02b6a3a6/mobly/controllers/android_device_lib/callback_handler.py#L74)):*
> ```Python
>                 raise TimeoutError(
>                     'Timeout waiting for event "%s" triggered by %s'
>                     % (event_name, self._method_name))
> ```

Why not keep the callback id in there as well?

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/135)*
<!-- Sent from Reviewable.io -->

xpconanfan: 



Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 74 at r1](https://reviewable.io:443/reviews/google/mobly/135#-Ke4ob0DXHPzYW-rBks6:-Ke4qNtizUsPekAcMHvk:bq9wddn) ([raw file](https://github.com/google/mobly/blob/17f126533ea9c8c2e19c8379964a14ea02b6a3a6/mobly/controllers/android_device_lib/callback_handler.py#L74)):*
<details><summary><i>Previously, dthkao (David T.H. Kao) wrote…</i></summary><blockquote>

Why not keep the callback id in there as well?
</blockquote></details>

It is not really useful for regular users, and may cause confusion...
Since it is an implementation detail, I don't want people to start relying on it for analysis and other things.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/135)*
<!-- Sent from Reviewable.io -->


Patch:
diff --git a/mobly/controllers/android_device_lib/callback_handler.py b/mobly/controllers/android_device_lib/callback_handler.py
index 399e999..93b13e2 100644
--- a/mobly/controllers/android_device_lib/callback_handler.py
+++ b/mobly/controllers/android_device_lib/callback_handler.py
@@ -42,10 +42,11 @@ class CallbackHandler(object):
     }
     """
 
-    def __init__(self, callback_id, event_client, ret_value):
+    def __init__(self, callback_id, event_client, ret_value, method_name):
         self._id = callback_id
         self._event_client = event_client
         self.ret_value = ret_value
+        self._method_name = method_name
 
     def waitAndGet(self, event_name, timeout=None):
         """Blocks until an event of the specified name has been received and
@@ -69,8 +70,8 @@ class CallbackHandler(object):
         except Exception as e:
             if "EventSnippetException: timeout." in str(e):
                 raise TimeoutError(
-                    'Timeout waiting for event "%s" of callback %s'
-                    % (event_name, self._id))
+                    'Timeout waiting for event "%s" triggered by %s (%s).'
+                    % (event_name, self._method_name, self._id))
             raise
         return event
 
diff --git a/mobly/controllers/android_device_lib/jsonrpc_client_base.py b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
index 84cf480..eed3aca 100644
--- a/mobly/controllers/android_device_lib/jsonrpc_client_base.py
+++ b/mobly/controllers/android_device_lib/jsonrpc_client_base.py
@@ -292,7 +292,10 @@ class JsonRpcClientBase(object):
             if self._event_client is None:
                 self._event_client = self._start_event_client()
             return callback_handler.CallbackHandler(
-                result['callback'], self._event_client, result['result'])
+                callback_id=result['callback'],
+                event_client=self._event_client,
+                ret_value=result['result'],
+                method_name=method)
         return result['result']
 
     def _is_app_running(self):
diff --git a/mobly/controllers/android_device_lib/snippet_client.py b/mobly/controllers/android_device_lib/snippet_client.py
index a053303..c3f2ac9 100644
--- a/mobly/controllers/android_device_lib/snippet_client.py
+++ b/mobly/controllers/android_device_lib/snippet_client.py
@@ -64,13 +64,13 @@ class SnippetClient(jsonrpc_client_base.JsonRpcClientBase):
         # Use info here so people know exactly what's happening here, which is
         # helpful since they need to create their own instrumentations and
         # manifest.
-        logging.info('Launching snippet apk with: %s', cmd)
+        logging.debug('Launching snippet apk %s', self.package)
         self._adb.shell(cmd)
 
     def stop_app(self):
         """Overrides superclass."""
         cmd = _STOP_CMD % self.package
-        logging.info('Stopping snippet apk with: %s', cmd)
+        logging.debug('Stopping snippet apk %s', self.package)
         out = self._adb.shell(_STOP_CMD % self.package).decode('utf-8')
         if 'OK (0 tests)' not in out:
             raise Error('Failed to stop existing apk. Unexpected output: %s' %


Test patch:
diff --git a/tests/mobly/controllers/android_device_lib/callback_handler_test.py b/tests/mobly/controllers/android_device_lib/callback_handler_test.py
index ffe3ad7..dbbb692 100755
--- a/tests/mobly/controllers/android_device_lib/callback_handler_test.py
+++ b/tests/mobly/controllers/android_device_lib/callback_handler_test.py
@@ -33,7 +33,7 @@ class CallbackHandlerTest(unittest.TestCase):
         mock_event_client.eventWaitAndGet = mock.Mock(
             side_effect=jsonrpc_client_base.ApiError(java_timeout_msg))
         handler = callback_handler.CallbackHandler(MOCK_CALLBACK_ID,
-                                                   mock_event_client, None)
+                                                   mock_event_client, None, None)
         expected_msg = 'Timeout waiting for event "ha" .*'
         with self.assertRaisesRegexp(callback_handler.TimeoutError,
                                      expected_msg):