1509534 - Simplify Android runApp / remoteautomation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, enhancement, P1)

Description

[Geoff Brown [:gbrown] (pto Apr 11-18)]

In bug 1507207, one of the challenges is passing the e10s request from the harness to the mozdevice launch_ function, since there is such a convoluted path through automation/remoteautomation. Let's try to simplify that path.

Comment 1

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: simplify Android runApp

There is no rush for review here -- some time next week is fine!

mochitest and reftest harnesses call automation.runApp() to launch a browser.

Before this patch, both desktop and Android used automation.runApp, which created and used a Process object. On Android, remoteautomation hooked in RProcess in place of Process.

With this patch, desktop continues without modification. Android overrides runApp in remoteautomation. RProcess is removed (collapsed into remoteautomation).

I am unhappy with remoteautomation in general. This doesn't resolve that discomfort entirely, but I think it is a step in the right direction, and enables the change I want to make for bug 1507207.
 
I've tested extensively with bug 1507207, which I intend to land at the same time; I think this patch is okay stand-alone, but I'm not completely confident of that.

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=6f64a29d0798d3ff8038222b2d01a0db22089c16

Comment 2

[Bob Clary [:bc] (inactive)]

Comment on attachment 9027286 [details] [diff] [review]
simplify Android runApp

Review of attachment 9027286 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a fix for the string interpolation existing issue. The other stuff is optional depending on how you feel about it.

::: build/mobile/remoteautomation.py
@@ +57,5 @@
> +        startTime = datetime.datetime.now()
> +
> +        self.lastTestSeen = "remoteautomation.py"
> +        self.launchApp([cmd] + args,
> +                       self.environment(env, crashreporter=not debuggerInfo),

This is defined in def launchApp as an optional parameter env=None. I would like to keep that distinction clear and not rely on position. The same goes for environment. Can we say env=self.environment(env=env, ...) ?

@@ +123,1 @@
>              print("Browser unexpectedly found running. Killing...")

Can we log the procName here as well? It might be interesting to know for sure whom we are killing.

@@ +258,5 @@
>          return app, args
>  
> +    def launchApp(self, cmd, env=None, messageLogger=None, counts=None):
> +        self.messageLogger = messageLogger
> +        self.procName = cmd[0].split(posixpath.sep)[-1]

Perhaps move this...

@@ +262,5 @@
> +        self.procName = cmd[0].split(posixpath.sep)[-1]
> +        self.stdoutlen = 0
> +
> +        if self.appName and self.device.process_exist(self.appName):
> +            print("remoteautomation.py %s is already running. Stopping...")

missing the self.appName for the string interpolation.

@@ +276,5 @@
> +            cmd = ' '.join(cmd)
> +            self.procName = self.appName
> +            if not self.device.shell_bool(cmd):
> +                print("remote_automation.py failed to launch %s" % cmd)
> +        else:

to here...
        self.procName = cmd[0].split(posixpath.sep)[-1]

@@ -243,3 @@
>  
> -            if app and self.device.process_exist(app):
> -                print("remoteautomation.py %s is already running. Stopping...")

Looks like this was an already existing issue. No time like the present to fix those! ;-)

@@ +322,5 @@
> +        try:
> +            newLogContent = self.device.get_file(self.remoteLog, offset=self.stdoutlen)
> +        except ADBTimeoutError:
> +            raise
> +        except Exception:

Do you think it would be worth logging something about the exception so we can be aware of it in case it is a symptom of another problem?

@@ +436,5 @@
> +            # Do not use the on-device screenshot options since
> +            # they rarely work well with Firefox on the Android
> +            # emulator. dump_screen provides an effective
> +            # screenshot of the emulator and its host desktop.
> +            dump_screen(self.utilityPath, get_default_logger())

This was confusing to me. Can we move the comment about the emulator above the conditional and do something like:

# Take a screenshot to capture the screen state just before
# the application is killed.
# Do not use the on-device screenshot options for the emulator since
# they rarely work well with Firefox on the Android
# emulator. dump_screen provides an effective
# screenshot of the emulator and its host desktop.
if self.utilityPath and self.device._device_serial.startswith('emulator-'):
    dump_screen(self.utilityPath, get_default_logger())
else:
    dump_device_screen(self.device, get_default_logger())

::: layout/tools/reftest/remotereftest.py
@@ +180,2 @@
>          self.automation = RemoteAutomation(self.device, options.app, self.remoteProfile,
> +                                           options.remoteLogFile, processArgs=args)

Again we are using optional parameters as positional ones, Can we make them explicit with 

self.automation = RemoteAutomation(self.device, appName=options.app, remoteProfile=self.remoteProfile,
                                   remoteLog=options.remoteLogFile, processArgs=args)

?

Comment 3

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Attachment: simplify Android runApp

Updated for review comments - thanks!!

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=d260560ef43bd8fa2cdc775537fa2b7f3f72d979

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecfb40d8aacc
Simplify Android runApp; r=bc

Comment 5

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ecfb40d8aacc