Closed Bug 1509534 Opened 2 years ago Closed 2 years ago
Simplify Android run
App / remoteautomation
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.
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
Attachment #9027286 - Flags: review?(bob)
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.split(posixpath.sep)[-1] Perhaps move this... @@ +262,5 @@ > + self.procName = cmd.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.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) ?
Attachment #9027286 - Flags: review?(bob) → review+
Updated for review comments - thanks!! https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=d260560ef43bd8fa2cdc775537fa2b7f3f72d979
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecfb40d8aacc Simplify Android runApp; r=bc
You need to log in before you can comment on or make changes to this bug.