Closed Bug 1179981 Opened 9 years ago Closed 9 years ago

Robocop harness has too much per-test overhead

Categories

(Firefox for Android Graveyard :: Testing, defect)

defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: gbrown, Assigned: gbrown)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Depends on: 1180215
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
Attachment #8629097 - Attachment is obsolete: true
Attached patch a new test harness for robocop (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48f4af05d7f

:nalexander -- Would you just check that your --serve option still works with this patch?
Attachment #8629529 - Attachment is obsolete: true
Attachment #8630573 - Flags: feedback?(nalexander)
(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.
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.
(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.
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.
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 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.
Attachment #8630573 - Attachment description: wip/candidate -- seems to work, but still testing → a new test harness for robocop
Attachment #8630573 - Flags: review?(jmaher)
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))
Attachment #8630573 - Flags: review?(jmaher) → review-
(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.
Attachment #8630573 - Attachment is obsolete: true
Attachment #8630573 - Flags: feedback?(nalexander)
Attachment #8631659 - Flags: review?(jmaher)
Comment on attachment 8631659 [details] [diff] [review]
new test harness for robocop - updated for review

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

thanks!
Attachment #8631659 - Flags: review?(jmaher) → review+
Comment on attachment 8631659 [details] [diff] [review]
new test harness for robocop - updated for review

Still just concerned about --serve...
Attachment #8631659 - Flags: feedback?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/b5ad4fc404d7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Depends on: 1207461
(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.)
Attachment #8631659 - Flags: feedback?(nalexander)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.