google / mobly

E2E test framework for tests with complex environment requirements.
https://github.com/google/mobly
Apache License 2.0
623 stars 174 forks source link

Standardize Async RPC for snippet client v2 #826

Closed mhaoli closed 2 years ago

mhaoli commented 2 years ago

A followup of #793.

Changes include:


This change is Reviewable

xpconanfan commented 2 years ago

This is more complicated than I expected...

Different platforms and programming languages will inherently have different conventions for their snippet rpc impls. So we just need to provide a simple abstract class for each client to implement the corresponding rpc calls, which is just 2 or 3 methods.

On Tue, May 31, 2022, 22:04 Minghao Li @.***> wrote:

@.**** commented on this pull request.

In mobly/snippet/general_callback_handler.py https://github.com/google/mobly/pull/826#discussion_r886260166:

+{

  • 'id': ,
  • 'method': 'eventGetAll',
  • 'params': (, ), +}
  • +Timeout error message: +In the response of eventWaitAndGet, this class checks for the existence of +the substring "EventSnippetException: timeout." in the error message. If it +exists, this class throws a timeout error from the original error. +""" +from mobly.snippet import callback_handler_base +from mobly.snippet import errors

  • +class GeneralCallbackHandler(callback_handler_base.CallbackHandlerBase):

The iOS one is special because it has limitation on the function name of RPCs, which are for pulling events from server.

Through writing in this way, I'm trying to make this point clearer: if you need to implement a new callback handler, which methods should you override. These methods are marked as abstractmethod and you cannot instantiate you class without overriding them. This seems clearer than reminding the user via docstring which functions should be overridden.

So it seems that I'm making things complex and confusing?

— Reply to this email directly, view it on GitHub https://github.com/google/mobly/pull/826#discussion_r886260166, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARDNZOYWKJWPBQRPCFHPRTVM3ALLANCNFSM5W4VB64Q . You are receiving this because your review was requested.Message ID: @.***>

mhaoli commented 2 years ago

I think this PR provides the abstract class and one general handler class with 2 methods.

Is the reason the GeneralCallbackHandler looks complicated because of the protocol summarized in the docstring?

Because the current implementations of Android and Windows callback handler are exactly the same, I think maybe other platforms can also implement this in future. Thus I summarize their protocol for interacting with the server side. Platforms that meet this protocol can reuse this class. Platforms that do not meet this protocol can implement callback handlers by themselves.

Or should I remove these descriptions of the protocol and the "general" word from the class name.