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

Simplify Chrome Android setup #9188

Merged
merged 2 commits into from Feb 2, 2018

Conversation

Projects
None yet
6 participants
@Hexcles
Copy link
Member

Hexcles commented Jan 24, 2018

Automatically start adb, connect to device, and set up port forwarding.


This change is Reviewable

@Hexcles Hexcles requested review from jgraham and kereliuk Jan 24, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Jan 25, 2018

Build PASSED

Started: 2018-02-01 22:20:53
Finished: 2018-02-01 22:38:33

View more information about this build on:

@tobie

This comment has been minimized.

Copy link
Contributor

tobie commented Jan 25, 2018

Summoning @wpt-pr-bot.

@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Jan 29, 2018

@jgraham the PR is ready for review

Most of the change is incremental improvements in our draft Chrome Android setup, except this line, which fixes the setup/cleanup imbalance I told you about:
f71d30a#diff-35e3a18456dc70eea5ca7fff107a1a46L232

@foolip
Copy link
Contributor

foolip left a comment

If @jgraham wants to take a look that would be most kind, but I'm happy to do my best and have tried.

I also tried running the wpt run chrome_android speech-api on my Mac and did see Chrome on my phone doing some things, but after a few tries I see a lot of this:

objc[66566]: +[__NSPlaceholderDate initialize] may have been in progress in another thread when fork() was called.
objc[66566]: +[__NSPlaceholderDate initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead. Set a breakpoint on objc_initializeAfterForkError to debug.

And then nothing seems to really work. But I haven't rooted this phone to update the hosts file, and it seems to work about the same without the changes. If confirmed to actually work somewhere, that's good! :)

@@ -35,6 +35,10 @@ def install(self, dest=None):
def install_webdriver(self):
return NotImplemented

@abstractmethod
def find_webdriver(self):

This comment has been minimized.

Copy link
@foolip

foolip Feb 1, 2018

Contributor

With no other changes to find_webdriver in this PR, an explanation of why this change is needed in the commit message would be good. Does this mean that a subclass not implementing this will blow up? (You can write a block in a comment describing what the commit message should be when merging.)

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 1, 2018

Author Member

Actually, let me hack the history of this branch. I'll make a feature change commit + a code health commit. And then we can rebase-merge this PR.

self._adb_run(['wait-for-device'])
self._adb_run(['forward', '--remove-all'])
self._adb_run(['reverse', '--remove-all'])
# TODO: Use ports from config.

This comment has been minimized.

Copy link
@foolip

foolip Feb 1, 2018

Contributor

Some logging confirms that there are in server_config in executor_kwargs, and I'm guessing the executor_kwargs return value ends up somehow propagated to the ChromeAndroidBrowser instance? In other words, is this easy to fix already? If so let's fix it, if it's hard because of some mis-layering, maybe file an issue about that layering problem.

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 1, 2018

Author Member

No, it doesn't. Although there is a hook for Browser modules to modify executor_kwargs, but the dictionary is not passed back to Browser classes, as Browsers are not Executors... I'm going to store the server_config in a module-local variable. It's not ideal, but probably better than hardcoding.

@Hexcles Hexcles force-pushed the chrome-android branch from f71d30a to 3735a95 Feb 1, 2018

@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Feb 1, 2018

I ended up splitting the changes into two PRs. See #9349 for the follow-up code health fixes. @foolip

@Hexcles Hexcles force-pushed the chrome-android branch from 3735a95 to b73f8e1 Feb 1, 2018

Simplify Chrome Android setup
Automatically start adb, connect to device, and set up port forwarding.
The list of ports to be forwarded is read from server config at run
time. When a test run is completed, port forwarding is torn down.

Also fix testrunner so that setup & cleanup balance out (called at enter
& exit of a Browser context, respectively). Previously, cleaup() didn't
do anything in all browsers, so the issue wasn't discovered. In this
commit, cleanup() is used for uninstalling port forwarding.

Document for Chrome Android is adjusted accordingly.

@Hexcles Hexcles force-pushed the chrome-android branch from b73f8e1 to 5a8d6a0 Feb 1, 2018

@foolip

foolip approved these changes Feb 2, 2018

@@ -229,7 +229,6 @@ def stop(self, force=False):
def cleanup(self):
if self.init_timer is not None:
self.init_timer.cancel()
self.browser.cleanup()

This comment has been minimized.

Copy link
@foolip

foolip Feb 2, 2018

Contributor

Wait was this removal intentional?

This comment has been minimized.

Copy link
@Hexcles

Hexcles Feb 2, 2018

Author Member

Yes, indeed, and it's quite important. See the commit message:

Also fix testrunner so that setup & cleanup balance out (called at enter
& exit of a Browser context, respectively). Previously, cleaup() didn't
do anything in all browsers, so the issue wasn't discovered. In this
commit, cleanup() is used for uninstalling port forwarding.

which is all about the removal of this one line.

@Hexcles Hexcles merged commit 71abd86 into master Feb 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Hexcles

This comment has been minimized.

Copy link
Member Author

Hexcles commented Feb 2, 2018

Also, @foolip regarding the errors you saw, I'm quite certain they are not related to my change. And I'm quite confused: where exactly do we ever call a compiler (objc)?

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 5, 2018

@foolip's errors appear to just be #6998 manifesting in a slightly less crashy way.

@gsnedders gsnedders deleted the chrome-android branch Feb 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.