1476165 - Update Android Gradle plugin to version 3.1.0

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Gabriel Vîjială [:gabriel-v]]

Because of an issue with the Android Gradle plugin version <3.1.0 ( ), JaCoCo offline instrumentation does not work with emulator tests.

The Android Gradle plugin version 3.1.0 fixes that problem.
It requires Gradle version 4.4 and Andoid Build Tools version 27.0.3 (as described here: https://developer.android.com/studio/releases/gradle-plugin#3-1-0).

Comment 1

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

I expect this to Just Work, although :tvijiala suggests it doesn't in https://bugzilla.mozilla.org/show_bug.cgi?id=1473313#c9.

Sucks that this requires bumping the Android Build tools as well, but c'est la vie -- we should be more aggressive updating if we can be.

Comment 2

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1476165 - Update Android Gradle plugin to version 3.1.0.

Review commit: https://reviewboard.mozilla.org/r/257434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257434/

Comment 3

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.

Review commit: https://reviewboard.mozilla.org/r/257484/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/257484/

Comment 4

[Gabriel Vîjială [:gabriel-v]]

It does Just Work, I was doing something stupid.

After upgrading these packages, A(test) is failing 3 tests, all with NullPointerException.

failing try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0895341d8ab95a77052884b96f9969b031bd37f&selectedJob=188571112

test report: https://taskcluster-artifacts.net/L_kYp8UoQBi0adr3WZ4iEg/0/public/app/unittest/testOfficialWithoutGeckoBinariesNoMinApiPhotonDebugUnitTest/index.html

Note: A(test-ccov) is also failing those 3 tests, but it comes out green because we ignore the test outcome and just collect the code coverage.

Comment 5

[Gabriel Vîjială [:gabriel-v]]

All the above tests fail with NPE when hashCode() gets called on a android.content.ComponentName that was constructed from an android.content.Context mocked with Mockito. This is because the mocked Context contains some null fields.

In one of the tests [1], I managed to pass in a valid ComponentName (created with the ComponentName(String packageName, String className) constructor), but that test is still failing, as the mocked Context will return null when 'context.getSystemService(Context.POWER_SERVICE)' is called on it.

I tried:

- updating Mockito 1.10.19 --> 2.19.9 and temporarily removed conflicting tests that wouldn't compile: problem persists.
- updating the support version library 26.1.10 --> 27.0.0, but I could not get the code to compile because of deprecated APIs being used.
- updating Roboelectric 3.8 --> 4.0-alpha-2: problem persists.

I also tried updating all the other test dependencies I could find to a more current version. Espresso 2.2.2 --> 3.0.0 and android test runner 0.5 --> 1.0 can be upgraded with no changes, but the problem persists. I went over the change logs for the android gradle plugin and build tools, they report nothing related to this change.

[1] org.mozilla.gecko.telemetry.schedulers.TestTelemetryUploadAllPingsImmediatelyScheduler#testScheduleUpload

Any idea on how to further debug this? Or can I just @Ignore those 3 tests for now?

Comment 6

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

I pulled your commit and rebased onto central; everything passes locally.  I see recent work, not included in your try push, in this area in https://bugzilla.mozilla.org/show_bug.cgi?id=1476237.  I've pushed your work, rebased, to https://treeherder.mozilla.org/#/jobs?repo=try&revision=c269ea026f0030b6343318cb7ea2e3b883973a17.  Let's see how that fares.

In general, it looks like we need to mock system services, and maybe we need to mock more than we are right now.  But let's hope not.

Comment 7

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8992609 [details]
Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257484/diff/1-2/

Comment 8

[Gabriel Vîjială [:gabriel-v]]

That's right, the tests are passing now.

The new plugin version adds some linting rules that are failing. They are small changes, so I'll add them to this bug as separate commits (one per error type).

Clearing the r? flag until I get everything working again.

Comment 9

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1476165 - Part 1a: Fix Android Lint - ResourceCycle: Cycle in resource definitions.

Review commit: https://reviewboard.mozilla.org/r/258786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258786/

Comment 10

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1476165 - Part 1b: Fix Android Lint - ResourceType: Wrong Resource Type.

Review commit: https://reviewboard.mozilla.org/r/258788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258788/

Comment 11

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1476165 - Part 1c: Fix Android Lint - TextFields: Missing inputType.

Review commit: https://reviewboard.mozilla.org/r/258790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258790/

Comment 12

[Gabriel Vîjială [:gabriel-v]]

Attachment: Bug 1476165 - Part 1d: Ignore ProtectedPermissions in Android Lint.

Review commit: https://reviewboard.mozilla.org/r/258792/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258792/

Comment 13

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8992609 [details]
Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257484/diff/2-3/

Comment 14

[Gabriel Vîjială [:gabriel-v]]

Here's a try build with android-lint passing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22abae41703dc9bb1de2942d1d6beb3261c47ad6

Still waiting for all the test suites to finish running.

Comment 15

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8994184 [details]
Bug 1476165 - Part 1a: Fix Android Lint - ResourceCycle: Cycle in resource definitions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258786/diff/1-2/

Comment 16

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8994185 [details]
Bug 1476165 - Part 1b: Fix Android Lint - ResourceType: Wrong Resource Type.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258788/diff/1-2/

Comment 17

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8994186 [details]
Bug 1476165 - Part 1c: Fix Android Lint - TextFields: Missing inputType.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258790/diff/1-2/

Comment 18

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8994187 [details]
Bug 1476165 - Part 1d: Ignore ProtectedPermissions in Android Lint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258792/diff/1-2/

Comment 19

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8992609 [details]
Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/257484/diff/3-4/

Comment 20

[Gabriel Vîjială [:gabriel-v]]

The try push linked above is green.

As context, here is the list of lint failures that the patches address: https://taskcluster-artifacts.net/fdGadniRTmWOsT0suBrs8Q/0/public/android/lint/lint-results-officialWithoutGeckoBinariesNoMinApiPhotonDebug.html

Comment 21

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8992609 [details]
Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.

https://reviewboard.mozilla.org/r/257484/#review265748

This looks great!  I'm going to flag snorp for an opinion on bumping GV to 27.0.x; I doubt it'll be a problem for them, but good to check.

::: taskcluster/scripts/misc/android-gradle-dependencies/after.sh:8
(Diff revision 4)
>  set -x -e
>  
>  echo "running as" $(id)
>  
>  : WORKSPACE ${WORKSPACE:=/builds/worker/workspace}
> -: GRADLE_VERSION ${GRADLE_VERSION:=4.1}
> +: GRADLE_VERSION ${GRADLE_VERSION:=4.4}

Huh.  We should really pull this from the in tree Gradle wrapper file, but that's definitely for another day.

Comment 22

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8994184 [details]
Bug 1476165 - Part 1a: Fix Android Lint - ResourceCycle: Cycle in resource definitions.

https://reviewboard.mozilla.org/r/258786/#review265752

I once wondered why these didn't have @.  I think this is correct.

Comment 23

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8994185 [details]
Bug 1476165 - Part 1b: Fix Android Lint - ResourceType: Wrong Resource Type.

https://reviewboard.mozilla.org/r/258788/#review265754

Hmm.  Is it true that resource IDs are always positive, and can never overflow and be negative?  Let's ship it; we can back out if we see problems in the wild.

Comment 24

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8994186 [details]
Bug 1476165 - Part 1c: Fix Android Lint - TextFields: Missing inputType.

https://reviewboard.mozilla.org/r/258790/#review265758

Sure.

Comment 25

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8994187 [details]
Bug 1476165 - Part 1d: Ignore ProtectedPermissions in Android Lint.

https://reviewboard.mozilla.org/r/258792/#review265760

Comment 26

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8994187 [details]
Bug 1476165 - Part 1d: Ignore ProtectedPermissions in Android Lint.

https://reviewboard.mozilla.org/r/258792/#review265762

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8992609 [details]
Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.

https://reviewboard.mozilla.org/r/257484/#review266202

Comment 28

[Gabriel Vîjială [:gabriel-v]]

Comment on attachment 8992609 [details]
Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.

https://reviewboard.mozilla.org/r/257484/#review265748

> Huh.  We should really pull this from the in tree Gradle wrapper file, but that's definitely for another day.

Filed as a follow-up bug.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6be5d9b0ac9d
Part 1a: Fix Android Lint - ResourceCycle: Cycle in resource definitions. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/dd0c6edcb031
Part 1b: Fix Android Lint - ResourceType: Wrong Resource Type. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/c1bbc1019bd3
Part 1c: Fix Android Lint - TextFields: Missing inputType. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/de5477d9e048
Part 1d: Ignore ProtectedPermissions in Android Lint. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/d6b31c8eb3b6
Part 2: Update Android Gradle plugin to version 3.1.0. r=nalexander,snorp

Comment 30

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6be5d9b0ac9d
https://hg.mozilla.org/mozilla-central/rev/dd0c6edcb031
https://hg.mozilla.org/mozilla-central/rev/c1bbc1019bd3
https://hg.mozilla.org/mozilla-central/rev/de5477d9e048
https://hg.mozilla.org/mozilla-central/rev/d6b31c8eb3b6