728298 - DeviceManager needs a good, standard way of starting an Android application

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[William Lachance (:wlach)]

Starting an Android application is complicated. You want to do this with the "am" tool, but the syntax is fairly complicated. In the worst case, you have to specify all of these seperately:

* Appname (e.g. org.mozilla.fennec)
* Activity name (eg. "App", "BrowserActivity")
* Intent (e.g. android.intent.action.VIEW)
* Environment variables (e.g. DEBUG=1)
* URL (e.g. http://google.com)
* Extra command line options (e.g. -profile /path/to/profile/on/device)

Other times, you just want to run a simple shell command on the device. 

The problem is that our current methods (fireProcess and launchProcess) don't really distinguish these cases, so you have special case code all over the place to try to do the above (which only ever works for Fennec!). The also imply that you can get a useful logfile of the output of running these commands, which is rarely the case.

We need good, non-confusing abstractions for both of these cases so I wrote some. The motivating use-case for me is to be able to launch different types of browsers (stock Android, Chrome, etc.) for Eideticker, but I think they will ultimately be useful as replacements for fireProcess and launchProcess. To avoid boiling the earth, I've left the latter two in for now (but marked them as deprecated). Eventually I envision we would make Mochitest, Talos, etc. use my new methods and we can remove these methods.

Comment 1

[William Lachance (:wlach)]

Attachment: Patch to add shell() and launchApplication() methods

Comment 2

[William Lachance (:wlach)]

Oh, also one thing about this patch: It kind of makes a joke of devicemanager being a cross-platform abstraction. Honestly I'm not sure what to think/do about that: there are very real differences between how operating systems will stop/start applications and we *need* operating system specific abstractions to be able to deal with that if this is going to be useful for anything other than testing a specific version of mobile Firefox (i.e. assuming that the activity name and intent always stay the same).

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 598285 [details] [diff] [review]
Patch to add shell() and launchApplication() methods

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

this means we could remove some api's from sutagent once we convert existing scripts to use this.  

r- I would just want to ensure we have an option for blocking/nonblocking calls to launch a process.

::: build/mobile/devicemanagerADB.py
@@ +72,5 @@
> +    proc.wait()
> +    if proc.returncode == 0:
> +      return proc.stdout.read()
> +    else:
> +      return None

this waits until the process is completed?

Comment 4

[William Lachance (:wlach)]

So after some discussions with jmaher and geoffbrown, I'm convinced this could use a bit of refinement:

* We should have an option/variant of shell() to direct output to a file, to handle cases like bug 705192 (where we might have a very large log file of results).
* We should seperate out the Android-specific stuff into a Mixin and create seperate classes to handle that case (going with DroidADB and DroidSUT for now).

Re: Blocking/non-blocking

* shell's intent is to run a command to completion. I can't really think of a use-case where you wouldn't want it to be synchronous/blocking.
* launchApplication is mostly non-blocking in the sense that it exits after the application is successfully launched (but not completely started up). We could in theory provide a variant which didn't wait to see the process before returning, but again, I don't really see the use case there. I worry that it would be just giving people yet another opportunity to shoot themselves in the foot. :P

Comment 5

[Geoff Brown [:gbrown]]

(In reply to William Lachance (:wlach) from comment #4)
> * shell's intent is to run a command to completion. I can't really think of
> a use-case where you wouldn't want it to be synchronous/blocking.

That seems fine to me. For xpcshell, synchronous/blocking should be good.

Comment 6

[Geoff Brown [:gbrown]]

It would be helpful (perhaps necessary) for shell() to accept and handle env and cwd arguments also: The SUT "EXEC" command expects environment variables to be specified in a specific format.

Comment 7

[Geoff Brown [:gbrown]]

...and also deal with character escapes, so that characters like ( are handled properly by ADB/SUT: See https://bugzilla.mozilla.org/show_bug.cgi?id=705192#c12

Comment 8

[William Lachance (:wlach)]

Attachment: Revised patch incorporating feedback and suggestions from geoffbrown and jmaher

Here's a revised patch which addresses the following:

1. geoffbrown's use case (running a command on the device and getting the full log file without loading the whole thing into memory). at this point that's only implemented for SUT (it would be more effort to do for ADB and may not be worth it atm given that ADB is debugging-only)
2. shell command now returns return code of command, and only returns actually command output.
3. handle cwd and environment (based on earlier work by geoffbrown)
4. android-specific functionality (launchApplication) moved into a mixin in a file called "droid". Instantiate new classes "DroidADB" and "DroidSUT" to use.

To make (1) and (2) work, a fairly invasive refactoring of devicemanagerSUT was required. I am fairly confident that the external behaviour is unchanged, but we'll see after the try run I just scheduled. 

(I may need to update this patch for bitrot, etc. as things are applied to the build -- just posting this here as an FYI for now)

Comment 9

[Mozilla RelEng Bot]

Try run for 74f3f3d38c90 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=74f3f3d38c90
Results (out of 31 total builds):
    exception: 2
    success: 25
    warnings: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-74f3f3d38c90

Comment 10

[William Lachance (:wlach)]

Attachment: Updated patch for specifying shell command, launchApplication

Updated patch for bitrot.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 599603 [details] [diff] [review]
Updated patch for specifying shell command, launchApplication

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

just some simple questions, otherwise this is great.

::: build/mobile/devicemanager.py
@@ +227,5 @@
> +      acmd.extend(["-d", ''.join(['"', url, '"'])])
> +
> +    self.shell(acmd)
> +
> +    return self.processExist(app)

is there a chance we could have a timing issue here?  For example launch fennec might take a few seconds?

Also when running robocop, we launch the instrumentation but we don't do am start, we do am instrument.

::: build/mobile/devicemanagerSUT.py
@@ +265,5 @@
> +          # periodically flush data to output file to make sure it doesn't get
> +          # too big/unwieldly
> +          if len(data) > 1024:
> +              outputfile.write(data[0:1024])
> +              data = data[1024:]

how is this w.r.t performance?  we run many instances of dm on a foopie along with webservers, etc...

Comment 12

[William Lachance (:wlach)]

Attachment: Updated patch to correct merge problems

Comment 13

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #11)
> Comment on attachment 599603 [details] [diff] [review]
> Updated patch for specifying shell command, launchApplication
> 
> Review of attachment 599603 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just some simple questions, otherwise this is great.
> 
> ::: build/mobile/devicemanager.py
> @@ +227,5 @@
> > +      acmd.extend(["-d", ''.join(['"', url, '"'])])
> > +
> > +    self.shell(acmd)
> > +
> > +    return self.processExist(app)
> 
> is there a chance we could have a timing issue here?  For example launch
> fennec might take a few seconds?

Good point. We should probably spin until the process has actually started here. I'll update the patch (shouldn't affect the try server run I'm doing, as nothing calls this code yet)

> Also when running robocop, we launch the instrumentation but we don't do am
> start, we do am instrument.

Yes, we may want to update this for robocop, which I'm not really very familiar with. How about we handle that in a seperate bug?

> ::: build/mobile/devicemanagerSUT.py
> @@ +265,5 @@
> > +          # periodically flush data to output file to make sure it doesn't get
> > +          # too big/unwieldly
> > +          if len(data) > 1024:
> > +              outputfile.write(data[0:1024])
> > +              data = data[1024:]
> 
> how is this w.r.t performance?  we run many instances of dm on a foopie
> along with webservers, etc...

This should actually be better than before in the worst case, where we always processed the entire output from the agent multiple times. Probably only important for edge cases like xpcshell thoguh, where we're processing an entire log file. Parsing a few kilobytes is never going to be very slow.

Comment 14

[Mozilla RelEng Bot]

Try run for d22280b55574 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d22280b55574
Results (out of 17 total builds):
    exception: 2
    success: 1
    warnings: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d22280b55574

Comment 15

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #13)
> (In reply to Joel Maher (:jmaher) from comment #11)
> > Comment on attachment 599603 [details] [diff] [review]
> > Updated patch for specifying shell command, launchApplication
> > 
> > Review of attachment 599603 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > just some simple questions, otherwise this is great.
> > 
> > ::: build/mobile/devicemanager.py
> > @@ +227,5 @@
> > > +      acmd.extend(["-d", ''.join(['"', url, '"'])])
> > > +
> > > +    self.shell(acmd)
> > > +
> > > +    return self.processExist(app)
> > 
> > is there a chance we could have a timing issue here?  For example launch
> > fennec might take a few seconds?
> 
> Good point. We should probably spin until the process has actually started
> here. I'll update the patch (shouldn't affect the try server run I'm doing,
> as nothing calls this code yet)

Come to think of it, maybe it would be better just to return right away. Just because the process exists it doesn't mean it's ready to do anything.

Maybe a better alternative here is to return the following:

On success (shell command to start app returned '0'): Return True 
On failure (shell command to start app didn't return '0'): Return False

Comment 16

[Geoff Brown [:gbrown]]

Comment on attachment 599641 [details] [diff] [review]
Updated patch to correct merge problems

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

Good stuff -- this should work for xpcshell too.

::: build/mobile/devicemanager.py
@@ +648,5 @@
> +
> +      # truncate off the return code line
> +      file.truncate(length - bytes_from_end)
> +      file.seek(0,2)
> +      file.write(0)

I think you intended: file.write('\0') ?

As it is, I get:

Traceback (most recent call last):
  File "/home/mozdev/src/config/pythonpath.py", line 52, in <module>
    main(sys.argv[1:])
  File "/home/mozdev/src/config/pythonpath.py", line 44, in main
    execfile(script, frozenglobals)
  File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 340, in <module>
    main()
  File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 335, in main
    **options.__dict__):
  File "/home/mozdev/src/testing/xpcshell/runxpcshelltests.py", line 516, in runTests
    stdout=pStdout, stderr=pStderr, env=self.env, cwd=testdir)
  File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 215, in launchProcess
    rc = self.device.shell(cmd, f, cwd=self.remoteHere, env=env)
  File "/home/mozdev/src/build/mobile/devicemanagerADB.py", line 96, in shell
    lastline = _pop_last_line(outputfile)
  File "/home/mozdev/src/build/mobile/devicemanager.py", line 652, in _pop_last_line
    file.write(0)
TypeError: expected a character buffer object

Comment 17

[Mozilla RelEng Bot]

Try run for c856fc3d065c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c856fc3d065c
Results (out of 29 total builds):
    exception: 2
    success: 3
    warnings: 24
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c856fc3d065c

Comment 18

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #16)

> I think you intended: file.write('\0') ?
> 
> As it is, I get:
> 
> Traceback (most recent call last):
>   File "/home/mozdev/src/config/pythonpath.py", line 52, in <module>
>     main(sys.argv[1:])
>   File "/home/mozdev/src/config/pythonpath.py", line 44, in main
>     execfile(script, frozenglobals)
>   File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 340,
> in <module>
>     main()
>   File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 335,
> in main
>     **options.__dict__):
>   File "/home/mozdev/src/testing/xpcshell/runxpcshelltests.py", line 516, in
> runTests
>     stdout=pStdout, stderr=pStderr, env=self.env, cwd=testdir)
>   File "/home/mozdev/src/testing/xpcshell/remotexpcshelltests.py", line 215,
> in launchProcess
>     rc = self.device.shell(cmd, f, cwd=self.remoteHere, env=env)
>   File "/home/mozdev/src/build/mobile/devicemanagerADB.py", line 96, in shell
>     lastline = _pop_last_line(outputfile)
>   File "/home/mozdev/src/build/mobile/devicemanager.py", line 652, in
> _pop_last_line
>     file.write(0)
> TypeError: expected a character buffer object

Hmm, I didn't get that behaviour here, but you are right that was what I intended. Will post a fixed patch soon.

Comment 19

[William Lachance (:wlach)]

(In reply to Mozilla RelEng Bot from comment #17)
> Try run for c856fc3d065c is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=c856fc3d065c
> Results (out of 29 total builds):
>     exception: 2
>     success: 3
>     warnings: 24
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/wlachance@mozilla.
> com-c856fc3d065c

Hmm, not sure what's going on here. I'm not seeing this kind of behaviour on my machine. Going to try running against try again.

Comment 20

[Mozilla RelEng Bot]

Try run for 8cd72c53c067 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=8cd72c53c067
Results (out of 28 total builds):
    success: 25
    warnings: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8cd72c53c067

Comment 21

[William Lachance (:wlach)]

Attachment: address a few more issues

final patch, addresses issue with appending integer, return value of launchApplication, and removes accidentally left-in launchApplication method in devicemanager

Comment 22

[William Lachance (:wlach)]

Comment on attachment 599971 [details] [diff] [review]
address a few more issues

I just realized that the agent changes (actually done by :gbrown) still need to be reviewed. Joel, could you take care of this? They look fine to me.

Comment 23

[William Lachance (:wlach)]

Comment on attachment 599971 [details] [diff] [review]
address a few more issues

My mistake, jmaher had already looked over the agent changes.

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de300d294f0

Comment 25

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/1de300d294f0