1482893 - Enable unstructured logging before calling adb in reftests

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Bob Clary [:bc] (inactive)]

adb*.py uses unstructured logging and if its logger is called before mach adds the ConvertToStructuredFilter, it will result in 'LogRecord' object has no attribute 'params' errors. We must make certain that we add the filter before adb*.py's logger is called.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: enable_unstructured_logging_before_calling_adb.patch

try coming soon.

Comment 2

[Bob Clary [:bc] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded&selectedJob=193668351

lint error with too long line. Will fix.

Comment 3

[Geoff Brown [:gbrown]]

Comment on attachment 8999620 [details] [diff] [review]
enable_unstructured_logging_before_calling_adb.patch

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

I don't see how this goes wrong, but it doesn't work for me:

$ ./mach reftest layout/reftests/reftest-sanity
No Android devices connected. Start an emulator? (Y/n) 
Starting emulator running Android 7.0 x86...
accel:
0
KVM (version 12) is installed and usable.
accel
adbd restarted as root
 0:16.36 adbd restarted as root
su 0 supported
 0:16.56 su 0 supported
/system/bin/ls -1A supported
 0:16.77 /system/bin/ls -1A supported
Native cp support: True
 0:16.87 Native cp support: True
Native chmod -R support: True
 0:16.97 Native chmod -R support: True
Running the x86 emulator; be sure to install an x86 APK!
Granting important runtime permissions to org.mozilla.fennec_gbrown
adbd running as root
 0:18.84 adbd running as root
su 0 supported
 0:19.05 su 0 supported
/system/bin/ls -1A supported
 0:19.26 /system/bin/ls -1A supported
Native cp support: True
 0:19.36 Native cp support: True
Native chmod -R support: True
 0:19.46 Native chmod -R support: True
usage: mach [-h] [--xre-path XREPATH] [--symbols-path SYMBOLSPATH]
            [--debugger DEBUGGER] [--debugger-args DEBUGGERARGS]
            [--debugger-interactive] [--appname APP]
            [--extra-profile-file EXTRAPROFILEFILES] [--timeout TIMEOUT]
            [--leak-threshold DEFAULTLEAKTHRESHOLD]
            [--utility-path UTILITYPATH] [--total-chunks TOTALCHUNKS]
            [--this-chunk THISCHUNK] [--log-file LOGFILE] [--skip-slow-tests]
            [--ignore-window-size] [--install-extension EXTENSIONSTOINSTALL]
            [--marionette MARIONETTE] [--setenv NAME=VALUE] [--filter FILTER]
            [--shuffle] [--run-until-failure] [--repeat REPEAT]
            [--focus-filter-mode FOCUSFILTERMODE] [--disable-e10s]
            [--setpref PREF=VALUE]
            [--reftest-extension-path REFTESTEXTENSIONPATH]
            [--special-powers-extension-path SPECIALPOWERSEXTENSIONPATH]
            [--cleanup-crashes] [--max-retries MAXRETRIES]
            [--sandbox-read-whitelist SANDBOXREADWHITELIST] [--verify]
            [--verify-max-time VERIFY_MAX_TIME] [--log-unittest LOG_UNITTEST]
            [--log-raw LOG_RAW] [--log-html LOG_HTML] [--log-tbpl LOG_TBPL]
            [--log-xunit LOG_XUNIT] [--log-mach LOG_MACH] [--log-tbpl-compact]
            [--log-mach-buffer LOG_MACH_BUFFER]
            [--log-tbpl-buffer LOG_TBPL_BUFFER] [--log-mach-verbose]
            [--log-mach-level LOG_MACH_LEVEL] [--log-raw-level LOG_RAW_LEVEL]
            [--log-tbpl-level LOG_TBPL_LEVEL] [--adbpath ADB_PATH]
            [--deviceSerial DEVICESERIAL] [--remote-webserver REMOTEWEBSERVER]
            [--http-port HTTPPORT] [--ssl-port SSLPORT]
            [--remoteTestRoot REMOTETESTROOT] [--httpd-path HTTPDPATH]
            [--no-device-info]
            [TEST_PATH [TEST_PATH ...]]
mach: error: ERROR: You must specify the path to the controller xre directory

Comment 4

[Bob Clary [:bc] (inactive)]

Wow, this is weird. I qpopped to the patch previous to this one in my queue (RECORD_AUDIO.path), did a clobber and build then ran the sanity test

./mach reftest layout/reftests/reftest-sanity

for my p2 without setting MOZ_HOST_BIN and it worked. I let it run to completion, then did a plain vanilla

./mach reftest

and it started fine.

Then I turned off my p2 and did the same. It prompted for the emulator but I didn't get the x86 version...

bclary@ruby ~/mozilla/builds/inbound-taskcluster/mozilla (sisyphus-dev)
$ ./mach reftest layout/reftests/reftest-sanity
No Android devices connected. Start an emulator? (Y/n) y
Starting emulator running Android 7.0...
Running the arm emulator; be sure to install an arm APK!
Granting important runtime permissions to org.mozilla.fennec_bclary
Using adb 1.0.40

I think the clobber is insufficient. find . -name '*.pyc' shows tons of files in the source tree (not the objdir). I'll clobber+build again and this time will rm all of the *.pyc files.

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #4)
> It prompted for the emulator but I
> didn't get the x86 version...

mach checks your build context and runs the associated emulator: If 'mach build' would build for arm, it will start an arm emulator.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: enable_unstructured_logging_before_calling_adb.patch

You'll love this one. I ran locally with fennec and firefox but I'll do a try with desktop linux64 as well with android.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d05605fb3df2f6d8a83db858643f14b14407b84b&filter-tier=1&filter-tier=2&filter-tier=3

Comment 7

[Geoff Brown [:gbrown]]

Comment on attachment 9000075 [details] [diff] [review]
enable_unstructured_logging_before_calling_adb.patch

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

::: layout/tools/reftest/mach_commands.py
@@ +109,5 @@
>              "crashtest": (self.topsrcdir, "testing", "crashtest", "crashtests.list"),
>              "jstestbrowser": ("jsreftest", "tests", "jstests.list")
>          }
>  
> +        if args.suite != 'jstestbrowser' and not args.tests:

What's this about? I find I cannot run 'mach jstestbrowser' with or without your patch.

Comment 8

[Bob Clary [:bc] (inactive)]

Thanks for catching that. It was misplaced when I manually edited the file after deleting the previous version of the patch. It should have been in the def _setup_objdir the way it was in the original version. Fixed locally.

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: enable_unstructured_logging_before_calling_adb.patch

This is the version with the correct location of jstestbrowser check in _setup_objdir. carrying forward r+.

Comment 10

[Bob Clary [:bc] (inactive)]

This is the latest try run for a vanilla checkout and one with the patches applied:

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded&fromchange=071dbb8c5c874e73b3314c9efce60566fb0d13a6

I did this to see if the exceptions in mochitest on Android hardware were related to my patches which they do not appear to be. One thing that seems to have changed is the *reduced* but not eliminated ADBTimeoutErrors in the android emulator tests.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13ee04709ba
Enable unstructured logging before calling adb in reftests, r=gbrown.

Comment 12

[Dorel Luca [:dluca]]

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