Closed Bug 1409087 Opened 7 years ago Closed 7 years ago

Remove the browser and background JUnit 3 test APKs

Categories

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

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

Bug 1409087 - Part 1: Purge typoed javaddons directory.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1409087 - Part 2: Move Fennec unit tests into standard Gradle locations.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1409087 - Part 3: Move services integration tests into standard Gradle location.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1409087 - Part 4: Remove browser JUnit 3 tests and build apparatus for {browser,background}.apk. .mielczarek
59 bytes, text/x-review-board-request
Grisha
: review+
ted
: review+
Details 
Bug 1064004 tracks running JUnit 3 tests in automation.  That effort has failed:

- we still don't run these tests in automation
- we instead build JUnit 4 tests that don't require a device
- new testing initiatives will run through Gradle (for example, Espresso on devices) and not through moz.build test APKs that are invoked by mozharness.

This ticket tracks migrating or removing these tests.
grisha: as the main Android Sync developer, can you comment on the usefulness of the tests rooted in

http://searchfox.org/mozilla-central/source/mobile/android/tests/background/junit3

I wonder how many of these can be run in JUnit 4, off device...
Flags: needinfo?(gkruglov)
Locally I see failures that I cannot explain:

org.mozilla.gecko.util.publicsuffix.TestPublicSuffix > testGetPublicSuffixZeroAdditionalParts FAILED
    org.junit.ComparisonFailure at TestPublicSuffix.java:105

org.mozilla.gecko.util.publicsuffix.TestPublicSuffix > testStripPublicSuffix FAILED
    org.junit.ComparisonFailure at TestPublicSuffix.java:24

org.mozilla.gecko.util.publicsuffix.TestPublicSuffix > testGetPublicSuffixNonZeroAdditionalParts FAILED
    org.junit.ComparisonFailure at TestPublicSuffix.java:122

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainNormalTwoLevelPublicSuffixWithSubdomain FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainNoSuffix0Parts FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainNoSuffix1Parts FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainNoSuffix2Parts FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainWithSuffix0Parts FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainWithSuffix1Parts FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainWithSuffix2Parts FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

org.mozilla.gecko.util.TestURIUtils > testGetFormattedDomainTwoLevelPublicSuffix FAILED
    org.junit.ComparisonFailure at TestURIUtils.java:200

833 tests completed, 11 failed, 2 skipped
:app:testOfficialPhotonDebugUnitTest FAILED

I see these without my patches, too -- and cannot explain those either!  I've pushed to try and we'll see what turns up:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a81f88888d9bf3a3b5be5b91fb6146eaf293778
Glad it's not just me. I see the same failures locally - asked Mike to take a look in https://bugzilla.mozilla.org/show_bug.cgi?id=1386902#c23, which is I think the last time we touched that code (although I suspect this regression in tests isn't rooted in the original work).
Flags: needinfo?(gkruglov)
Didn't mean to actually remove the NI.

There's certainly some conceptual overlap between junit4 and junit3 services tests, and they do cover somewhat different cross-sections of the code. Having assumed that junit3 tests actually do run in automation, I have certainly done my part in the sisyphean effort of "keeping junit3 services tests up-to-date", which feels rather silly now. Lesson learned! Given the questionable nature of their usefulness - and your report that a good chunk fail isn't surprising, either - I say go forth and excise them from the codebase.

I will note that keeping junit3 tests "up-to-date" after large changes to sync was/is both an entirely frustrating experience, but also a somewhat useful one - a few times, I think, it did force me to consider some cases which I wouldn't have otherwise. Since they never did run, it's hard to say if I actually considered these cases enough :)
Comment on attachment 8924761 [details]
Bug 1409087 - Part 1: Purge typoed javaddons directory.

https://reviewboard.mozilla.org/r/195988/#review201514

Not sure what this was supposed to be; it doesn't even seem to be in the tree.
Attachment #8924761 - Flags: review?(gkruglov) → review+
Comment on attachment 8924762 [details]
Bug 1409087 - Part 2a: Move test resources under app/.

https://reviewboard.mozilla.org/r/195990/#review201516

Looks sensible (and I'm going to assume you moved _all_ of the resources), but I'm guessing this change by itself isn't quite enough? E.g. I'm assuming consumers of these resources will also be moved in subsequent patches.
Attachment #8924762 - Flags: review?(gkruglov) → review+
(In reply to :Grisha Kruglov from comment #12)
> Comment on attachment 8924761 [details]
> Bug 1409087 - Part 1: Purge typoed javaddons directory.
> 
> https://reviewboard.mozilla.org/r/195988/#review201514
> 
> Not sure what this was supposed to be; it doesn't even seem to be in the
> tree.

Correct -- it was a typo, so it didn't get purged by https://bugzilla.mozilla.org/show_bug.cgi?id=1363843.
(In reply to :Grisha Kruglov from comment #13)
> Comment on attachment 8924762 [details]
> Bug 1409087 - Part 2a: Move test resources under app/.
> 
> https://reviewboard.mozilla.org/r/195990/#review201516
> 
> Looks sensible (and I'm going to assume you moved _all_ of the resources),
> but I'm guessing this change by itself isn't quite enough? E.g. I'm assuming
> consumers of these resources will also be moved in subsequent patches.

Yep -- the parts that are conceptually together are 2{a,b,c,d} and 3{a,b}.  Just a bunch are "hg mv ..." and impossible to really read on MozReview.
(In reply to :Grisha Kruglov from comment #11)
> Didn't mean to actually remove the NI.
> 
> There's certainly some conceptual overlap between junit4 and junit3 services
> tests, and they do cover somewhat different cross-sections of the code.
> Having assumed that junit3 tests actually do run in automation, I have
> certainly done my part in the sisyphean effort of "keeping junit3 services
> tests up-to-date", which feels rather silly now. Lesson learned!


 Given the
> questionable nature of their usefulness - and your report that a good chunk
> fail isn't surprising, either - I say go forth and excise them from the
> codebase.

Not quite.  There were _two_ sets of JUnit 3 tests: browser/ and background/.  browser/ I'm just removing (with your blessing); they've never been run in automation and are quite broken.  background/ have also never been run in automation, but I haven't checked if they're broken.  I assumed you were running them from within Android Studio, but I see you probably haven't been :/  I'll check and update.  We should try to keep these green locally and actually run them in automation (using Gradle).

> I will note that keeping junit3 tests "up-to-date" after large changes to
> sync was/is both an entirely frustrating experience, but also a somewhat
> useful one - a few times, I think, it did force me to consider some cases
> which I wouldn't have otherwise. Since they never did run, it's hard to say
> if I actually considered these cases enough :)

Perhaps you have been running these?  They were definitely green not that long ago... will update.
> > I will note that keeping junit3 tests "up-to-date" after large changes to
> > sync was/is both an entirely frustrating experience, but also a somewhat
> > useful one - a few times, I think, it did force me to consider some cases
> > which I wouldn't have otherwise. Since they never did run, it's hard to say
> > if I actually considered these cases enough :)
> 
> Perhaps you have been running these?  They were definitely green not that
> long ago... will update.

These have rotted severely: locally, about 40/160 tests are failing.  Off the top, I see changes to the counting of records and changes to bookmarks reconciling didn't actually update these tests.  I'm not going to purge the code immediately, but we also can't "just" start running them :/
(In reply to Nick Alexander :nalexander from comment #9)
> Locally I see failures that I cannot explain:
> 
> org.mozilla.gecko.util.publicsuffix.TestPublicSuffix >
> testGetPublicSuffixZeroAdditionalParts FAILED
>     org.junit.ComparisonFailure at TestPublicSuffix.java:105
> 
> ...

It looks like we're failing to open `android/app/assets/publicsuffixlist` [1][2] and we silently swallow this error (which seems silly – I'll file) until the tests fail.

Nevin's push on 11/2 [3] didn't fail, which is the same day --with-gradle landed so I'd expect Nevin pushed before --with-gradle and this is a regression from gradle. Nick, what do you think?

Note: we only run unit tests when *java code changes so we didn't run the unit tests (and thus this failure) for the push in comment 9.

[1]: https://searchfox.org/mozilla-central/source/mobile/android/app/assets/publicsuffixlist
[2]: https://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/publicsuffix/PublicSuffixPatterns.java#33

[3]: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e1a4d0b5eb152db7d06ccd20f919879a440ab6db&selectedJob=141650454
Flags: needinfo?(nalexander)
> and we silently swallow this error (which seems silly – I'll file)

bug 1414995.
Comment on attachment 8924763 [details]
Bug 1409087 - Part 2b: Move Fennec tests into standard Gradle locations.

https://reviewboard.mozilla.org/r/195992/#review202162

Seems like you got them all. The seemingly arbitrary inclusion of `test` in the various `sync` test paths (e.g. `sync/test/TestFile.java` vs `sync/repositories/TestFile.java` vs `sync/repositories/test/TestFile.java`) paths is annoying, but I'm not sure it's annoying enough to actually fix.

::: commit-message-c0505:20
(Diff revision 2)
> +hg purge mobile/android
> +
> +mkdir -p mobile/android/services/src/test/java/org/mozilla
> +hg mv mobile/android/tests/background/junit4/src/org/mozilla/android mobile/android/services/src/test/java/org/mozilla/
> +mkdir -p mobile/android/services/src/test/java/org/mozilla/gecko
> +hg mv mobile/android/tests/background/junit4/src/org/mozilla/gecko/background mobile/android/services/src/test/java/org/mozilla/gecko/

TestTabsProvider.java seems to fit better under mozilla/gecko/db
Attachment #8924763 - Flags: review?(gkruglov) → review+
Comment on attachment 8924764 [details]
Bug 1409087 - Part 2: Move Fennec unit tests into standard Gradle locations.

https://reviewboard.mozilla.org/r/195994/#review202166

::: mobile/android/app/build.gradle:195
(Diff revision 2)
>          }
>  
>          test {
>              java {
> -                srcDir "${topsrcdir}/mobile/android/tests/background/junit4/src"
> +                // Bug 1229149 tracks pushing this into a :services Gradle project.
> +                srcDir "${topsrcdir}/mobile/android/services/src/test/java"

What about all the junit4 tests under `mobile/android/app/src/test`? Now that you've split them, this config seems to exclude them? Or will they be included automatically due to their 'default' location?
Comment on attachment 8924765 [details]
Bug 1409087 - Part 3: Move services integration tests into standard Gradle location.

https://reviewboard.mozilla.org/r/195996/#review202168

OK.
Attachment #8924765 - Flags: review?(gkruglov) → review+
Comment on attachment 8924765 [details]
Bug 1409087 - Part 3: Move services integration tests into standard Gradle location.

https://reviewboard.mozilla.org/r/195996/#review202172

::: commit-message-70478:3
(Diff revision 2)
> +Bug 1409087 - Part 3a: Move services integration tests into standard Gradle location. r=grisha
> +
> +This is the result of `hg mv mobile/android/tests/background/junit3/srcmobile/android/services/src/androidTest/java`

nit: missing a space between paths.
Comment on attachment 8924766 [details]
Bug 1409087 - Part 3b: Patch up Gradle configuration.

https://reviewboard.mozilla.org/r/195998/#review202174
Attachment #8924766 - Flags: review?(gkruglov) → review+
Trying to |mach build && mach package| with this patch series gives me the following at the very end of 'package':

 0:18.09 cp: cannot stat ‘/home/grisha/Code/mozilla-central/objdir-frontend/mobile/android/tests/browser/robocop/robocop-debug-unsigned-unaligned.apk’: No such file or directory
 0:18.09 /home/grisha/Code/mozilla-central/toolkit/mozapps/installer/packager.mk:97: recipe for target 'make-package-internal' failed
 0:18.09 gmake[3]: *** [make-package-internal] Error 1
 0:18.09 gmake[3]: Leaving directory '/home/grisha/Code/mozilla-central/objdir-frontend/mobile/android/installer'
 0:18.09 /home/grisha/Code/mozilla-central/toolkit/mozapps/installer/packager.mk:101: recipe for target 'make-package' failed
 0:18.09 gmake[2]: *** [make-package] Error 2
 0:18.09 gmake[2]: Leaving directory '/home/grisha/Code/mozilla-central/objdir-frontend/mobile/android/installer'
 0:18.09 /home/grisha/Code/mozilla-central/config/rules.mk:442: recipe for target 'default' failed
 0:18.09 gmake[1]: *** [default] Error 2
 0:18.09 gmake[1]: Leaving directory '/home/grisha/Code/mozilla-central/objdir-frontend/mobile/android/installer'
 0:18.09 /home/grisha/Code/mozilla-central/mobile/android/build.mk:11: recipe for target 'package' failed
 0:18.09 gmake: *** [package] Error 2
 0:18.09 gmake: Leaving directory '/home/grisha/Code/mozilla-central/objdir-frontend'
Comment on attachment 8924767 [details]
Bug 1409087 - Part 4: Remove browser JUnit 3 tests and build apparatus for {browser,background}.apk. .mielczarek

https://reviewboard.mozilla.org/r/196000/#review202178

IIUC, among others things this removes build hooks for *background* stuff, but leaves the tests themselves in place allowing us to run them locally. I'm having some trouble running these tests on my local device, but I assume that's because I'm doing something wrong (java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.Class java.lang.Object.getClass()' on a null object reference
at org.mozilla.gecko.tests.testShareLink.<init>(testShareLink.java:29)).

But to clarify - the reason we're leaving *services* junit3 tests around is because you're seeing a lot of them fail, and we're not confident that these failures aren't indicative of actual problems. And so, it seems that before we consider removing them entirely, we first need to make sure these tests aren't telling us something useful. Or, perhaps instead of removing them after greening/inspection, we can also transition them to be regular off-device unit tests.

Similarly for the browser junit3 tests, in the commit message you've stated that 'Half of the existing "tests" are broken on my local device'. I take it that by removing these tests regardless, you're stating that there isn't anything valuable in these failures? They aren't indicative of any real regressions, but simply of code rot?
A few attempts later, Comment 32 no longer happens and |package| runs without problems.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
Comment on attachment 8924764 [details]
Bug 1409087 - Part 2: Move Fennec unit tests into standard Gradle locations.

https://reviewboard.mozilla.org/r/195994/#review202378

::: mobile/android/app/build.gradle:195
(Diff revision 2)
>          }
>  
>          test {
>              java {
> -                srcDir "${topsrcdir}/mobile/android/tests/background/junit4/src"
> +                // Bug 1229149 tracks pushing this into a :services Gradle project.
> +                srcDir "${topsrcdir}/mobile/android/services/src/test/java"

They will be automatically included due to their default location.  Over time, we'll remove all of these non-standard things, greatly simplifying our configurations (and eventually, our lives).
Comment on attachment 8924767 [details]
Bug 1409087 - Part 4: Remove browser JUnit 3 tests and build apparatus for {browser,background}.apk. .mielczarek

https://reviewboard.mozilla.org/r/196000/#review202380
Attachment #8924767 - Flags: review?(ted) → review+
Comment on attachment 8924763 [details]
Bug 1409087 - Part 2b: Move Fennec tests into standard Gradle locations.

https://reviewboard.mozilla.org/r/195992/#review202162

> TestTabsProvider.java seems to fit better under mozilla/gecko/db

I concur, but let's leave renaming (and reformatting to fit Fennec style, etc) for a later day.
(In reply to :Grisha Kruglov from comment #33)
> Comment on attachment 8924767 [details]
> Bug 1409087 - Part 4: Remove browser JUnit 3 tests and build apparatus for
> {browser,background}.apk. .mielczarek
> 
> https://reviewboard.mozilla.org/r/196000/#review202178
> 
> IIUC, among others things this removes build hooks for *background* stuff,
> but leaves the tests themselves in place allowing us to run them locally.
> I'm having some trouble running these tests on my local device, but I assume
> that's because I'm doing something wrong (java.lang.NullPointerException:
> Attempt to invoke virtual method 'java.lang.Class
> java.lang.Object.getClass()' on a null object reference
> at org.mozilla.gecko.tests.testShareLink.<init>(testShareLink.java:29)).

What is happening here is that the background test code expects to run against a debug build, but our debug build is Proguarded  for ... reasons (see http://searchfox.org/mozilla-central/source/mobile/android/app/build.gradle#45-59).  So you're seeing standard Proguard errors.  It's possible to work around them in the existing configuration, but the real solution is to use the Android-Gradle plugin 3.0+ (https://bugzilla.mozilla.org/show_bug.cgi?id=1411654), which I am working on now.

> But to clarify - the reason we're leaving *services* junit3 tests around is
> because you're seeing a lot of them fail, and we're not confident that these
> failures aren't indicative of actual problems.

Not really: I honestly doubt there are actual problems.  The existing tests simply haven't followed the code as it has evolved, due to them never being exercised :(

 And so, it seems that before
> we consider removing them entirely, we first need to make sure these tests
> aren't telling us something useful.

Not really.  You're doing fine without these tests -- how valuable can they be?

 Or, perhaps instead of removing them
> after greening/inspection, we can also transition them to be regular
> off-device unit tests.

This seems more likely.

> Similarly for the browser junit3 tests, in the commit message you've stated
> that 'Half of the existing "tests" are broken on my local device'. I take it
> that by removing these tests regardless, you're stating that there isn't
> anything valuable in these failures? They aren't indicative of any real
> regressions, but simply of code rot?

In this case, there's nothing of value.  Most of the tests are totally irrelevant.
(In reply to Michael Comella (:mcomella) from comment #25)
> (In reply to Nick Alexander :nalexander from comment #9)
> > Locally I see failures that I cannot explain:
> > 
> > org.mozilla.gecko.util.publicsuffix.TestPublicSuffix >
> > testGetPublicSuffixZeroAdditionalParts FAILED
> >     org.junit.ComparisonFailure at TestPublicSuffix.java:105
> > 
> > ...
> 
> It looks like we're failing to open `android/app/assets/publicsuffixlist`
> [1][2] and we silently swallow this error (which seems silly – I'll file)
> until the tests fail.
> 
> Nevin's push on 11/2 [3] didn't fail, which is the same day --with-gradle
> landed so I'd expect Nevin pushed before --with-gradle and this is a
> regression from gradle. Nick, what do you think?

Kinda-sorta.  There's a pipeline issue here where the Robotium "fake Android environment" code has to find the assets, but there isn't a real APK to fish them for when the unit tests run.  I think Robotium uses the Gradle configuration to find the assets but it's perhaps not working.  I'll investigate; I don't think it'll be hard to address.

This might interact strangely with the APK packing stuff we do in |mach package|, but I want to mostly deprecate that business for these assets anyway.

> Note: we only run unit tests when *java code changes so we didn't run the
> unit tests (and thus this failure) for the push in comment 9.
> 
> [1]:
> https://searchfox.org/mozilla-central/source/mobile/android/app/assets/
> publicsuffixlist
> [2]:
> https://searchfox.org/mozilla-central/rev/
> 7e090b227f7a0ec44d4ded604823d48823158c51/mobile/android/geckoview/src/main/
> java/org/mozilla/gecko/util/publicsuffix/PublicSuffixPatterns.java#33
> 
> [3]:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=e1a4d0b5eb152db7d06ccd20f919879a440ab6db&selected
> Job=141650454
Depends on: 1415298
> Nevin's push on 11/2 [3] didn't fail, which is the same day --with-gradle
> landed so I'd expect Nevin pushed before --with-gradle and this is a
> regression from gradle. Nick, what do you think?

I can confirm this regression: Bug 1415298.  It doesn't really block this work.
OK, I've tracked down the testing regression (which doesn't block this) and addressed grisha's comments.  I'm missing one tiny r+ from grisha, and he's PTO for the rest of the week, so I'm going to r=me this by squashing some commits so that I _do_ have grisha's review.
I'm just mostly being cautious out of "I don't know what I don't know" mindset. Your response make sense, so roll on!
Comment on attachment 8924764 [details]
Bug 1409087 - Part 2: Move Fennec unit tests into standard Gradle locations.

https://reviewboard.mozilla.org/r/195994/#review202466
Attachment #8924764 - Flags: review?(gkruglov) → review+
Comment on attachment 8924767 [details]
Bug 1409087 - Part 4: Remove browser JUnit 3 tests and build apparatus for {browser,background}.apk. .mielczarek

https://reviewboard.mozilla.org/r/196000/#review202468

I'm not as confident about the value (or lack of) of *services* tests as you are, but we're not removing them here - so we'll deal with them in due course.
Attachment #8924767 - Flags: review?(gkruglov) → review+
Attachment #8924762 - Attachment is obsolete: true
Attachment #8924763 - Attachment is obsolete: true
Attachment #8924766 - Attachment is obsolete: true
Comment on attachment 8924765 [details]
Bug 1409087 - Part 3: Move services integration tests into standard Gradle location.

https://reviewboard.mozilla.org/r/195996/#review202172

> nit: missing a space between paths.

Fixed in updated commits.
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0e29a3d47257
Part 1: Purge typoed javaddons directory. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/6565cf9eed28
Part 2: Move Fennec unit tests into standard Gradle locations. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/02cd8929225a
Part 3: Move services integration tests into standard Gradle location. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/7f0357dd1f97
Part 4: Remove browser JUnit 3 tests and build apparatus for {browser,background}.apk. r=Grisha,ted.mielczarek
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 58 → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: