1214812 - Autophone - re-enable mochitest-dom-media tests

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

We currently only have the mochitest-dom-media tests available on Try due to their perma orange. We need to re-enable them at least on a limited set of repos/devices.

Comment 1

[Bob Clary [:bc] (inactive)]

try run on mozilla-central 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c74f6de52dbe&exclusion_profile=false

nexus-4-jdq39-4
nexus-5-kot49h-5
nexus-7-jss15q-2
nexus-s-4

We'll see how this goes.

Comment 2

[Bob Clary [:bc] (inactive)]

The initial try run failed all devices with exceptions. I re-tested today and the nexus-s-4 device successfully ran Mdb Mdm Mm Mtw but the Nexus 4, Nexus 5 and Nexus 7 devices all failed. The issue appears to be the error:

Automation Error: No crash directory (/storage/sdcard0/tests/profile/minidumps/) found on remote device

The nexus s devices all run shell as the shell user/group but the nexus 4/5/7 devices all run shell as root. Not sure what is up yet but I expect permissions to be involved somehow. I don't see this in my local gs3-3 or nexus one devices.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Eugen, you have a Nexus 7. Can you try running the dom/media mochitests there and see what happens?

Comment 4

[Eugen Sawin [:esawin]]

Running mochitests dom/media locally (on Linux) on a N7 gives me:

Automation Error: [...] ImportError: No module named fix_linux_stack

Comment 5

[Bob Clary [:bc] (inactive)]

I would expect the import error to be an issue with your host installation.

I (finally) got a fennec opt build locally with the new sdk path requirements, then with MOZCONFIG pointing to the fennec mozconfig I used to build, did:

export MOZCONFIG=pathtomozconfig
export MOZ_HOST_BIN=$(pwd)/firefox-debug/dist/bin

# My gs3. I had to use this since I have multiple devices connected
export ANDROID_SERIAL=501a2aa7

mach -f plain dom/media
...
Ran 840 tests (229 parents, 611 subtests)
Expected results: 750
Unexpected results: 31 (FAIL: 31)
Skipped: 58

Note in my case fix_linux_stack.py is in my $MOZ_HOST_BIN directory:
./firefox-debug/dist/bin/fix_linux_stack.py

esawin: Can you check your MOZ_HOST_BIN?

Comment 6

[Bob Clary [:bc] (inactive)]

I will enable debug level logging temporarily over the weekend and attempt to diagnose the issue. I can't run debug level logging during peak load times due to memory issues.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-v1.patch

I think the problem is a discrepency with where the profile is located and where it is expected along with the redundant crash processing by AutophoneCrashProcessor.

Test run with local devices: https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&revision=b456daa0503f&filter-searchStr=autophone

This is working locally but the proof will be in running it in production.

Comment 8

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

Comment on attachment 8684201 [details] [diff] [review]
bug-1214812-v1.patch

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

::: tests/runtestsremote.py
@@ +79,4 @@
>  
>      def setup_job(self):
>          PhoneTest.setup_job(self)
> +        # Remove the AutophonCrashProcessor set in PhoneTest.setup_job

nit: add an 'e' in Autophone

Comment 9

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/72de3bcbc4a13815dab4093848bafb826fbb79a2
deployed 2015-11-06 11:15:30,519

Leave open until Mdm is tested.

Comment 10

[Bob Clary [:bc] (inactive)]

No joy in Mudville:

MochitestServer : launching [u'/mozilla/projects/autophone/downloads/FirefoxNightly.app/Contents/MacOS/xpcshell', '-g', '/mozilla/projects/autophone/downloads/FirefoxNightly.app/Contents/Resources', '-v', '170', '-f', '/mozilla/projects/autophone/downloads/FirefoxNightly.app/Contents/MacOS/components/httpd.js', '-e', "const _PROFILE_PATH = '/var/folders/94/zbj0mcp54hx99z748npbknzm0000gn/T/tmpUG6wdb.mozrunner'; const _SERVER_PORT = '61984'; const _SERVER_ADDR = '10.252.73.89'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = false;", '-f', '/mozilla/projects/autophone/src/autophone/builds/aHR0cDovL2FyY2hpdmUubW96aWxsYS5vcmcvcHViL21vYmlsZS90cnktYnVpbGRzL2JjbGFyeUBtb3ppbGxhLmNvbS02YjkxOGE3ZDVhZjlmMDNmM2FjMjVjZTNlNDZjY2FmNDcyMzYxYjY1L3RyeS1hbmRyb2lkLWFwaS0xMS9mZW5uZWMtNDUuMGExLmVuLVVTLmFuZHJvaWQtYXJtLmFwaw==/tests/mochitest/server.js']
runtests.py | Server pid: 46511
runtests.py | Websocket server pid: 46512
runtests.py | SSL tunnel pid: 46513
runtests.py | Running tests: start.

INFO | automation.py | Application pid: 0
INFO | automation.py | Application ran for: 0:00:12.012772
INFO | zombiecheck | Reading PID log: /var/folders/94/zbj0mcp54hx99z748npbknzm0000gn/T/tmp_l40X0pidlog
/data/tombstones does not exist; tombstone check skipped
Automation Error: No crash directory (/storage/sdcard0/tests/autophone/profile/minidumps/) found on remote device

Comment 11

[Bob Clary [:bc] (inactive)]

fyi, I *can* reproduce this on my nexus 6 device locally. Debugging now.

Comment 12

[Bob Clary [:bc] (inactive)]

I have been testing patches and finally have at least my local devices and the Android 2.3 devices in production running Mdm successfully. They can also run the mochitest robocop and reftest based tests successfully on all devices.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99b2d0a39bd6&exclusion_profile=false
https://treeherder.allizom.org/#/jobs?repo=try&revision=99b2d0a39bd6&exclusion_profile=false

The basic issues fall into two groups:

* proper detection of rooting and the use of su especially for the newer nexus 6/9 devices which are user debug AOSP builds which do not run adbd as root and which have the Android su 0 command... usage. This affects both Autophone and devicemanager.

* too long command lines. Unfortunately we still use devicemanager launchProcess to launch the mochitest runner which creates one long command line which results in an error which is not reported consistently (required me to run in a debugger). The error appears as something like "service name too long" in the stdout of the Popen. This is due to launchProcess which creates the command line via acmd = ["shell", ' '.join(map(lambda x: '"' + x + '"', ["am", "start"] + acmd))] and then calls _checkCmd with this an arg list like ['adb', '-s', 'serial', 'largestring'].

I've reduced the command line arguments in Autophone for the unit tests by realizing that the prefs and env variables are already handled in the unit test runners. This helped but does not completely resolve the issue.

The proper fix for the too long command line will be to use devicemanager launchApplication which passes the args to Popen as a full list rather than concatenating most of them into a string.

gbrown, wlach, jmaher: I'd appreciate your opinions on the best way to fix this issue in mozdevice/devicemanagerADB.py.

Comment 13

[William Lachance (:wlach)]

(In reply to Bob Clary [:bc:] from comment #12)
> gbrown, wlach, jmaher: I'd appreciate your opinions on the best way to fix
> this issue in mozdevice/devicemanagerADB.py.

It's been a while since I've looked at this. I recall wanting to switch mochitest / automation.py to use launchApplication, but I recall it seemed difficult for some reason (robocop?). I'd be in favor of moving everything over to adb.py long term (I think we talked about this last year) but don't really have any short-term suggestions, sorry. :(

Comment 14

[Geoff Brown [:gbrown]]

I believe "service name too long" happens when the adb command line exceeds 1024 characters: https://code.google.com/p/android/issues/detail?id=23351. 

I ran into similar problems with Android xpcshell tests some time ago and resolved that by writing a shell script, pushing the script to device, and then executing the script via adb shell. Maybe we should do something like that for test browser launches also.

http://hg.mozilla.org/mozilla-central/annotate/4294bf91174b/testing/xpcshell/remotexpcshelltests.py#l299

Comment 15

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

moving to adb.py is an investment- something to consider later this quarter or next quarter.  In the short term can we reduce the commandline options to adb by adding them as prefs or other environment/config variables?  That might be a little bit of work, but probably less than a wholesale conversion to adb.py.

Another option is to move to adb.py for the dmd tests only!  Then we have a working model for it and can easily port the rest of the tests to use it as time permits.  Do feel free to ask for help and divide tasks up- we can help out!

Comment 16

[Bob Clary [:bc] (inactive)]

Lots of orange but no exceptions in production or with my local devices which include Nexus 6 (Android 5.1) and Nexus 9 (Android 5.0)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1769ac687a87&exclusion_profile=false
https://treeherder.allizom.org/#/jobs?repo=try&revision=1769ac687a87&exclusion_profile=false&selectedJob=14631238

I have patches for mozdevice and autophone which I'll attach here. They share 3 patches which are for adb.py and adb_android.py in both mozdevice and autophone.

Comment 17

[Bob Clary [:bc] (inactive)]

Attachment: devicemanagerADB-su.patch

Bug 1214812 - [mozdevice] - devicemanagerADB.py - use su 0 if it is available; check for root updates

Necessary for basic AOSP builds which native android su without Superuser type version of su.

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: devicemanagerADB-ls.patch

Bug 1214812 - [mozdevice] - devicemanagerADB.py - check if busybox's ls -1A is used

This is necessary on some devices which have busybox installed like my Nexus One with CyanogenMod builds.

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: devicemanagerADB-listFiles-busy.patch

Bug 1214812 - [mozdevice] - devicemanagerADB.py - listFiles - check for device or resource busy failure message

Found this potential failure message with my Nexus One.

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-01-v1.patch

Bug 1214812 - [mozdevice] - adb.py - do not prepend LD_LIBRARY_PATH to su commands

I did this originally modelling the code after that used in devicemanagerSUT, but it is not necessary and does not work with AOSP builds as written. This just removes the LD_LIBRARY_PATH stuff altogether.

Comment 21

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-02-v1.patch

Bug 1214812 - [mozdevice] - adb.py, adb_android.py - if possible always run adbd as root, set SELinux to Permissive

The way the unit test runners are written assumes that the devices are running adbd as root for the most part. This patch makes the device run adbd as root if possible and eliminates potential errors due to parts of the test runners not requesting root when needed. This is especially important on newer Androids which have Enforcing SELinux. As part of this patch, SELinux is also set to Permissive during initialization and after reboots.

Comment 22

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-03-v1.patch

Bug 1214812 - [mozdevice] - adb_android.py - fix missing test_root argument to ADBAndroid

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-01-v1.patch

same as  attachment 8692728 [details] [diff] [review]  bug-1214812-01-v1.patch except for autophone

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-02-v1.patch

same as attachment 8692729 [details] [diff] [review] bug-1214812-02-v1.patch except for autophone

Comment 25

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-03-v1.patch

Bug 1214812 - Autophone - worker should check SELinux setting during ping

Comment 26

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-04-v1.patch

Bug 1214812 - Autophone - explicitly use ADBAndroid

basically stops the confusing renaming of ABDAndroid to ADBDevice and just uses ABDAndroid explicitly.

Comment 27

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-05-v1.patch

Bug 1214812 - Autophone - explicitly pass remoteTestRoot and do not use AutophoneCrashProcessor for UnitTests

Same as attachment 8684201 [details] [diff] [review] bug-1214812-v1.patch. Carrying forward jmaher's r+.

Comment 28

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-06-v1.patch

Bug 1214812 - Autophone - add failure message from UnitTest exception failure

Comment 29

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-07-v1.patch

Bug 1214812 - Autophone - use absolute paths when creating UnitTest PYTHONPATH

This isn't strictly necessary but it seems better to have both a belt and suspenders.

Comment 30

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-08-v1.patch

Bug 1214812 - Autophone - add configurable test_root

gbrown, I experimented with changing the test root to /data/local/tests but found that it actually had more problems with permissions than before. But I like the flexibility this gives us to try to make it work better in the future. Rather than lose the work, I thought we could just land it and proceed from there.

Comment 31

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-09-v1.patch

Bug 1214812 - Autophone - adb_android.py - fix missing test_root argument to ADBAndroid

Autophone version of the bug-1214812-03-v1.patch

Comment 32

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-10-v1.patch

Bug 1214812 - Autophone - add remote-logfile option to unittests

Comment 33

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-11-v1.patch

Bug 1214812 - Autophone - map arm64-v8a to armv8

Comment 34

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-12-v1.patch

Bug 1214812 - Autophone - use short file names, reduce android command line sizes

I didn't go with the wrapper solution since the env vars were only part of the problem. The passing of all of the prefs, env vars and the really long testURL was the problem. Apart from the initial adb arguments we pass most of the command line via launchProcess in devicemanagerADB as a string which exceeds the command line length.

This patch reduces the command line size by eliminating the raw logs and using temporary file names for the lognames which are then renamed to more human readable versions when the test completes.

Comment 35

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-autophone-13-v1.patch

Bug 1214812 - Autophone - do not pass prefs/unittests to unittests

This patch removes the setting of the prefs and env vars via the command line. I realized this wasn't necessary in autophone's case since the actual unit test runners set the necessary prefs and env vars for the fennec process on the device.

Comment 36

[Bob Clary [:bc] (inactive)]

Some final thoughts.

I tried very hard to be able to get around the command line length limitation by not passing the majority of the arguments as a single long string. Popen can run a command with an arbitrarily long command line if you pass it a list of arguments. This continually failed to work for me. processhandler.py and its munging of Popen appeared to break the ability to pass all of the arguments as a list.

Even with the changes in bug-1214812-autophone-12-v1.patch and bug-1214812-autophone-13-v1.patch, the testURL is almost 1000 bytes. If we ever need to add more arguments to the test runners via the testURL, we will hit this command line limitation again.

I can set the test root explicitly to a short path. On some of our devices adb detects /storage/sdcard0/ for the sdcard which results in /storage/sdcard0/tests/autophone for the path to the autophone test directory. I like using a subdirectory of tests which gives us some safety from rm -r'ing the world. This will shorten the testURL somewhat. I didn't do it here since the softlink from /sdcard to /storage/sdcard0 might affect performance.

We can fix processhandler at some point so we can pass a list of arguments without having to join the arguments into a long string but this won't have an effect on the testURL if we continue to pass parameters to the test runner via the url.

Finally, we can modify the extensions installed in fennec by the test runners to accept command line arguments instead of passing the test runner arguments via the testURL.

Comment 37

[Geoff Brown [:gbrown]]

Comment on attachment 8692724 [details] [diff] [review]
devicemanagerADB-su.patch

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

If the _have variables can be None/True/False, let's set them to False in _checkForRoot; if only 2 states are possible, I'd prefer to keep True/False.

Otherwise, all's well here.

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
@@ +23,5 @@
>      port.
>      """
>  
> +    _haveRootShell = None
> +    _haveSu = None

Why the change from False to None?

@@ +104,5 @@
>          # If requested to run as root, check that we can actually do that
> +        if root:
> +            if self._haveRootShell is None and self._haveSu is None:
> +                self._checkForRoot()
> +            if not self._haveRootShell and not self._haveSu:

It feels like _haveRootShell and _haveSu are now tri-state (None/True/False) variables, but they are never set to False.

Comment 38

[Geoff Brown [:gbrown]]

Comment on attachment 8692725 [details] [diff] [review]
devicemanagerADB-ls.patch

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

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
@@ +265,5 @@
>                             retryLimit=retryLimit, timeout=timeout)
>              mozfile.remove(tmpDir)
>  
>      def dirExists(self, remotePath):
> +        self._detectLsModifier()

I think it would be okay to call _detectLsModifier from __init__ -- most clients will use one of these functions in a session.

Comment 39

[Geoff Brown [:gbrown]]

Comment on attachment 8692729 [details] [diff] [review]
bug-1214812-02-v1.patch

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

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +623,5 @@
>              return "usb:%s" % usb
>  
>          raise ValueError("Unable to get device serial")
>  
> +    def _check_adb_root(self, timeout=300):

Consider not using a timeout parameter default value, or using timeout=None.

::: testing/mozbase/mozdevice/mozdevice/adb_android.py
@@ +107,5 @@
> +        reboot() reboots the device, issues an adb wait-for-device in order to
> +        wait for the device to complete rebooting, then calls is_device_ready()
> +        to determine if the device has completed booting.
> +
> +        If the device supports running adbd as root, it adbd will be

s/it adbd/adbd/

Comment 40

[Geoff Brown [:gbrown]]

Comment on attachment 8692740 [details] [diff] [review]
bug-1214812-autophone-04-v1.patch

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

Thank you!

Comment 41

[Geoff Brown [:gbrown]]

Comment on attachment 8692745 [details] [diff] [review]
bug-1214812-autophone-08-v1.patch

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

::: autophone.py
@@ -561,5 @@
> -                    device['hardware'] = dm.get_prop('ro.product.model')
> -                    device['abi'] = dm.get_prop('ro.product.cpu.abi')
> -                    try:
> -                        sdk = int(dm.get_prop('ro.build.version.sdk'))
> -                        device['sdk'] = 'api-9' if sdk <= 10 else 'api-11'

I think you lose/change some logic here -- is that okay?

Comment 42

[Geoff Brown [:gbrown]]

Comment on attachment 8692751 [details] [diff] [review]
bug-1214812-autophone-12-v1.patch

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

I feel vaguely uneasy about this, but don't have specific advice. :)

::: tests/runtestsremote.py
@@ +246,5 @@
>          self.parms['ssl_port'] = self.parms['port_manager'].reserve()
>  
> +        # Create a short version of the remote logfile name.
> +        fh, temppath = tempfile.mkstemp(suffix='.log',
> +                                        dir='%s/tests' % self.parms['build_dir'])

Use the same indent style for all your mkstemp() calls, please.

Comment 43

[Bob Clary [:bc] (inactive)]

(In reply to Geoff Brown [:gbrown] from comment #37)
> Comment on attachment 8692724 [details] [diff] [review]
> devicemanagerADB-su.patch
> 
> Review of attachment 8692724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If the _have variables can be None/True/False, let's set them to False in
> _checkForRoot; if only 2 states are possible, I'd prefer to keep True/False.
> 

I wanted the tri-state so I wouldn't call _checkForRoot twice. I'll init them there as you suggest.

(In reply to Geoff Brown [:gbrown] from comment #38)
> Comment on attachment 8692725 [details] [diff] [review]
> devicemanagerADB-ls.patch

> I think it would be okay to call _detectLsModifier from __init__ -- most
> clients will use one of these functions in a session.

Good idea.


(In reply to Geoff Brown [:gbrown] from comment #39)
> 
> Consider not using a timeout parameter default value, or using timeout=None.

Thanks. I meant to change that and forgot.
 
> ::: testing/mozbase/mozdevice/mozdevice/adb_android.py
> @@ +107,5 @@
> > +        reboot() reboots the device, issues an adb wait-for-device in order to
> > +        wait for the device to complete rebooting, then calls is_device_ready()
> > +        to determine if the device has completed booting.
> > +
> > +        If the device supports running adbd as root, it adbd will be
> 
> s/it adbd/adbd/

Thanks.

(In reply to Geoff Brown [:gbrown] from comment #41)
> Comment on attachment 8692745 [details] [diff] [review]
> bug-1214812-autophone-08-v1.patch
> 
> Review of attachment 8692745 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: autophone.py
> @@ -561,5 @@
> > -                    device['hardware'] = dm.get_prop('ro.product.model')
> > -                    device['abi'] = dm.get_prop('ro.product.cpu.abi')
> > -                    try:
> > -                        sdk = int(dm.get_prop('ro.build.version.sdk'))
> > -                        device['sdk'] = 'api-9' if sdk <= 10 else 'api-11'
> 
> I think you lose/change some logic here -- is that okay?

It wasn't my intention to change it other than removing the duplicate code, but I don't quite see it yet. Maybe we can chat about it this afternoon after the project meeting and you can point out where you think it differs.

(In reply to Geoff Brown [:gbrown] from comment #42)
> Comment on attachment 8692751 [details] [diff] [review]
> bug-1214812-autophone-12-v1.patch
> 
> Review of attachment 8692751 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel vaguely uneasy about this, but don't have specific advice. :)
> 

I don't like it either. I think I'll look into the command line option thing more later.

> Use the same indent style for all your mkstemp() calls, please.

done.

Comment 44

[Bob Clary [:bc] (inactive)]

Attachment: devicemanagerADB-su.patch (v2)

Comment 45

[Geoff Brown [:gbrown]]

Comment on attachment 8693653 [details] [diff] [review]
devicemanagerADB-su.patch (v2)

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

Thanks.

Comment 46

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #43)
> (In reply to Geoff Brown [:gbrown] from comment #41)
> > Comment on attachment 8692745 [details] [diff] [review]
> > bug-1214812-autophone-08-v1.patch
> > 
> > Review of attachment 8692745 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: autophone.py
> > @@ -561,5 @@
> > > -                    device['hardware'] = dm.get_prop('ro.product.model')
> > > -                    device['abi'] = dm.get_prop('ro.product.cpu.abi')
> > > -                    try:
> > > -                        sdk = int(dm.get_prop('ro.build.version.sdk'))
> > > -                        device['sdk'] = 'api-9' if sdk <= 10 else 'api-11'
> > 
> > I think you lose/change some logic here -- is that okay?
> 
> It wasn't my intention to change it other than removing the duplicate code,
> but I don't quite see it yet. Maybe we can chat about it this afternoon
> after the project meeting and you can point out where you think it differs.

Sorry, I was mistaken -- I see now that nothing is missing.

Comment 47

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/902b985b3273
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eaf9f30b00d
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f94be2a3c1d
https://hg.mozilla.org/integration/mozilla-inbound/rev/df6d3dc3fca9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b5c66f34e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/b36710809f0c

Comment 48

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/37a5c4c20f7a2c865ec5f7364c8e4d9daec4d48b
https://github.com/mozilla/autophone/commit/fa810efab8e007d9ee5a7ec3d5f14329d00f0c7a
https://github.com/mozilla/autophone/commit/a623553d2e259257ea63778bfd92ccf3b69961dd
https://github.com/mozilla/autophone/commit/0c6e56442f0a5c7a7a5def095b12264423264d46
https://github.com/mozilla/autophone/commit/d50410c0093038aa72b59c34b05ad4a3026ddd0b
https://github.com/mozilla/autophone/commit/6ef2de4ee67a1440b508ce12ffd0e714c20454fe
https://github.com/mozilla/autophone/commit/4b0e4f19a5cc71ade046b62a35a02454d2e2e150
https://github.com/mozilla/autophone/commit/40f71e0f2405cd012eaf2ed71f1bbdf6a055cadd
https://github.com/mozilla/autophone/commit/a2e43233d5e02bb00d4c6a306c11a5a159da4f7e
https://github.com/mozilla/autophone/commit/f59da5d4f049f3ff444c969c44cf9cfdfd52ec48
https://github.com/mozilla/autophone/commit/4de4d7d412715fb7ec423e54016c204f1f1f52db
https://github.com/mozilla/autophone/commit/8e2faf674be14b5031b139882f14bdb8f875b5c0

Comment 49

[Bob Clary [:bc] (inactive)]

deployed 2015-12-01 14:50:11,916

Comment 50

[Bob Clary [:bc] (inactive)]

forgot to link the try runs before the commit:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cafb4b39fc5b&exclusion_profile=false
https://treeherder.allizom.org/#/jobs?repo=try&revision=cafb4b39fc5b&exclusion_profile=false

Comment 51

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/82e204405fe5 for apparently breaking tests on KK emulator x86:

https://treeherder.mozilla.org/logviewer.html#?job_id=18111408&repo=mozilla-inbound

Comment 52

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #43)

> 
> > I think it would be okay to call _detectLsModifier from __init__ -- most
> > clients will use one of these functions in a session.
> 

Actually, I think this is a problem. I realize I had originally put it in the methods rather than the initializer since the device is not always connected during initialization. 

https://public-artifacts.taskcluster.net/Dc5XH0Y2Q26bVqB8y8R94Q/0/public/test_info//mochitest_raw.log

{"thread": "MainThread", "level": "DEBUG", "pid": 378, "component": "mozdevice", "source": "mochitest", "time": 1449012562972, "action": "log", "message": "_runCmd - command: /home/worker/build/emulator/b2g-distro/out/host/linux-x86/bin/adb shell ls -1A/"}
{"thread": "ProcessReader", "level": "DEBUG", "pid": 378, "component": "mozdevice", "source": "mochitest", "time": 1449012566074, "action": "log", "message": "error: device not found"}
{"thread": "ProcessReader", "level": "DEBUG", "pid": 378, "component": "mozdevice", "source": "mochitest", "time": 1449012566075, "action": "log", "message": "* daemon not running. starting it now on port 5037 *"}
{"thread": "ProcessReader", "level": "DEBUG", "pid": 378, "component": "mozdevice", "source": "mochitest", "time": 1449012566075, "action": "log", "message": "* daemon started successfully *"}

devicemanager is notorious for not detecting errors with devices and this caused the new code to think -1A was supported when it was really just a device connection failure.

This try run is with an unmodified version of the ls -1A detection 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc691a8ba4b8&exclusion_profile=false

and it appears to be green. 

I think I'll put the detection back into the methods and also force spaces around the ls argument since it appears devicemanger is failing to separate the arguments with a space: e.g. db shell ls -1A/

Comment 53

[Bob Clary [:bc] (inactive)]

Attachment: bug-1214812-02-v2.patch

Old patch: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b36710809f0c&exclusion_profile=false

New patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06ff4a8724af&exclusion_profile=false

1. Moves the ls detection back to the methods to prevent the detection running before the device is connected.

2. Fixes a missing comma in the _runCmd in _detectLsModifier. doh!

Comment 54

[Bob Clary [:bc] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=24d3eab3db9c

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b234d1db6f34
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b477107b22
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac225883686
https://hg.mozilla.org/integration/mozilla-inbound/rev/620fbd06d749
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcdde085ebbb

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b234d1db6f34
https://hg.mozilla.org/mozilla-central/rev/80b477107b22
https://hg.mozilla.org/mozilla-central/rev/5ac225883686
https://hg.mozilla.org/mozilla-central/rev/620fbd06d749
https://hg.mozilla.org/mozilla-central/rev/dcdde085ebbb
https://hg.mozilla.org/mozilla-central/rev/24d3eab3db9c