1182193 - Add unit test support to fennec.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

I am finding, on some devices (see Bug 964854) that org.json.simple is just completely broken.  I don't have any device that manifests this.  But those tickets are top crashers that have been open for ages.  I can provide some logs that support my conclusion.

I think it's time for org.json.simple to go.  We used it because:

1) Android's org.json has a poor interface;
2) org.json doesn't work in our JUnit 4 tests (those not run on the device).

1) still applies.  But we use org.json extensively in Fennec; there's no reason to keep our snow-flake here.  I think 2) may have improved, and I'd like to think we could make roboelectric work with our tests.

So this ticket tracks:

* finding a configuration of the android-sync JUnit 4 tests that can run and use org.json, which is usually unhelpfully stubbed.  This might Just Work in Gradle, or might need roboelectric, or a different unstubbing library.

* converting ExtendedJSONObject to use org.json.  There are API mismatches here, so this takes manual effort.

* mopping up the last few org.json.simple uses (JSONArray, etc).  Again, this takes manual effort.

The wins, though, are smaller code size, unification across the Fennec/android-sync divide, and fixing some catastrophic failures on many devices.

This is a good mentor ticket, but not a good first bug.

Comment 1

[Tom Levels]

It would be great if this bug is solved since an increasingly amount of devices cannot use Sync anymore (like me, I have a MT6752 device).

Comment 2

[Richard Newman [:rnewman]]

a+

Comment 3

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 : Pre 1 -Update android gradle plugin to latest version r?nalexander

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1182193 : Pre2 - Unit tests directory related changes r?nalexander

Bug 1182193 : Pre2 - Unit tests directory related changes r?nalexander

Ported TestDateUtils.java to use junit4 annotations.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8653127 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

https://reviewboard.mozilla.org/r/17369/#review15573

I have some issues, so this is just f+.

I think we might just roll this in to the next commit.  We'll want some testing on the Gradle change, too.

::: mobile/android/gradle/base/build.gradle:31
(Diff revision 1)
> -                exclude 'org/mozilla/gecko/tests/**'
> +                exclude 'org/mozilla/gecko/tests/tests/background/**'

These paths must be wrong -- it can't be "tests/tests".

This is really part of the next commit, right?  I.e., you're going to add a new directory of tests/background/junit4 or something?  (I'm fine with pre-landing bits, just this one is odd.)

::: mobile/android/gradle/build.gradle:33
(Diff revision 1)
>          // IntelliJ 14.0.2 wants 0.14.4; IntelliJ 14.0.3 and Android Studio want

The comment needs to be updated too.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8653128 [details]
MozReview Request: Bug 1182193 : Pre2 - Unit tests directory related changes r?nalexander

https://reviewboard.mozilla.org/r/17375/#review15575

This isn't quite what I'm after but it's a great start.  Thanks for pushing into uncharted territory!

::: mobile/android/mach_commands.py:135
(Diff revision 1)
> +        srcdir('base/src/test/java/org/mozilla/gecko',  'mobile/android/tests/unittest/src')

Ah, I see.  We may have multiple unittest suites (like we have browser and background JUnit 3 / instrumentation suites).

So let's go ``mobile/android/tests/background/junit4/src`` or ``/unittest/src`` as you prefer.

::: mobile/android/tests/unittest/src/background/common/TestDateUtils.java:4
(Diff revision 1)
> +package org.mozilla.gecko.background.common;

Ah, I see what you did here.  I'm sure this works, but this test in particular was written for device so that it can be used to time things.  (See the disabled perf test.)

So it's not a good candidate for a JUnit 4 test.  Try...

https://github.com/mozilla-services/android-sync/blob/develop/src/test/java/org/mozilla/gecko/tokenserver/test/TestTokenServerClient.java

or similar.

Comment 7

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8653127 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 Pre: Gradle robolectric junit tests dependency added r?nalexander

Comment 8

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8653127 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 Pre: Gradle robolectric junit tests dependency added r?nalexander

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8653127 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

https://reviewboard.mozilla.org/r/17369/#review15813

Nifty if it works.  I prefer to put new code in the correct paths (for Gradle projects) right off the top, but we don't /have/ to.

::: mobile/android/tests/unittest/moz.build:7
(Diff revision 3)
> +sources= [

This won't work.  Leave it for now, it's a little tricky.  (And kill the other moz.build stuff.)

::: mobile/android/tests/unittest/src/fxa/TestSkewHandler.java:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This didn't include the path changes.


mobile/android/tests/background/unittest/src/test/java/org/mozilla/gecko/fxa/TestSkewHandler.java

would be best.  (Whew, what a mouthful!)

Comment 10

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8653127 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 Pre: Gradle robolectric junit tests dependency added r?nalexander

* Moved robolectric test configuration to properties file.
* Moved login tests from android-sync to use robolectric

Comment 11

[Nick Alexander :nalexander [he/him]]

There are a few pieces to the test-porting part of this work.  First, it's about:

* getting android-sync src/test into mozilla-central;
* configuring build.gradle to use Robolectric to run these as JUnit 4 unit tests without a device.

Second:

* it would be nice to build the src/test files in automation, to make sure that the tests don't bit rot (faster than the source code).  This is like the JUnit 3 instrumentation tests in mobile/android/tests/{browser,background}: they don't get run automatically, but they are compiled.  So the IDE tells you when you change the code, etc.
* it would be nice to build without requiring additional Robolectric, Mockito, test runner, etc JARs in the tree.  Not a show-stopper, but better to just build the Java without new dependencies.

Third, and much harder:

* run the new JUnit 4 tests in automation.  This is a long way out; see also Bug 1064004.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655696 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Comment 14

[Vivek Balakrishnan[:vivek]]

https://reviewboard.mozilla.org/r/17979/#review16185

LGTM. May be we should change the reviewer in bug comment :)

Comment 15

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8653127 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Comment 16

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1182193 - Part 2: Copy the tests from android-sync project. r?nalexander

Bug 1182193 - Part 2: Copy the tests from android-sync project. r?nalexander

Comment 17

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1182193 - Part 3: Added runwith annotations for unittest with gradle r?nalexander

Bug 1182193 - Part 3: Added runwith annotations for unittest with gradle r?nalexander

Comment 18

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8656832 [details]
MozReview Request: Bug 1182193 - Part 3: Added runwith annotations for unittest with gradle r?nalexander

https://reviewboard.mozilla.org/r/18253/#review16371

This will need circulation and testing before landing, but it's looking great!

::: mobile/android/gradle/base/build.gradle:90
(Diff revision 1)
> +    testCompile 'org.simpleframework:simple-http:4.+'

Always pin versions.  This one should 4.1.13, it looks like.

::: mobile/android/tests/background/junit4/src/org/mozilla/android/sync/net/test/TestAccountAuthenticatorStage.java:17
(Diff revision 1)
> +import org.junit.runner.RunWith;

Based on https://github.com/junit-team/junit/wiki/Test-runners#runwith-annotation, it looks like we could have all tests inherit from a BaseTest class, and then annotate just that class.  Can you see if this works?  It would be nice to centralize the single test runner declaration.

Comment 19

[Nick Alexander :nalexander [he/him]]

> :::
> mobile/android/tests/background/junit4/src/org/mozilla/android/sync/net/test/
> TestAccountAuthenticatorStage.java:17
> (Diff revision 1)
> > +import org.junit.runner.RunWith;
> 
> Based on
> https://github.com/junit-team/junit/wiki/Test-runners#runwith-annotation, it
> looks like we could have all tests inherit from a BaseTest class, and then
> annotate just that class.  Can you see if this works?  It would be nice to
> centralize the single test runner declaration.

vivek, on IRC:

10:23 vivek: [12:24:21] Some of the tests are written such a way that they extend from the class we are testing. These tests (eg :TestCommand
10:23 vivek: [12:25:08] Processor) access protected member of CommandProcessor class. Do you want me to modify these  tests?
10:23 vivek: [12:26:40] I was planning to have a base interface to centralize @RunWith annotations but it feels like anti-pattern as interface does not define any contract.

This is a good point.  Let's roll with the existing @RunWith annotations.  With that, I think we're ready to post to the mailing list and land.  vivek, could you draft a message to the list?  I mostly want to publicize that we're bumping the Gradle plugin version and that we want reports of incompatibility with AS and IJ.  The actual unittest stuff we can document separately.

Comment 20

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8653127 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

Comment 21

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8656831 [details]
MozReview Request: Bug 1182193 - Part 2: Copy the tests from android-sync project. r?nalexander

Bug 1182193 - Part 2: Copy the tests from android-sync project. r?nalexander

Comment 22

[Vivek Balakrishnan[:vivek]]

Comment on attachment 8656832 [details]
MozReview Request: Bug 1182193 - Part 3: Added runwith annotations for unittest with gradle r?nalexander

Bug 1182193 - Part 3: Added runwith annotations for unittest with gradle r?nalexander

Comment 23

[Vivek Balakrishnan[:vivek]]

Attachment: MozReview Request: Bug 1182193 - Part 4: Disable unit test for regular gradle build tasks r?nalexander

Bug 1182193 - Part 4: Disable unit test for regular gradle build tasks r?nalexander

Comment 24

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8660165 [details]
MozReview Request: Bug 1182193 - Part 4: Disable unit test for regular gradle build tasks r?nalexander

https://reviewboard.mozilla.org/r/19089/#review17013

::: mobile/android/gradle/build.gradle:64
(Diff revision 1)
> +// I bet there is a easier/cleaner way to do this, but this gets the job done for now.

Do you have a reference for this?

Please file a follow-up to make this check more clever.  I'd like |gradle test| to do like this; but I'd also like to make |gradle base:test| restrict to the base project, and to make |gradle testDebugUnitTest| restrict to just the debug builds.

But definitely follow-up.

Comment 25

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8656831 [details]
MozReview Request: Bug 1182193 - Part 2: Copy the tests from android-sync project. r?nalexander

https://reviewboard.mozilla.org/r/18251/#review17015

lgtm.

Comment 26

[Nick Alexander :nalexander [he/him]]

vivek: run your last tests, address that one nit about pinning, and then land and post to the mailing list!

Let's take this bug for the test changes, update the title, and start a new one for the JSON bits.  Great work!

Comment 27

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8655696 [details]
MozReview Request: Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests. r?nalexander

This might be r=vivek?  Can't recall.

Comment 28

[Vivek Balakrishnan[:vivek]]

https://hg.mozilla.org/integration/fx-team/rev/c02886680b09ced8b1c359ebf698b7f1ba827085
Bug 1182193 - Part 1: Add Gradle-based Robolectric JUnit 4 tests r=nalexander

Comment 29

[Vivek Balakrishnan[:vivek]]

https://hg.mozilla.org/integration/fx-team/rev/45d1ea755875ff11ebf461206c707dc46e3d2b04
Bug 1182193 - Part 2: Copy the tests from android-sync project. r=nalexander

Comment 30

[Vivek Balakrishnan[:vivek]]

https://hg.mozilla.org/integration/fx-team/rev/bceeacdb09f195cde005199dae3eab2ac44b3939
Bug 1182193 - Part 3: Added runwith annotations for unittest with gradle r=nalexander

Comment 31

[Vivek Balakrishnan[:vivek]]

https://hg.mozilla.org/integration/fx-team/rev/ce211c03a51cadee6b45673394ec3cbfd620830f
Bug 1182193 - Part 4: Disable unit test for regular gradle build tasks r=nalexander

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c02886680b09
https://hg.mozilla.org/mozilla-central/rev/45d1ea755875
https://hg.mozilla.org/mozilla-central/rev/bceeacdb09f1
https://hg.mozilla.org/mozilla-central/rev/ce211c03a51c