840825 - [Robocop] Basic tests for distribution customization

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

Attachment: WIP

We should write some tests for distribution support!

Right now, the two main things to test are setting up preferences from preferences.json, and creating bookmarks from bookmarks.json.

I have a WIP that lets us call Distribution.init with a mock package where it looks for the distribution files.

Some remaining issues here:
* Reading the preferences to make sure they're set correctly
* I wrote this as a ContentProviderTest, since that's probably the best way to check the bookmarks, but I'm not sure how to delay creating BrowserProvider until after the distribution stuff is initialized (this may require changing around ContentProviderTest a bit)

I think we can land this once the preferences part is working, then do the bookmarks stuff in a follow-up bug. Cc'ing capella, since he said he might be willing to help out here :)

Comment 1

[Wesley Johnston (:wesj)]

Comment on attachment 713217 [details] [diff] [review]
WIP

Review of attachment 713217 [details] [diff] [review]:
-----------------------------------------------------------------

Cool

::: mobile/android/base/Distribution.java
@@ +75,5 @@
>       * Returns true if distribution files were found and copied.
>       */
> +    private static boolean copyFiles(Activity activity, String mockPackagePath) throws IOException {
> +        String packagePath = (mockPackagePath == null) ? activity.getPackageResourcePath() : mockPackagePath;
> +        File applicationPackage = new File(packagePath);

We could just as easily call this packagePath and do

if (packagePath == null)
  packagePath = activity.getRes..

which feels less confusing to me. i.e. I don't think we need 'mock' in here or probably in the earlier method either.

::: mobile/android/base/tests/testDistribution.java.in
@@ +44,5 @@
> +
> +            Actions.RepeatedEventExpecter contentEventExpecter = mActions.expectGeckoEvent("Preferences:Data");
> +            mActions.sendGeckoEvent("Preferences:Get", message.toString());
> +            contentEventExpecter.blockUntilClear(2000);
> +            mAsserter.ok(true, "getting preferences", "got preferences");

Grr.. I can land my stuff from bug 777451 so that you can do contentEventExpecter.waitForReturn() instead here and actually check the results.

@@ +82,5 @@
> +        Actions.EventExpecter eventExpector = mActions.expectGeckoEvent("Distribution:Set:OK");
> +        init.invoke(null, getActivity(), getMockPackagePath());
> +        eventExpector.blockForEvent();
> +
> +        mAsserter.ok(true, "waiting for \"Distribution:Set:OK\"", "got \"Distribution:Set:OK\"");

The expector should throw if we don't get this, right?

Comment 2

[Mark Capella [:capella]]

Cool!

1) This leaves the test distribution folder in the current installed /data/data.org.mozilla.fennec and |distribution.| prefs set in about:config

2) I wish I'd had this when doing that last patch for the |About Nightly| page :D

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 713217 [details] [diff] [review]
WIP

I get all sad faced when I see testing code/paths in the production code. What can we do to remove it?

Comment 4

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #3)

> I get all sad faced when I see testing code/paths in the production code.
> What can we do to remove it?

We'd have to somehow get the distribution files into the test APK, which seems complicated, since you can't repackage the APK while it's running.

I feel like this doesn't really add that much to Distribution.java, and if I change the variable name to be "packagePath" like wesj suggests, it implies you could look elsewhere for the distribtion files, and isn't this what we want to do for the partner repacks anyway?

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Wesley Johnston (:wesj) from comment #1)
> > +        eventExpector.blockForEvent();
> > +
> > +        mAsserter.ok(true, "waiting for \"Distribution:Set:OK\"", "got \"Distribution:Set:OK\"");
> 
> The expector should throw if we don't get this, right?

Right. eventExpector.blockForEvent() will throw if the event is not received in MAX_WAIT_MS=90000 ms. I think this check is fine as it is.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #4)

> I feel like this doesn't really add that much to Distribution.java, and if I
> change the variable name to be "packagePath" like wesj suggests, it implies
> you could look elsewhere for the distribtion files, and isn't this what we
> want to do for the partner repacks anyway?

That would make my face less sad

Comment 7

[:Margaret Leibovic]

Attachment: (Part 1) Add blockForEventData method to EventExpecter

This is an updated version of wesj's second patch from bug 777451. I figure we can land it here, then update that bug to just be about the remaining patch in there.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 2) Basic distribution test

This patch sets up a distribution, checks that the basic prefs are set correctly, then cleans up after itself.

To make mfinkle happy, I removed any reference to testing from Distribution.java. It will be nice if we can expand on this to just pass a different packagePath for our special partner repacks.

I'll push this to try to make sure it doesn't break anything, and I'd like to file follow up bugs for the other things we should be testing (other preferences, bookmarks, localization, etc).

Comment 9

[Mark Capella [:capella]]

TRY testing I bet will work ok, as we use a fresh install each time.

During local robocop testing, my initial test of a clean installed nightly runs ok, but we leave the keyName: org.mozilla.fennec.distribution_state == |STATE_SET| causing subsequent test fails ... 

(tmi: On the second test, Distribution.java - init() - Bails as |state == STATE_SET| but still posts |Distribution:Set| to browser.js, with trickles down through |reload-default-prefs|, then |prefservice:after-app-defaults| then getPrefs() which fails trying to readJSON() the file /data/data/org.mozilla.fennec/distribution/preferences.json that we avoided copying .... which then avoids triggering |Distribution:Set:OK| which the testDistribution.java.in is waiting on until timeout ... blah blah blah)

Comment 10

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #9)
> TRY testing I bet will work ok, as we use a fresh install each time.
> 
> During local robocop testing, my initial test of a clean installed nightly
> runs ok, but we leave the keyName: org.mozilla.fennec.distribution_state ==
> |STATE_SET| causing subsequent test fails ... 
> 
> (tmi: On the second test, Distribution.java - init() - Bails as |state ==
> STATE_SET| but still posts |Distribution:Set| to browser.js, with trickles
> down through |reload-default-prefs|, then |prefservice:after-app-defaults|
> then getPrefs() which fails trying to readJSON() the file
> /data/data/org.mozilla.fennec/distribution/preferences.json that we avoided
> copying .... which then avoids triggering |Distribution:Set:OK| which the
> testDistribution.java.in is waiting on until timeout ... blah blah blah)

Hrm, another good catch here. Maybe we should make some Distribution.uninit() method that takes care of cleaning up the data directory and resetting the pref.

Unfortunately my try run failed because it also included my patch for bug 836450. I'm going to work on fixing that up today.

Comment 11

[:Margaret Leibovic]

Without the patch for bug 836450, this is green on try:
https://tbpl.mozilla.org/?tree=Try&rev=643a662629e0

But as capella mentioned in comment 9, we should clean up that pref.

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 713728 [details] [diff] [review]
(Part 1) Add blockForEventData method to EventExpecter

Review of attachment 713728 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for resurrecting this!

Would there be any use in returning Object or Object[] from blockForEventData, rather than the String? Probably not...just a thought.

Comment 13

[Geoff Brown [:gbrown]]

Comment on attachment 713729 [details] [diff] [review]
(Part 2) Basic distribution test

Review of attachment 713729 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/tests/testDistribution.java.in
@@ +119,5 @@
> +        File dataDir = new File(getActivity().getApplicationInfo().dataDir);
> +
> +        // Delete mock package from data directory.
> +        File mockPackage = new File(dataDir, MOCK_PACKAGE);
> +        mAsserter.ok(mockPackage.delete(), "clean up " + MOCK_PACKAGE, "deleted " + MOCK_PACKAGE);

I would prefer to see the full path (not just the file name) in the log message.

@@ +127,5 @@
> +        File[] files = distDir.listFiles();
> +        for (File f : files) {
> +            mAsserter.ok(f.delete(), "clean up " + f.getName(), "deleted " + f.getName());
> +        }
> +        mAsserter.ok(distDir.delete(), "clean up distribution directory", "deleted distribution directory");

Log the deleted path here too please.

Comment 14

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #12)

> Would there be any use in returning Object or Object[] from
> blockForEventData, rather than the String? Probably not...just a thought.

Hm, not sure. I don't know what all the Objects in that args array correspond to, and whether or not they're worth passing along. I feel like the JSON string associated with the message has everything we'd ever want to see.

Comment 15

[:Margaret Leibovic]

Attachment: (Part 2) Basic distribution test (v2)

Updated this patch to address capella and gbrown's comments (also to apply on top of the new patch from bug 836450).

I'd like to get wesj's review on the Distribution.java/browser.js parts.

Pushed to try to see what happens:
https://tbpl.mozilla.org/?tree=Try&rev=9a0463d32f26

Comment 16

[:Margaret Leibovic]

The try run was green, so I pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3be58b112a33
https://hg.mozilla.org/integration/mozilla-inbound/rev/458969f6b369

Comment 17

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #16)
> The try run was green, so I pushed to inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3be58b112a33
> https://hg.mozilla.org/integration/mozilla-inbound/rev/458969f6b369

Urgh, commented in wrong bug. Please ignore this.

Comment 18

[:Margaret Leibovic]

(The try run was green, just waiting on a final look from wesj)

Comment 19

[Mark Capella [:capella]]

If I create a custom distribution folder w/preferences.json, and package and install it through an apk, this robotest fails (I think) due to the same reason in comment 9.

If this is something that realistically could happen (vs me looking for creative ways to shoot holes), maybe the test could warn out / bypass? I'm not sure how you'd make the test run over this described starting condition, then determine the correct recovery state vs. simply deleting the prefs key for example.

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 714116 [details] [diff] [review]
(Part 2) Basic distribution test (v2)

Review of attachment 714116 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +240,5 @@
>          registerEventListener("Feedback:MaybeLater");
>          registerEventListener("Dex:Load");
>          registerEventListener("Telemetry:Gather");
>  
> +        Distribution.init(this, null);

I think we should consider passing in context.getPackageResourcePath(); here and just returning if null is passed in. But it doesn't make a ton of difference I guess.

::: mobile/android/base/Distribution.java
@@ +42,5 @@
>      /**
>       * Initializes distribution if it hasn't already been initalized.
> +     *
> +     * packagePath specifies where to look for the distribution directory.
> +     * If it is null, we'll just look in the root of the APK.

To do this right, you should prepend this with @param.

::: mobile/android/base/tests/testDistribution.java.in
@@ +111,5 @@
> +
> +        // Call Distribution.init with the mock package.
> +        ClassLoader classLoader = mActivity.getClassLoader();
> +        Class distributionClass = classLoader.loadClass("org.mozilla.gecko.Distribution");
> +        Class activityClass = classLoader.loadClass("android.content.Context");

seems like this should be contextClass

Comment 21

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #19)
> If I create a custom distribution folder w/preferences.json, and package and
> install it through an apk, this robotest fails (I think) due to the same
> reason in comment 9.
> 
> If this is something that realistically could happen (vs me looking for
> creative ways to shoot holes), maybe the test could warn out / bypass? I'm
> not sure how you'd make the test run over this described starting condition,
> then determine the correct recovery state vs. simply deleting the prefs key
> for example.

Yeah, the test definitely does not expect for there to already be an existing distribution. If we're going to run these automated tests on partner repacks, this is something to consider, but I think we can deal with that in a separate bug if it proves to be a problem.

Comment 22

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ca3db39a8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/791937021ae5

Just after landing this, I realized that addressing wesj's comment about the packagePath parameter means that we're going to always be checking for the distribution on startup instead of bailing out in init(). I'm working on a quick follow-up for this that we can just land as part of this bug.

Comment 23

[:Margaret Leibovic]

Attachment: (Part 3) Add "force" parameter to Distribtion.init

I think it was bad that I was using this null check before to decide whether or not we should bail when state == STATE_NONE. Using this force parameter is much more explicit.

Comment 24

[:Margaret Leibovic]

Attachment: (Part 3) Clear distribution pref during testDistribution setup

This fixes the issue without messing with the signature of Distribution.init.

Comment 25

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8619eb3bdee1

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4ca3db39a8d
https://hg.mozilla.org/mozilla-central/rev/791937021ae5
https://hg.mozilla.org/mozilla-central/rev/8619eb3bdee1