Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite snippet management to account for new launch protocol. #216

Merged
merged 10 commits into from May 26, 2017

Conversation

adorokhine
Copy link
Collaborator

@adorokhine adorokhine commented May 24, 2017

  • Snippet will now try and detect v1 protocol and fall back to v0 if that
    fails.
  • Make snippet_client and sl4a_client responsible for bringup and teardown
    of their apks, instead of sprinkling the logic for this between
    jsonrpc_client_base, snippet_client, sl4a_client and android_device.
    This is needed because the retry structure is now different for v1
    snippets.
  • Change how device port is handled. Device port comes from device side in
    v1 snippets.
  • Speed up snippet startup by avoiding extra stop before starting a
    snippet.

Fixes #91
Fixes #215


This change is Reviewable

* Snippet will now try and detect v1 protocol and fall back to v0 if that
  fails.
* Make snippet_client and sl4a_client responsible for bringup and teardown
  of their apks, instead of sprinkling the logic for this between
  jsonrpc_client_base, snippet_client, sl4a_client and android_device.
  This is needed because the retry structure is now different for v1
  snippets.
* Change how device port is handled. Device port comes from device side in
  v1 snippets.
* Speed up snippet startup by avoiding extra stop before starting a
  snippet.
@xpconanfan
Copy link
Collaborator

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


mobly/utils.py, line 191 at r1 (raw file):

    for path in paths:
        p = abs_path(path)
        for dirPath, unused_subdirList, fileList in os.walk(p):

Simply use _ instead of unused_subdirList?


mobly/utils.py, line 429 at r1 (raw file):

    that matches a given regex pattern.

    This function is specifically used to grep strings from AdbProxy's

If this is specific to AdbProxy, should it live there instead?


mobly/controllers/android_device_lib/jsonrpc_client_base.py, line 124 at r1 (raw file):

    # Methods to be implemented by subclasses.

    def start_app(self):

Should this method's name be changed to something like start_app_and_connect?
(as long as the new name indicates that it also connects)


mobly/controllers/android_device_lib/snippet_client.py, line 35 at r1 (raw file):

    'am instrument -w -e action stop %s/' + _INSTRUMENTATION_RUNNER_PACKAGE)

# Maximum time to wait for the app to start on the device (10 minutes).

Could we keep this in base class and assign _APP_START_WAIT_TIME_V0 as an alias, so we don't have to dup this in two places.


mobly/controllers/android_device_lib/snippet_client.py, line 50 at r1 (raw file):

class SnippetClient(jsonrpc_client_base.JsonRpcClientBase):

Can we add some context on the v0 and v1 changes in the docstring here?


tools/sl4a_shell.py, line 67 at r1 (raw file):

        'Device serial to connect to (if more than one device is connected)')
    args = parser.parse_args()
    logging.basicConfig(level=logging.INFO)

why this change?


Comments from Reviewable

@adorokhine
Copy link
Collaborator Author

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


mobly/utils.py, line 191 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Simply use _ instead of unused_subdirList?

Done.


mobly/utils.py, line 429 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

If this is specific to AdbProxy, should it live there instead?

Changed this text; it's not specific to AdbProxy.


mobly/controllers/android_device_lib/jsonrpc_client_base.py, line 124 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Should this method's name be changed to something like start_app_and_connect?
(as long as the new name indicates that it also connects)

Done.


mobly/controllers/android_device_lib/snippet_client.py, line 35 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Could we keep this in base class and assign _APP_START_WAIT_TIME_V0 as an alias, so we don't have to dup this in two places.

Actually it shouldn't be duped, because the start time of sl4a has nothing to do with the start time of snippet. I want to reduce the sl4a timeout again but I don't want to do that in this PR.

The one in snippet will soon be deleted because v1 snippets don't have a timeout value like this.


mobly/controllers/android_device_lib/snippet_client.py, line 50 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Can we add some context on the v0 and v1 changes in the docstring here?

Done.


tools/sl4a_shell.py, line 67 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

why this change?

The same line already exists in snippet_shell so this makes it consistent. It makes it easier to debug because you can just flip this value and rerun to get debug logs. (there's no file logging happening in the shells so there's no other way to view debug logs at the moment.)


Comments from Reviewable

@xpconanfan
Copy link
Collaborator

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


mobly/controllers/android_device_lib/snippet_client.py, line 35 at r1 (raw file):

Previously, adorokhine (Alexander Dorokhine) wrote…

Actually it shouldn't be duped, because the start time of sl4a has nothing to do with the start time of snippet. I want to reduce the sl4a timeout again but I don't want to do that in this PR.

The one in snippet will soon be deleted because v1 snippets don't have a timeout value like this.

Should this comment mention the pending removal of this value?
Should the comment for the one in sl4a mention the coming reduction in that value?


Comments from Reviewable

@adorokhine
Copy link
Collaborator Author

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


mobly/controllers/android_device_lib/snippet_client.py, line 35 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Should this comment mention the pending removal of this value?
Should the comment for the one in sl4a mention the coming reduction in that value?

Done.


Comments from Reviewable

@xpconanfan
Copy link
Collaborator

:lgtm:


Review status: 0 of 10 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@adorokhine adorokhine merged commit efa488e into master May 26, 2017
@adorokhine adorokhine deleted the protov2 branch May 26, 2017 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants