Closed Bug 1214812 Opened 5 years ago Closed 4 years ago

Autophone - re-enable mochitest-dom-media tests

Categories

(Testing :: Autophone, defect)

defect
Not set

Tracking

(firefox44 affected, firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 --- affected
firefox45 --- fixed

People

(Reporter: bc, Assigned: bc)

References

Details

Attachments

(21 files, 1 obsolete file)

5.91 KB, patch
gbrown
: review-
Details | Diff | Splinter Review
4.43 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
1.19 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
3.08 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
14.24 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
1.29 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
2.00 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
11.25 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
6.62 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
2.44 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
1.72 KB, patch
Details | Diff | Splinter Review
2.14 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
1.07 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
10.57 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
769 bytes, patch
gbrown
: review+
Details | Diff | Splinter Review
824 bytes, patch
gbrown
: review+
Details | Diff | Splinter Review
523 bytes, patch
gbrown
: review+
Details | Diff | Splinter Review
5.21 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
1.19 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
6.01 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
4.48 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
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.
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.
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.
Eugen, you have a Nexus 7. Can you try running the dom/media mochitests there and see what happens?
Flags: needinfo?(esawin)
Running mochitests dom/media locally (on Linux) on a N7 gives me:

Automation Error: [...] ImportError: No module named fix_linux_stack
Flags: needinfo?(esawin)
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?
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.
Attached patch bug-1214812-v1.patch (obsolete) — Splinter Review
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.
Attachment #8684201 - Flags: review?(jmaher)
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
Attachment #8684201 - Flags: review?(jmaher) → review+
https://github.com/mozilla/autophone/commit/72de3bcbc4a13815dab4093848bafb826fbb79a2
deployed 2015-11-06 11:15:30,519

Leave open until Mdm is tested.
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
fyi, I *can* reproduce this on my nexus 6 device locally. Debugging now.
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.
Flags: needinfo?(wlachance)
Flags: needinfo?(jmaher)
Flags: needinfo?(gbrown)
(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. :(
Flags: needinfo?(wlachance)
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
Flags: needinfo?(gbrown)
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!
Flags: needinfo?(jmaher)
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.
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.
Attachment #8692724 - Flags: review?(gbrown)
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.
Attachment #8692725 - Flags: review?(gbrown)
Bug 1214812 - [mozdevice] - devicemanagerADB.py - listFiles - check for device or resource busy failure message

Found this potential failure message with my Nexus One.
Attachment #8692726 - Flags: review?(gbrown)
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.
Attachment #8692728 - Flags: review?(gbrown)
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.
Attachment #8692729 - Flags: review?(gbrown)
Bug 1214812 - [mozdevice] - adb_android.py - fix missing test_root argument to ADBAndroid
Attachment #8692730 - Flags: review?(gbrown)
same as  attachment 8692728 [details] [diff] [review]  bug-1214812-01-v1.patch except for autophone
Attachment #8692734 - Flags: review?(gbrown)
same as attachment 8692729 [details] [diff] [review] bug-1214812-02-v1.patch except for autophone
Attachment #8692737 - Flags: review?(gbrown)
Bug 1214812 - Autophone - worker should check SELinux setting during ping
Attachment #8692739 - Flags: review?(gbrown)
Bug 1214812 - Autophone - explicitly use ADBAndroid

basically stops the confusing renaming of ABDAndroid to ADBDevice and just uses ABDAndroid explicitly.
Attachment #8692740 - Flags: review?(gbrown)
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+.
Attachment #8684201 - Attachment is obsolete: true
Attachment #8692741 - Flags: review+
Bug 1214812 - Autophone - add failure message from UnitTest exception failure
Attachment #8692742 - Flags: review?(gbrown)
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.
Attachment #8692744 - Flags: review?(gbrown)
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.
Attachment #8692745 - Flags: review?(gbrown)
Bug 1214812 - Autophone - adb_android.py - fix missing test_root argument to ADBAndroid

Autophone version of the bug-1214812-03-v1.patch
Attachment #8692746 - Flags: review?(gbrown)
Bug 1214812 - Autophone - add remote-logfile option to unittests
Attachment #8692747 - Flags: review?(gbrown)
Bug 1214812 - Autophone - map arm64-v8a to armv8
Attachment #8692748 - Flags: review?(gbrown)
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.
Attachment #8692751 - Flags: review?(gbrown)
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.
Attachment #8692752 - Flags: review?(gbrown)
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 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.
Attachment #8692724 - Flags: review?(gbrown) → review-
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.
Attachment #8692725 - Flags: review?(gbrown) → review+
Attachment #8692726 - Flags: review?(gbrown) → review+
Attachment #8692728 - Flags: review?(gbrown) → review+
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/
Attachment #8692729 - Flags: review?(gbrown) → review+
Attachment #8692730 - Flags: review?(gbrown) → review+
Attachment #8692734 - Flags: review?(gbrown) → review+
Attachment #8692737 - Flags: review?(gbrown) → review+
Attachment #8692739 - Flags: review?(gbrown) → review+
Comment on attachment 8692740 [details] [diff] [review]
bug-1214812-autophone-04-v1.patch

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

Thank you!
Attachment #8692740 - Flags: review?(gbrown) → review+
Attachment #8692742 - Flags: review?(gbrown) → review+
Attachment #8692744 - Flags: review?(gbrown) → review+
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?
Attachment #8692745 - Flags: review?(gbrown) → review+
Attachment #8692746 - Flags: review?(gbrown) → review+
Attachment #8692747 - Flags: review?(gbrown) → review+
Attachment #8692748 - Flags: review?(gbrown) → review+
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.
Attachment #8692751 - Flags: review?(gbrown) → review+
Attachment #8692752 - Flags: review?(gbrown) → review+
(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.
Attachment #8693653 - Flags: review?(gbrown)
Comment on attachment 8693653 [details] [diff] [review]
devicemanagerADB-su.patch (v2)

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

Thanks.
Attachment #8693653 - Flags: review?(gbrown) → review+
(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.
deployed 2015-12-01 14:50:11,916
(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/
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!
Flags: needinfo?(bob)
Attachment #8694767 - Flags: review?(gbrown)
Attachment #8694767 - Flags: review?(gbrown) → review+
Blocks: 1229820
You need to log in before you can comment on or make changes to this bug.