Closed Bug 1476165 Opened 6 years ago Closed 6 years ago

Update Android Gradle plugin to version 3.1.0

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

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

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1476165 - Part 2: Update Android Gradle plugin to version 3.1.0.
59 bytes, text/x-review-board-request
nalexander
: review+
snorp
: review+
Details
Bug 1476165 - Part 1a: Fix Android Lint - ResourceCycle: Cycle in resource definitions.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1476165 - Part 1b: Fix Android Lint - ResourceType: Wrong Resource Type.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1476165 - Part 1c: Fix Android Lint - TextFields: Missing inputType.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1476165 - Part 1d: Ignore ProtectedPermissions in Android Lint.
59 bytes, text/x-review-board-request
nalexander
: review+
Details 
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: nobody → tvijiala
Status: NEW → ASSIGNED
Attachment #8992571 - Attachment is obsolete: true
Attachment #8992571 - Flags: review?(nalexander)
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.
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)
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.
Attachment #8992609 - Flags: review?(nalexander)
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.
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 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 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 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 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 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 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+
Keywords: checkin-needed
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.
Blocks: 1478398
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: