1545234 - Some gtests in gfx/layers/apz/test/gtest are not run on Android

Status: NEW

(Core :: Panning and Zooming, defect, P3)

Description

[Geoff Brown [:gbrown]]

Bug 1318091 started running gtest on our Android 7.0 test platform: gtest runs in the geckoview TestRunnerActivity, against an x86_64 build, running in an emulator running Android 7.0 on packet.net.

Many gtests run fine and pass consistently, but others did not and have been disabled.

gfx/layers/apz/test/gtest tests disabled on Android can be found at:

https://searchfox.org/mozilla-central/rev/d33d470140ce3f9426af523eaa8ecfa83476c806/gfx/layers/apz/test/gtest/moz.build#16

Comment 1

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=10da3cbfed0e64f51fd7faf3de9251b808908a88 shows this assertion failing on debug builds. On release there's a bunch of fling and overscroll tests failing which is presumably related.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Is it possible to run these tests locally? I tried mach gtest "APZ*" on a local android-x86 build and it errored out with Binary expected at /home/kats/zspace/gecko-fennec/obj-android-x86-opt/dist/bin/fennec does not exist.

Comment 3

[Geoff Brown [:gbrown]]

mach gtest "APZ*" should work for android-x86, but support is new -- there are probably some rough edges.

I usually run mach build && mach package && mach android archive-geckoview before running mach gtest, but I'm not sure of the actual dependencies.

By default, mach gtest should run against org.mozilla.geckoview.test, so I am confused by the reference to dist/bin/fennec.

Comment 4

[Geoff Brown [:gbrown]]

I don't think you would get far without it, but perhaps best to check that you have

https://hg.mozilla.org/mozilla-central/rev/98df47a219e2088d431e3961fe6ea7f99017a1be

Comment 5

[Kartikaya Gupta (email:[e-mail])]

I had a copy of m-c from a few days ago which didn't have the commit from comment 4. After updating i'm getting further, but ran into the "no space left on device" problem that bug 1546453 will presumably address (although I'm using a custom AVD at the moment, so maybe it wouldn't happen with a mozbuild AVD). Anyway, thanks for getting me further. I'll increase the AVD size and see what happens.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I increased the AVD size so that the apk seems to get transferred but it still fails to run. In the logcat I see GeckoThread setting stuff up and then XRE_main returns 1 pretty much right away. Not really sure if it's a problem specific to my local set up or a general problem. I'll try digging into it as a background-ish task as I have other stuff to work on at the moment too.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: A log on ./mach gtest APZCSnappingTester.Bug1265510

I am running into the same issue. I can also see the "XRE_main returned 1" in the log I got.

Comment 8

[Geoff Brown [:gbrown]]

Are you trying to run on armv7/aarch64 devices? I realized that won't work and will fail as you describe: bug 1558885. You should have a better experience with an x86_64 build on an emulator.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: ./mach gtest APZ*

I was using an x86_64 build on an emulator. I just tried again and am back to getting the "no space left on device" problem. Log attached.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I force-updated the emulator with ./mach android-emulator --force-update --version x86-7.0 and now running ./mach gtest APZ* works!

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I started investigating some of the disabled gtest failures. The first one, APZCBasicTester.Fling, is failing because the android velocity tracker requires at least two move deltas in order to generate a velocity, but the test's Pan function only generates one move delta. I was able to make this test pass by splitting that move delta into two, each one doing half the distance from aTouchStart to aTouchEnd.

The next failure was for APZCBasicTester.FlingIntoOverscroll - as far as I can tell the my change for the previous patch does actually make the APZC go into overscroll, but the overscroll is eaten by the MockContentController and so the test's IsOverscrolled check always returns false.

I can keep working through these, but I guess the larger question is - how useful are these tests given the divergence between Android and desktop fling implementations? Do you think it's valuable to keep the tests running on both, and if so, would it be ok to make ifdef-variants of some tests to account for the differences?

Comment 12

[Botond Ballo [:botond]]

I think the principled approach here would be:

• For a given test, evaluate whether the STR it's intended to test (looking at the bug it landed with if it's not clear) is applicable to both desktop and android.
• If it's applicable to both desktop and android, try to write the test in such a way that the same steps trigger the scenario on both desktop and android. If we really need to do something different between the two, we can abstract that away in the test infrastructure.
• If it's only applicable to one, restrict the test to only run on the applicable platform.

For example, FlingIntoOverscroll is intended to test the "transform the view when overscrolled" style of overscroll effect (in code terms, GenericOverscrollEffect rather than WidgetOverscrollEffect). It was originally written for B2G, and actually neither desktop nor android use this style of overscroll effect at the moment, but macOS will soon, so we can consider this as being relevant to desktop only and disable it on android.

Do we have a mechanism for selectively enabling tests on android? If so, we could do it incrementally, rather than doing it all in this bug. I would be happy to chip away at it as well as time permits.

Comment 13

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #12)

For example, FlingIntoOverscroll is intended to test the "transform the view when overscrolled" style of overscroll effect (in code terms, GenericOverscrollEffect rather than WidgetOverscrollEffect). It was originally written for B2G, and actually neither desktop nor android use this style of overscroll effect at the moment, but macOS will soon, so we can consider this as being relevant to desktop only and disable it on android.

(It would be even more ideal if the enablement condition would be semantic, that is, "enabled only for platforms which use GenericOverscrollEffect" rather than "enabled only for desktop". That way, if at some point Android starts using GenericOverscrollEffect, the test would automatically be enabled for it. However, I'm happy to consider that aspirational.)

Comment 14

[Kartikaya Gupta (email:[e-mail])]

That makes sense. It's easy enough to disable individual tests via use of #ifdef (or even runtime conditions that early-exit the test). So maybe a first step would be to ifdef out the failing android tests instead of removing entire files of tests. Then we can work on incrementally getting them to pass or add appropriate conditions to skip them.

Comment 15

[Markus Stange [:mstange]]

Attachment: patch to reproduce test failures on Desktop (DO NOT LAND)

Most, if not all, of these test failures reproduce on desktop platforms when using AndroidVelocityTracker and AndroidFlingPhysics.

Comment 16

[Markus Stange [:mstange]]

I'm going to fix the failures that reproduce on macOS with the above patch, in bug 1674279. But there are other failures that do not reproduce with that patch.

Comment 17

[Markus Stange [:mstange]]

Those remaining failures all seem to be related to overscroll. Maybe there's just a pref that's different on Android.