1190567 - Aggressive ProGuard rules strip out RecyclerView.getAdapter

Status: NEW

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Jonathan Almeida [:jonalmeida]]

When updating some ListViews with RecyclerViews, I see robocop runtime errors about missing getAdapter methods. This hasn't been seen to be a problem with List/TwoWay views.

Error trace:

Exception caught during test! - java.lang.NoSuchMethodError: No virtual method getAdapter()Landroid/support/v7/widget/RecyclerView$Adapter; in class Lorg/mozilla/gecko/home/SearchEngineBar; or its super classes (declaration of 'org.mozilla.gecko.home.SearchEngineBar' appears in /data/app/org.mozilla.fennec_jalmeida-1/base.apk)
    at org.mozilla.gecko.tests.testAddSearchEngine$1.isSatisfied(testAddSearchEngine.java:160)
    at com.jayway.android.robotium.solo.Waiter.waitForCondition(Waiter.java:370)
    at com.jayway.android.robotium.solo.Solo.waitForCondition(Solo.java:426)
    at org.mozilla.gecko.tests.BaseTest.waitForCondition(BaseTest.java:295)
    at org.mozilla.gecko.tests.testAddSearchEngine.verifyDisplayedSearchEnginesCount(testAddSearchEngine.java:149)
    at org.mozilla.gecko.tests.testAddSearchEngine.testAddSearchEngine(testAddSearchEngine.java:68)
    at java.lang.reflect.Method.invoke(Native Method)
    at java.lang.reflect.Method.invoke(Method.java:372)
    at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
    at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
    at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
    at org.mozilla.gecko.tests.BaseTest.runTest(BaseTest.java:133)
    at junit.framework.TestCase.runBare(TestCase.java:134)
    at junit.framework.TestResult$1.protect(TestResult.java:115)
    at junit.framework.TestResult.runProtected(TestResult.java:133)
    at junit.framework.TestResult.run(TestResult.java:118)
    at junit.framework.TestCase.run(TestCase.java:124)
    at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
    at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
    at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555)
    at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1853)

Comment 1

[Richard Newman [:rnewman]]

See m/a/b/config/proguard/adjust-keeps.cfg for one way to solve this problem.

We can't apply the usual solution of adding @RobocopTarget here, because we don't own RecyclerView.

Comment 2

[Jonathan Almeida [:jonalmeida]]

Actually, that was my current fix for bug 1184211:

> +    @RobocopTarget
> +    @Override
> +    public SearchEngineAdapter getAdapter() {
> +        return mAdapter;
> +    }

Not sure if this would have been the best way to do so though.

Comment 3

[Nick Alexander :nalexander [he/him]]

This is just hard.  The issue is that we're running JUnit 3 tests against a Proguarded APK: see Bug 1093218 for some tales of woe.  For now, config/proguard and a new -keeps.cfg file to hack it in.

Comment 4

[Chris Kitching [:ckitching]]

(In reply to Nick Alexander :nalexander from comment #3)
> This is just hard.  The issue is that we're running JUnit 3 tests against a
> Proguarded APK: see Bug 1093218 for some tales of woe.  For now,
> config/proguard and a new -keeps.cfg file to hack it in.

Why not add the testing code to the proguard invocation as well? (with suitable general keep rules to keep the test entry points).
That way, you'd get an optimised-but-consistent-under-testing output that could run the tests. You could then run Proguard again without the test classes to get a further optimised "release" build. You would stop being able to detect failures due to obscure Proguard bugs in CI, but that's probably not something sane to be worrying about.

Wasn't this approach used for robocop tests at one point, or did that never get finished?

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Chris Kitching [:ckitching] from comment #4)
> (In reply to Nick Alexander :nalexander from comment #3)
> > This is just hard.  The issue is that we're running JUnit 3 tests against a
> > Proguarded APK: see Bug 1093218 for some tales of woe.  For now,
> > config/proguard and a new -keeps.cfg file to hack it in.
> 
> Why not add the testing code to the proguard invocation as well? (with
> suitable general keep rules to keep the test entry points).

We could do this, at the cost of bloating the APK.  It might be nice to get a handle on that bloat.

> That way, you'd get an optimised-but-consistent-under-testing output that
> could run the tests. You could then run Proguard again without the test
> classes to get a further optimised "release" build. You would stop being
> able to detect failures due to obscure Proguard bugs in CI, but that's
> probably not something sane to be worrying about.

I don't think we've ever witnessed a PG bug.  (We've witnessed lots of bugs due to the process of PG, but not PG bugs leading to Fennec bugs.)

This is a philosophical thing, I think: we only build and ship one thing.  There's a small releng detail here: the APKs pushed to device are hard-coded.  I think I discovered how to make this more general once, but I remember it being a pain point when I was trying to do Bug 1093218.  Eventually I think we have to do this in order to write meaningful instrumentation tests.

> Wasn't this approach used for robocop tests at one point, or did that never
> get finished?

Not that I recall.