Repository: google/mobly

Issue:
Create an `event` type on the client side
So instead of dealing with the dict and key access:
```
{u'callbackId': u'2-2', u'data': {u'isSuccess': True}, u'name': u'status', u'time': 1487989424790}
```
We could have some sort of structure:
```
event.data -> {'isSuccess': True}
event.name -> 'status'
event.creation_time -> 1487989424790
```

Discussion:
adorokhine: 



Reviewed 1 of 2 files at r1.
Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 59 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafhOKadEKJiLgP7gH:-KeafhOLrEIcl4RQ0eT3:b-kx858c) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L59)):*
> ```Python
>         self.ret_value = ret_value
> 
>     def _msgToEvent(self, raw_event):
> ```

Our Python style is lowercase_with_underscores.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 63 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafmHMPALFwnwIopJG:-KeafmHMPALFwnwIopJH:b-akwad4) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L63)):*
> ```Python
>             callback_id=raw_event['callbackId'],
>             name=raw_event['name'],
>             creation_tie=raw_event['time'],
> ```

tie? I don't think this would run; can you test this?

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 66 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafukV0V8RVakOJdMM:-KeafukV0V8RVakOJdMN:b-9nmx1w) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L66)):*
> ```Python
>             data=raw_event['data'])
> 
>     def waitAndGet(self, event_name, timeout=None):
> ```

Should be lowercase_with_underscores right? (and elsewhere)

---

*[mobly/controllers/android_device_lib/snippet_event.py, line 1 at r1](https://reviewable.io:443/reviews/google/mobly/134#-Keag8VONgs_comAMvjD:-Keag8VONgs_comAMvjE:b-8q37n6) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/snippet_event.py#L1)):*
> ```Python
> #/usr/bin/env python3.4
> ```

1. Don't quite understand why we are creating new files without the bang. Is it supposed to be there or isn't it?
2. Since this isn't a main class and isn't intended to be executed directly, no bang line is needed.

---

*[mobly/controllers/android_device_lib/snippet_event.py, line 23 at r1](https://reviewable.io:443/reviews/google/mobly/134#-Keag5vDpkhj42nrEfqp:-Keag5vEcTvkA8DeVpq_:bkaa4h2) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/snippet_event.py#L23)):*
> ```Python
> class SnippetEvent(object):
>     """The class that represents callback events for mobly snippet library.
>     
> ```

Whitespace on indented line

---


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

xpconanfan: 



Review status: 0 of 2 files reviewed at latest revision, 5 unresolved discussions.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 63 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafmHMPALFwnwIopJG:-Keaih1W4qlZRt5zcACw:b-896fix) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L63)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

tie? I don't think this would run; can you test this?
</blockquote></details>

Done.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 66 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafukV0V8RVakOJdMM:-KeagrCsfOUTVWNCG-J2:bohdahi) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L66)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Should be lowercase_with_underscores right? (and elsewhere)
</blockquote></details>

Hmm, should we?
This class deals with data from Java side and is closely tied with the Java method names, it seems make sense to use camel case here.

```
callback = ad.spt.doAsyncStuff()
callback.waitAndGet('blahEvent')
```

---

*[mobly/controllers/android_device_lib/snippet_event.py, line 1 at r1](https://reviewable.io:443/reviews/google/mobly/134#-Keag8VONgs_comAMvjD:-Keah2cmy_OYwhjiO1-o:b-896fix) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/snippet_event.py#L1)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

1. Don't quite understand why we are creating new files without the bang. Is it supposed to be there or isn't it?
2. Since this isn't a main class and isn't intended to be executed directly, no bang line is needed.
</blockquote></details>

Done.

---

*[mobly/controllers/android_device_lib/snippet_event.py, line 23 at r1](https://reviewable.io:443/reviews/google/mobly/134#-Keag5vDpkhj42nrEfqp:-Keah30E7XriE80Se1Ju:b-896fix) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/snippet_event.py#L23)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Whitespace on indented line
</blockquote></details>

Done.

---


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

adorokhine: 



Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 63 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafmHMPALFwnwIopJG:-Kealgjf9qegfnk_UPvi:bdakz5s) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L63)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Done.
</blockquote></details>

Did we have a unit test capable of catching this?

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 66 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafukV0V8RVakOJdMM:-Kealn3xeLpy4qz8xwFz:b-6e7p7t) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L66)):*
<details><summary><i>Previously, xpconanfan (Ang Li) wrote…</i></summary><blockquote>

Hmm, should we?
This class deals with data from Java side and is closely tied with the Java method names, it seems make sense to use camel case here.

```
callback = ad.spt.doAsyncStuff()
callback.waitAndGet('blahEvent')
```
</blockquote></details>

For the method names that map directly to the java side like waitAndGet <-> eventWaitAndGet, sg.
For unrelated method names like _msgToEvent we should maintain our python convention.

---


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

xpconanfan: 



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

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 59 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafhOKadEKJiLgP7gH:-Keb0Fl52Y_09mhX2EcR:b-896fix) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L59)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Our Python style is lowercase_with_underscores.
</blockquote></details>

Done.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 63 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafmHMPALFwnwIopJG:-Keb0GMXDLBQkBPF_jDB:b-896fix) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L63)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Did we have a unit test capable of catching this?
</blockquote></details>

Done.

---

*[mobly/controllers/android_device_lib/callback_handler.py, line 66 at r1](https://reviewable.io:443/reviews/google/mobly/134#-KeafukV0V8RVakOJdMM:-Keb0GybKDoO7No6vN6E:b-896fix) ([raw file](https://github.com/google/mobly/blob/ac07cff0770546fe6ffc9087d423d3236400775c/mobly/controllers/android_device_lib/callback_handler.py#L66)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

For the method names that map directly to the java side like waitAndGet <-> eventWaitAndGet, sg.
For unrelated method names like _msgToEvent we should maintain our python convention.
</blockquote></details>

Done.

---


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

adorokhine: 



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

---

*[tests/mobly/controllers/android_device_lib/callback_handler_test.py, line 42 at r3](https://reviewable.io:443/reviews/google/mobly/134#-Keb3TNWhm1veWNWxwy0:-Keb3TNWhm1veWNWxwy1:b-kkxbju) ([raw file](https://github.com/google/mobly/blob/b3d6d78c2855411ed3ec35a3f3e01eba7ebc3291/tests/mobly/controllers/android_device_lib/callback_handler_test.py#L42)):*
> ```Python
>         mock_event_client.eventWaitAndGet = mock.Mock(return_value=MOCK_RAW_EVENT)
>         handler = callback_handler.CallbackHandler(MOCK_CALLBACK_ID,
>                                                    mock_event_client, None, None)
> ```

Can you change this to kwargs so the meaning of the two Nones is clearer?

---


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

xpconanfan: 



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

---

*[tests/mobly/controllers/android_device_lib/callback_handler_test.py, line 42 at r3](https://reviewable.io:443/reviews/google/mobly/134#-Keb3TNWhm1veWNWxwy0:-KebLP7CiCjz4E8MrI22:b-896fix) ([raw file](https://github.com/google/mobly/blob/b3d6d78c2855411ed3ec35a3f3e01eba7ebc3291/tests/mobly/controllers/android_device_lib/callback_handler_test.py#L42)):*
<details><summary><i>Previously, adorokhine (Alexander Dorokhine) wrote…</i></summary><blockquote>

Can you change this to kwargs so the meaning of the two Nones is clearer?
</blockquote></details>

Done.

---


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

adorokhine: <img class="emoji" title=":lgtm:" alt=":lgtm:" align="absmiddle" src="https://reviewable.io/lgtm.png" height="20" width="61"/>

---

Reviewed 2 of 3 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---



*Comments from [Reviewable](https://reviewable.io:443/reviews/google/mobly/134#-:-KebPTqrpHKK-5HOzqt1:bnfp4nl)*
<!-- 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 93b13e2..d86bf9e 100644
--- a/mobly/controllers/android_device_lib/callback_handler.py
+++ b/mobly/controllers/android_device_lib/callback_handler.py
@@ -17,6 +17,8 @@
 import logging
 import time
 
+from mobly.controllers.android_device_lib import snippet_event
+
 
 class Error(Exception):
     pass
@@ -30,16 +32,23 @@ class CallbackHandler(object):
     """The class used to handle a specific group of callback events.
 
     All the events handled by a CallbackHandler are originally triggered by one
-    async Rpc call. All the events are tagged with a callback_id specific to a call
-    to an AsyncRpc method defined on the server side.
+    async Rpc call. All the events are tagged with a callback_id specific to a
+    call to an AsyncRpc method defined on the server side.
 
-    The callback events are dictionaries that follow this schema:
+    The raw message representing an event looks like:
     {
         'callbackId': <string, callbackId>,
         'name': <string, name of the event>,
-        'time': <long, epoch time of when the event was created on the server side>,
+        'time': <long, epoch time of when the event was created on the server
+                 side>,
         'data': <dict, extra data from the callback on the server side>
     }
+
+    Each message is then used to create a SnippetEvent object on the client
+    side.
+
+    Attributes:
+        ret_value: The direct return value of the async Rpc call.
     """
 
     def __init__(self, callback_id, event_client, ret_value, method_name):
@@ -57,7 +66,7 @@ class CallbackHandler(object):
             timeout: float, the number of seconds to wait before giving up.
 
         Returns:
-            The oldest entry of the specified event.
+            SnippetEvent, the oldest entry of the specified event.
 
         Raises:
             TimeoutError: The expected event does not occur within time limit.
@@ -65,25 +74,27 @@ class CallbackHandler(object):
         if timeout:
             timeout *= 1000  # convert to milliseconds for java side
         try:
-            event = self._event_client.eventWaitAndGet(self._id, event_name,
-                                                       timeout)
+            raw_event = self._event_client.eventWaitAndGet(self._id,
+                                                           event_name, timeout)
         except Exception as e:
             if "EventSnippetException: timeout." in str(e):
                 raise TimeoutError(
                     'Timeout waiting for event "%s" triggered by %s (%s).'
                     % (event_name, self._method_name, self._id))
             raise
-        return event
+        return snippet_event.from_dict(raw_event)
 
     def getAll(self, event_name):
-        """Gets all the events of a certain name that have been received so far.
-        This is a non-blocking call.
+        """Gets all the events of a certain name that have been received so
+        far. This is a non-blocking call.
 
         Args:
             callback_id: The id of the callback.
             event_name: string, the name of the event to get.
 
         Returns:
-            A list of dicts, each dict representing an event from the Java side.
+            A list of SnippetEvent, each representing an event from the Java
+            side.
         """
-        return self._event_client.eventGetAll(self._id, event_name)
+        raw_events = self._event_client.eventGetAll(self._id, event_name)
+        return [snippet_event.from_dict(msg) for msg in raw_events]
diff --git a/mobly/controllers/android_device_lib/snippet_event.py b/mobly/controllers/android_device_lib/snippet_event.py
new file mode 100644
index 0000000..0947a55
--- /dev/null
+++ b/mobly/controllers/android_device_lib/snippet_event.py
@@ -0,0 +1,50 @@
+# Copyright 2017 Google Inc.
+# 
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+# 
+#     http://www.apache.org/licenses/LICENSE-2.0
+# 
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import logging
+import time
+
+
+def from_dict(event_dict):
+    """Create a SnippetEvent object from a dictionary.
+
+    Args:
+        event_dict: a dictionary representing an event.
+
+    Returns:
+        A SnippetEvent object.
+    """
+    return SnippetEvent(
+        callback_id=event_dict['callbackId'],
+        name=event_dict['name'],
+        creation_time=event_dict['time'],
+        data=event_dict['data'])
+
+
+class SnippetEvent(object):
+    """The class that represents callback events for mobly snippet library.
+
+    Attributes:
+        callback_id: string, the callback ID associated with the event.
+        name: string, the name of the event.
+        creation_time: int, the epoch time when the event is created on the
+                       Rpc server side.
+        data: dictionary, the data held by the event. Can be None.
+    """
+
+    def __init__(self, callback_id, name, creation_time, data):
+        self.callback_id = callback_id
+        self.name = name
+        self.creation_time = creation_time
+        self.data = data


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 dbbb692..d121fe8 100755
--- a/tests/mobly/controllers/android_device_lib/callback_handler_test.py
+++ b/tests/mobly/controllers/android_device_lib/callback_handler_test.py
@@ -21,19 +21,47 @@ from mobly.controllers.android_device_lib import callback_handler
 from mobly.controllers.android_device_lib import jsonrpc_client_base
 
 MOCK_CALLBACK_ID = "1-0"
+MOCK_RAW_EVENT = {
+    'callbackId': '2-1',
+    'name': 'AsyncTaskResult',
+    'time': 20460228696,
+    'data': {
+        'exampleData': "Here's a simple event.",
+        'successful': True,
+        'secretNumber': 12
+    }
+}
 
 
 class CallbackHandlerTest(unittest.TestCase):
     """Unit tests for mobly.controllers.android_device_lib.callback_handler.
     """
 
+    def test_event_dict_to_snippet_event(self):
+        mock_event_client = mock.Mock()
+        mock_event_client.eventWaitAndGet = mock.Mock(
+            return_value=MOCK_RAW_EVENT)
+        handler = callback_handler.CallbackHandler(
+            callback_id=MOCK_CALLBACK_ID,
+            event_client=mock_event_client,
+            ret_value=None,
+            method_name=None)
+        event = handler.waitAndGet('ha')
+        self.assertEqual(event.name, MOCK_RAW_EVENT['name'])
+        self.assertEqual(event.creation_time, MOCK_RAW_EVENT['time'])
+        self.assertEqual(event.data, MOCK_RAW_EVENT['data'])
+        self.assertEqual(event.callback_id, MOCK_RAW_EVENT['callbackId'])
+
     def test_wait_and_get_timeout(self):
         mock_event_client = mock.Mock()
         java_timeout_msg = 'com.google.android.mobly.snippet.event.EventSnippet$EventSnippetException: timeout.'
         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, None)
+        handler = callback_handler.CallbackHandler(
+            callback_id=MOCK_CALLBACK_ID,
+            event_client=mock_event_client,
+            ret_value=None,
+            method_name=None)
         expected_msg = 'Timeout waiting for event "ha" .*'
         with self.assertRaisesRegexp(callback_handler.TimeoutError,
                                      expected_msg):