Update Android Gradle plugin to version 3.1.0

RESOLVED FIXED in Firefox 63

Status

()

defect
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: gabriel-v, Assigned: gabriel-v)

Tracking

(Blocks 1 bug)

unspecified
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

9 months ago
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).
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.
(Assignee)

Updated

9 months ago
Assignee: nobody → tvijiala
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8992571 - Attachment is obsolete: true
Attachment #8992571 - Flags: review?(nalexander)
(Assignee)

Comment 4

9 months ago
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.
(Assignee)

Comment 5

9 months ago
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?
Flags: needinfo?(nalexander)
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.
Flags: needinfo?(nalexander)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

9 months ago
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.
(Assignee)

Updated

9 months ago
Attachment #8992609 - Flags: review?(nalexander)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

9 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

9 months ago
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

9 months ago
mozreview-review
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.
Attachment #8992609 - Flags: review?(nalexander) → review+

Comment 22

9 months ago
mozreview-review
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.
Attachment #8994184 - Flags: review?(nalexander) → review+

Comment 23

9 months ago
mozreview-review
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.
Attachment #8994185 - Flags: review?(nalexander) → review+

Comment 24

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

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

Sure.
Attachment #8994186 - Flags: review?(nalexander) → review+

Comment 25

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

https://reviewboard.mozilla.org/r/258792/#review265760
Attachment #8994187 - Flags: review?(nalexander) → review+

Comment 26

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

https://reviewboard.mozilla.org/r/258792/#review265762
Attachment #8992609 - Flags: review?(snorp)
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
Attachment #8992609 - Flags: review?(snorp) → review+
(Assignee)

Updated

9 months ago
Keywords: checkin-needed
(Assignee)

Comment 28

9 months ago
mozreview-review-reply
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.
(Assignee)

Updated

9 months ago
Blocks: 1478398

Comment 29

9 months ago
Pushed by apavel@mozilla.com:
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
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.