Last Comment Bug 1179981 - Robocop harness has too much per-test overhead
: Robocop harness has too much per-test overhead
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Testing (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: Firefox 42
Assigned To: Geoff Brown [:gbrown]
:
: Geoff Brown [:gbrown]
Mentors:
: 1142112 (view as bug list)
Depends on: 1180215 1207461
Blocks:
  Show dependency treegraph
 
Reported: 2015-07-02 14:52 PDT by Geoff Brown [:gbrown]
Modified: 2016-02-24 08:31 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
wip - does not fully work yet, incomplete profile and log management (45.12 KB, patch)
2015-07-02 14:54 PDT, Geoff Brown [:gbrown]
no flags Details | Diff | Splinter Review
wip - basic functionality working; needs testing (50.47 KB, patch)
2015-07-03 15:55 PDT, Geoff Brown [:gbrown]
no flags Details | Diff | Splinter Review
a new test harness for robocop (61.70 KB, patch)
2015-07-07 10:48 PDT, Geoff Brown [:gbrown]
jmaher: review-
Details | Diff | Splinter Review
log showing some local testing with mach and make (23.06 KB, text/plain)
2015-07-07 10:52 PDT, Geoff Brown [:gbrown]
no flags Details
new test harness for robocop - updated for review (61.75 KB, patch)
2015-07-09 09:29 PDT, Geoff Brown [:gbrown]
jmaher: review+
Details | Diff | Splinter Review

Description User image Geoff Brown [:gbrown] 2015-07-02 14:52:23 PDT
Compared to normal mochitests or reftests for instance, robocop tests have much more per-test overhead for test setup and cleanup. Some of that overhead is in the harness (runtestsremote.py) and some of *that* overhead can be eliminated with some restructuring.

I'm thinking of, for instance:
 - starting and stopping servers (xpcshell, ssltunnel) between each test
 - pushing the profile to device between each test

I want to give robocop its own harness script, runrobocop.py, removing robocop entirely from runtestsremote.py. I think runrobocop.py might still use some functions from runtests.py, but only where runtests provides necessary functionality in a useful, uncomplicated way. I think this will make the logic behind both robocop and android mochitest harnesses more clear and give us the flexibility to manage robocop test life-cycle efficiently.
Comment 1 User image Geoff Brown [:gbrown] 2015-07-02 14:54:55 PDT
Created attachment 8629097 [details] [diff] [review]
wip - does not fully work yet, incomplete profile and log management
Comment 2 User image Geoff Brown [:gbrown] 2015-07-03 15:55:01 PDT
Created attachment 8629529 [details] [diff] [review]
wip - basic functionality working; needs testing

On Android 4.3, seems to reduce run-time by 10 to 15 minutes per job.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee557716f882
Comment 4 User image Geoff Brown [:gbrown] 2015-07-07 10:48:27 PDT
Created attachment 8630573 [details] [diff] [review]
a new test harness for robocop

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48f4af05d7f

:nalexander -- Would you just check that your --serve option still works with this patch?
Comment 5 User image Nick Alexander :nalexander 2015-07-07 10:50:59 PDT
(In reply to Geoff Brown [:gbrown] from comment #0)
> Compared to normal mochitests or reftests for instance, robocop tests have
> much more per-test overhead for test setup and cleanup. Some of that
> overhead is in the harness (runtestsremote.py) and some of *that* overhead
> can be eliminated with some restructuring.
> 
> I'm thinking of, for instance:
>  - starting and stopping servers (xpcshell, ssltunnel) between each test

Yeah, we shouldn't do this.

>  - pushing the profile to device between each test

I think tests expect this, although I'd be /thrilled/ if we could draw a line in the sand and say tests cannot expect a clean profile.

I have wondered about pushing a profile to a temp location, and then over-writing the profile between each test.  Is that what you're doing?

> 
> I want to give robocop its own harness script, runrobocop.py, removing
> robocop entirely from runtestsremote.py. I think runrobocop.py might still
> use some functions from runtests.py, but only where runtests provides
> necessary functionality in a useful, uncomplicated way. I think this will
> make the logic behind both robocop and android mochitest harnesses more
> clear and give us the flexibility to manage robocop test life-cycle
> efficiently.

I would *love* this separation.
Comment 6 User image Geoff Brown [:gbrown] 2015-07-07 10:52:16 PDT
Created attachment 8630574 [details]
log showing some local testing with mach and make

Note the more compact logging when running in mach: Robocop screenshots, device info, and logcat are no longer logged, so you can see your test results without scrolling all over the place.
Comment 7 User image Geoff Brown [:gbrown] 2015-07-07 10:58:09 PDT
(In reply to Nick Alexander :nalexander from comment #5)
> (In reply to Geoff Brown [:gbrown] from comment #0)
> I have wondered about pushing a profile to a temp location, and then
> over-writing the profile between each test.  Is that what you're doing?

Yes...if I can. I create the profile locally, push it to device once, then remotely rm -r/cp -r to quickly create a fresh profile before each test. Unfortunately, cp is not available on older Androids (including the 2.3 image we use in the emulator), so I fallback to pushing each time if cp fails the first time.
Comment 8 User image Geoff Brown [:gbrown] 2015-07-07 12:12:14 PDT
On mozilla-central, Android 2.3 API9 opt rc1 normally takes about 70 minutes to complete; with this patch, that is reduced to about 52 minutes.

On mozilla-central, Android 4.3 API11+ opt rc1 normally takes about 65 minutes to complete; with this patch, that is reduced to about 49 minutes.
Comment 9 User image Geoff Brown [:gbrown] 2015-07-07 15:51:06 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09434ab5e2ca shows that assertion failures are handled correctly: failure reported, logcat and device info dumped, screenshot generated and uploaded to blobber. A hang (no output for 180 seconds) is also demonstrated -- all's well.
Comment 10 User image Geoff Brown [:gbrown] 2015-07-08 14:34:42 PDT
Comment on attachment 8630573 [details] [diff] [review]
a new test harness for robocop

Structurally, this patch removes the robocop-specific code from runtestsremote.py and moves it to a new file, runrobocop.py. RobocopTestRunner still inherits from Mochitest but only uses a few mochitest facilities, like option definitions and profile creation. RobocopTestRunner also continues to launch the browser with automation/remoteautomation's runApp, leveraging all that code for monitoring the remote process, crash reporting, timeout-handling, logcat parsing, etc. I like the result: runrobocop.py seems straight-forward and efficient; runtestsremote.py is simplified.

With this patch, servers are started only once and kept running for the duration of the job, rather than started/stopped with each test. Also, we try to create the profile and push it to device just once, then do a simple remote copy to refresh the profile between tests. (If cp -r fails, the harness falls back to pushing a new profile.) On treeherder, these changes are demonstrated to reduce robocop job times by 10 to 20 minutes per chunk.

I have also tried to "tidy" logging:
 - some repetitive and low-value info messages have been removed
 - logcat and device info is never logged when running via mach, so the tester can see results at a glance without scrolling
 - screenshots are pulled to blobber instead of hex-printed to the log
 - additional debug messages have been added and can be viewed with "--log-mach-level debug"
(See separate attachment for logging examples.)

I started to make changes for what I call "responsive logging" -- the ability to see log messages in near real-time. Historically, "mach robocop" only reports logs after the test finishes; I find this disconcerting and would prefer to see log messages as they are generated. The main problem turns out to be that the harness runs "am" with the -w (wait) flag, and then waits for am to complete -- so the harness is stuck waiting for the process for the duration of the test. This patch would achieve responsive logging for mach robocop if the -w flag was removed, but removing -w exposes a weakness in our process wait which causes some robocop tests to fail intermittently. I am leaving the -w in place and intend to revisit responsive logging in another bug. Related changes in this patch do no harm, and improve logging responsiveness for other scenarios, like running remote mochitests.

I have completed significant testing on try and have no concerns. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48f4af05d7f demonstrates successful tests. https://treeherder.mozilla.org/#/jobs?repo=try&revision=09434ab5e2ca demonstrates failure and hang reporting.
Comment 11 User image Joel Maher ( :jmaher) 2015-07-09 02:43:16 PDT
Comment on attachment 8630573 [details] [diff] [review]
a new test harness for robocop

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

overall, how much profile cleanup/setup is duplicated with the android mochitests?  Same for pulling the logs, checking for crashes, etc.

I don't think we should ensure 100% code reuse, but I would like to at least have comments where we have similar or identical code.

::: testing/mochitest/mach_commands.py
@@ +373,5 @@
>          options.manifestFile = manifest
>  
>          return runtestsremote.run_test_harness(options)
>  
> +    def run_robocop_test(self, context, tests, suite=None, **kwargs):

how does this get robocop.apk?  Maybe it is automagic inside of mach.

::: testing/mochitest/runrobocop.py
@@ +11,5 @@
> +
> +sys.path.insert(
> +    0, os.path.abspath(
> +        os.path.realpath(
> +            os.path.dirname(__file__))))

I really don't like sys.path.insert, sadly we still use it in our other harnesses.

@@ +23,5 @@
> +from manifestparser.filters import chunk_by_slice
> +import devicemanager
> +import mozinfo
> +
> +SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(__file__)))

do you use this anywhere? I suspect this is used by methods imported from other modules.

@@ +70,5 @@
> +        else:
> +            self.auto.setProduct(self.options.remoteProductName)
> +        self.auto.setAppName(self.options.remoteappname)
> +        self.certdbNew = True
> +        self.cpOK = True

could we make this a more descriptive variable?  I don't know what cpOK stands for.

@@ +101,5 @@
> +        self.log.info(
> +            "Android sdk version '%s'; will use this to filter manifests" %
> +            str(androidVersion))
> +        mozinfo.info['android_version'] = androidVersion
> +        if (self.options.dm_trans == 'adb' and self.options.robocopApk):

can we assume self.options.robocopApk?  without it, I am not sure we can test

@@ +136,5 @@
> +            os.system("rm -Rf %s" % self.localProfile)
> +        self.dm.removeDir(self.remoteProfile)
> +        self.dm.removeDir(self.remoteProfileCopy)
> +        self.dm.removeDir(self.remoteScreenshots)
> +        self.dm.removeFile(self.remoteConfigFile)

do we need to remove self.remoteNSPR?

@@ +241,5 @@
> +            os.path.join(
> +                self.localProfile,
> +                'extensions',
> +                'staged',
> +                'workerbootstrap-test@mozilla.org'))

it would be nice to do:
desktop_extensions = ['mochikit@mozilla.org', 'worker-test@mozilla.org', 'workerbootstrap-test@mozilla.org']
for ext in desktop_extensions:
    shutil.rmtree(os.path.join(self.localProfile, 'extensions', 'staged', ext))
Comment 12 User image Geoff Brown [:gbrown] 2015-07-09 09:27:42 PDT
(In reply to Joel Maher (:jmaher) from comment #11)
> Comment on attachment 8630573 [details] [diff] [review]
> a new test harness for robocop
> 
> Review of attachment 8630573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall, how much profile cleanup/setup is duplicated with the android
> mochitests?  Same for pulling the logs, checking for crashes, etc.
> 
> I don't think we should ensure 100% code reuse, but I would like to at least
> have comments where we have similar or identical code.

There are some similar code blocks in runtestsremote.py, but not many. (And similar code in remotereftests.py.) I suppose remoteautomation.py is the natural place for some of this code, but we also talk about removing automation, so I am reluctant to add to it.

I have added some comments "This is similar to XXX in runtestsremote.py."

> > +    def run_robocop_test(self, context, tests, suite=None, **kwargs):
> 
> how does this get robocop.apk?  Maybe it is automagic inside of mach.

> > +        if (self.options.dm_trans == 'adb' and self.options.robocopApk):
> 
> can we assume self.options.robocopApk?  without it, I am not sure we can test

robocop.apk is built alongside the fennec apk. In automated testing, mozharness installs robocop.apk along with Firefox. mach automatically sets the robocopApk option. Also, there are existing checks in mochitest_options for robocopApk: http://hg.mozilla.org/mozilla-central/annotate/f34a7120f46b/testing/mochitest/mochitest_options.py#l1122

> ::: testing/mochitest/runrobocop.py
> @@ +11,5 @@
> > +
> > +sys.path.insert(
> > +    0, os.path.abspath(
> > +        os.path.realpath(
> > +            os.path.dirname(__file__))))
> 
> I really don't like sys.path.insert, sadly we still use it in our other
> harnesses.

Exactly. I find I don't need it, but I worry about breakage for others, and for the sake of consistency with other harnesses, I would prefer to leave it in.

> > +SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(__file__)))
> 
> do you use this anywhere? I suspect this is used by methods imported from
> other modules.

It's used by MochitestServer -- http://hg.mozilla.org/mozilla-central/file/f34a7120f46b/testing/mochitest/runtests.py#l401.
 
> > +        self.cpOK = True
> 
> could we make this a more descriptive variable?  I don't know what cpOK
> stands for.

Not my best variable name, for sure! This is the flag that is set to false if "cp -r" fails on device.

s/cpOK/remoteCopyAvailable/

> @@ +136,5 @@
> > +            os.system("rm -Rf %s" % self.localProfile)
> > +        self.dm.removeDir(self.remoteProfile)
> > +        self.dm.removeDir(self.remoteProfileCopy)
> > +        self.dm.removeDir(self.remoteScreenshots)
> > +        self.dm.removeFile(self.remoteConfigFile)
> 
> do we need to remove self.remoteNSPR?

That's a good idea - done.
 
> it would be nice to do:
> desktop_extensions = ['mochikit@mozilla.org', 'worker-test@mozilla.org',
> 'workerbootstrap-test@mozilla.org']
> for ext in desktop_extensions:
>     shutil.rmtree(os.path.join(self.localProfile, 'extensions', 'staged',
> ext))

Yes, that's tidier! Done.
Comment 13 User image Geoff Brown [:gbrown] 2015-07-09 09:29:50 PDT
Created attachment 8631659 [details] [diff] [review]
new test harness for robocop - updated for review
Comment 14 User image Joel Maher ( :jmaher) 2015-07-09 09:34:40 PDT
Comment on attachment 8631659 [details] [diff] [review]
new test harness for robocop - updated for review

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

thanks!
Comment 15 User image Geoff Brown [:gbrown] 2015-07-09 11:12:33 PDT
Comment on attachment 8631659 [details] [diff] [review]
new test harness for robocop - updated for review

Still just concerned about --serve...
Comment 17 User image Wes Kocher (:KWierso) 2015-07-09 18:03:32 PDT
https://hg.mozilla.org/mozilla-central/rev/b5ad4fc404d7
Comment 18 User image Nick Alexander :nalexander 2015-10-19 14:24:29 PDT
(In reply to Geoff Brown [:gbrown] from comment #15)
> Comment on attachment 8631659 [details] [diff] [review]
> new test harness for robocop - updated for review
> 
> Still just concerned about --serve...

I'll file something if I ever bring those --serve patches back.  (Or did the land?  I can't remember.  I don't do much Robocop testing locally.)
Comment 19 User image Geoff Brown [:gbrown] 2016-02-24 08:31:40 PST
*** Bug 1142112 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.