Closed Bug 1360353 Opened 3 years ago Closed 3 years ago

Autophone - add GeckoView perf test

Categories

(Testing :: Autophone, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bc, Assigned: bc)

References

Details

Attachments

(6 files)

gbrown did all the leg work. The details of how the current GeckoView smoke test runs are in the log:

https://public-artifacts.taskcluster.net/BIvKjA_eRS61hKvP7pEy5A/0/public/logs/live_backing.log
Depends on: 1362985
I've been looking into this and the differences between the GeckoView example app and Fennec mean there are multiple places where this is going to require changes. All the way from detecting and downloading builds, installations, launching, checking processes, killing processes, etc, etc. This is my #1 task at the moment but it make take a bit of time since it is not nearly as uncomplicated as I thought. Follow along here or the dependencies for news.
I have a work in progress patch which essentially treats geckoview_example.apk as a parallel file to the target.apk. I had to rewrite utils.urlretrieve to use requests so that I can reliably detect 404s and relax the requirement in autophonepulsemonitor.py and builds.py that the taskcluster builds be tier 1, but that successfully gets the geckoview_example.apk in addition to the fennec build apk.

The problem is that it would essentially treat the corresponding gradle fennec build as a first class build and I would need to jump through some hoops to make sure the other tests ignored it. Otherwise we would test the same revision with both the tier 1 fennec build and the tier 2 gradle build. That isn't a big deal but is a bit of a complication and special case in the code.

The other choice is to treat the geckoview_example.apk as independent of the fennec build and save the url to the geckoview_example.apk in the jobs database . That would mean we could generalize the dispatching of these types of special testing apks independently of the fennec build. This is probably the right choice. I tried something similar at first but ran into some issues, but I think I understand the situation better now and can implement this without too much trouble.

I have to go out during the middle of the day today but will work on resolving this issue this afternoon.

After we get the geckoview_example build situation resolved, my first step will to add the ability to run rungeckoview.py as a unit test. After that, I'll add a perf test.
Progress Update: Successfully downloading builds from pulse notifications and running the geckoview mochitest. Still need to make sure to handle local builds properly and to implement the perftest.
Progress Update: Trigger is working for fennec and geckoview_example at least when specifying revision ranges where we get fennec android-api-15 but not android-api-15-gradle and we get geckoview_example for android-api-15-gradle but not android-api-15. I also have geckoview versions of the S1S2 tests for remote nytimes, twitter and blank though there are outstanding problems.

1. Extensions aren't supported so quitter does not work to automatically shutdown after a page is loaded. I have to manually kill the geckoview_example app. Setting the times to kill low enough to make the test run quickly enough is tricky but can be done though the total run time *will* be longer than the normal fennec tests.

2. geckoview_example emits page load start and page load stop messages to logcat for about:blank prior to the messages for the pages which are actually loaded. I'll have to rework the analyze_logcat logic to detect when to ignore page load start|stop so I don't get confused by about:blank.

3. I still need to test other trigger options such as build urls, local builds, etc.
When we inspect the test scripts to create the test classes in Autophone, we previously filtered the returned classes and eliminated PhoneTest and PerfTest explicitly. This worked but was not general enough as I found out when I created an S1S2 based geckoview_example test by subclassing S1S2Test. This patch generalizes the filter by only creating tests whose script source file matches the appropriate test section in the test manifest. Without this patch, we would have created a S1S2GeckoViewTest test but also its parent S1S2Test for the same test section in the manifest which would fail due to a duplicate key condition.
Attachment #8868125 - Flags: review?(jmaher)
I've done several try runs reporting to staging:

https://treeherder.allizom.org/#/jobs?repo=try&author=bclary@mozilla.com&exclusion_profile=false&group_state=expanded&fromchange=8988a79c21af2071cc486d7c83383f7c8aeb8aa4

The symbol is A(tg).

http://phonedash-dev.allizom.org/#/2017-05-14/2017-05-16/binning=repo-phonetype-phoneid-test_name-cached_label-metric&rejected=norejected&errorbars=noerrorbars&errorbartype=standarderror&valuetype=median&geckoview-nytimes=on&remote-twitter=on&throbberstart=on&throbberstop=on&first=on&try-bclary=on&nexus-4=on&nexus-4-13=on

Note that s1s2geckoview throbber start is slower than s1s2 though the total throbber time is shorter. Perhaps due to the lack of support for quitter and the clean creation of the profile and clean shutdown of the app.

Note the try syntax:

try: -b o -p android-api-15-gradle -u autophone-s1s2geckoview -t none --artifact 

We'll have to update try chooser to support not only autophone-s1s2geckoview but android-api-15-gradle as well since the android-api-15-gradle-dependencies is insufficient to create the geckoview_example app.

gbrown's geckoview unittest is invoked via mochitest-geckoview which I unfortunately left out of the public try runs.
Attachment #8868136 - Flags: review?(jmaher)
PS. I've tested this with Pulse and with trigger_runs with a build url pointing to the geckoview_example.apk on taskcluster. This will not work with a local build as of yet. I haven't been able to spend the time necessary to produce one so that will have to be a follow up.
Comment on attachment 8868125 [details] [diff] [review]
bug-1360353-class-hierarchy-filter.patch

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

seems a little hacky, but the comment is great!
Attachment #8868125 - Flags: review?(jmaher) → review+
Comment on attachment 8868136 [details] [diff] [review]
bug-1360353-s1s2geckoview-etc-v1.patch

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

only a few nits- a lot of changes!

::: autophone.py
@@ +1389,5 @@
> +                      default=['android-api-15'],
> +                      help='The build types to test. '
> +                      'One of opt or debug. To specify multiple build types, '
> +                      'specify them with additional --buildtype options. '
> +                      'Defaults to opt.')

as this is append, will --platforms always include android-api-15?  is that desirable?

::: builds.py
@@ +502,3 @@
>                  if platform in self.build_platforms and \
>                     build_type in self.buildtypes and \
> +                   (builder_type == 'buildbot' or tier >= 1):

can we say tier <=2 ?  if not, maybe just drop the tier clause?
Attachment #8868136 - Flags: review?(jmaher) → review+
(In reply to Joel Maher ( :jmaher) from comment #9)
> Comment on attachment 8868136 [details] [diff] [review]
> bug-1360353-s1s2geckoview-etc-v1.patch
> 
> Review of attachment 8868136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> only a few nits- a lot of changes!
> 
> ::: autophone.py
> @@ +1389,5 @@
> > +                      default=['android-api-15'],
> > +                      help='The build types to test. '
> > +                      'One of opt or debug. To specify multiple build types, '
> > +                      'specify them with additional --buildtype options. '
> > +                      'Defaults to opt.')
> 
> as this is append, will --platforms always include android-api-15?  is that
> desirable?
> 

Not necessarily. I remember thinking about this when I first did it and not being entirely happy. We could default to an empty list and just require at least one platform to be explicitly set. And I should fix the help text while I'm here too!

> ::: builds.py
> @@ +502,3 @@
> >                  if platform in self.build_platforms and \
> >                     build_type in self.buildtypes and \
> > +                   (builder_type == 'buildbot' or tier >= 1):
> 
> can we say tier <=2 ?  if not, maybe just drop the tier clause?

I think we can drop the entire parenthetical.
jmaher: I made a few tweaks based on your feedback. I also updated USAGE.md to match the new arguments. I'll fold this into the original patch if it is ok with you.

The latest try run on staging:

https://treeherder.allizom.org/#/jobs?repo=try&author=bclary@mozilla.com&exclusion_profile=false&group_state=expanded&fromchange=31d2e1595dae904944c0276ee3fef0698c3bf228&tochange=f91183fe066f496b6d6e3eb66aaf508e8e3e9e69

The geckoview unit test is now failing though I'm fairly sure it was passing initially. gbrown: thoughts?
Flags: needinfo?(gbrown)
Attachment #8868340 - Flags: feedback?(jmaher)
The emulator unit test seems to be broken in the same way - "No crash directory..." - but fails to turn the job orange. Sigh.

Maybe rungeckoview is not waiting long enough for startup? I can look tomorrow.
Follow up to allow both e10s and no e10s tests.
See Also: → 1365636
Comment on attachment 8868340 [details] [diff] [review]
bug-1360353-followup.patch

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

::: builds.py
@@ -491,4 @@
>                  task_definition = self.queue.task(task_id)
>                  logger.debug('_find_latest_task_ids: task_definition: %s', task_definition)
>                  worker_type = task_definition['workerType']
> -                builder_type = 'buildbot' if worker_type == 'buildbot' else 'taskcluster'

do we have any remaining buildbot android builders?
Attachment #8868340 - Flags: feedback?(jmaher) → feedback+
gbrown: The other error I see in the Mg test is:
TEST-UNEXPECTED-FAIL | startup logcat messages | 'Displayed org.mozilla.geckoview_example/.GeckoViewActivity' not found in logcat

though I do see other geckoview related messages in the logcat.

jmaher: I don't think so.
(In reply to Bob Clary [:bc:] from comment #15)
> gbrown: The other error I see in the Mg test is:
> TEST-UNEXPECTED-FAIL | startup logcat messages | 'Displayed
> org.mozilla.geckoview_example/.GeckoViewActivity' not found in logcat

Yes, I see that. That seems to be specific to certain phones or Android versions. Not sure how to handle that yet...might just stop watching for that message.
Ok, I've got a follow up to support e10s. Try run here:

https://treeherder.allizom.org/#/jobs?repo=try&revision=6ab934757086c15cdb65859351d311047cf7dda5&filter-searchStr=android&filter-tier=1&filter-tier=2&filter-tier=3&exclusion_profile=false&group_state=expanded

s1s2-geckoview supports both e10s and non-e10s but the mochitest-geckoview only supports e10s since it uses the default.

I'll package up the changes for additional review once this completes.
Attachment #8869122 - Flags: review?(jmaher)
Comment on attachment 8869122 [details] [diff] [review]
bug-1360353-e10s-v1.patch

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

e10s ftw!
Attachment #8869122 - Flags: review?(jmaher) → review+
s1s2geckoviewtest.py is missing an import ConfigParser in the e10s patch. Fixed locally. Retesting.
and of course I forgot to specify the now required platforms in production.
https://github.com/mozilla/autophone/commit/0cefeebee9810424b213a635b39ac8ed5b80eedc
(In reply to Bob Clary [:bc:] from comment #11)
> The geckoview unit test is now failing though I'm fairly sure it was passing
> initially. gbrown: thoughts?

I'm pretty sure bug 1365636 will resolve this - :droeh is following up there.
Flags: needinfo?(gbrown)
You need to log in before you can comment on or make changes to this bug.