841252 - [Robocop] Tests for localized distribution preferences

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

There are three things here:

1) localized "distribution.about"
2) "LocalizablePreferences", which substitute the device's locale for %LOCALE%
3) localized "LocalizablePreferences", which override the prefs specified in "LocalizablePreferences" for a specific locale

Here are the contents of an example preferences.json that would do all three of these things:

{
  "Global": {
    "id": "testpartner",
    "version": 1.0,
    "about": "Afiliado de Prueba",
    "about.en-US": "Test Partner"
  },

  "LocalizablePreferences": {
    "distribution.test.localizeable": "http://test.org/%LOCALE%/%LOCALE%/",
    "distribution.test.localizeable-override": "http://test.org/%LOCALE%/%LOCALE%/"
  },

  "LocalizablePreferences.en-US": {
    "distribution.test.localizeable-override": "http://cheese.com"
  }
}

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Let's start by asking for feedback :)

I think this is close to what you're looking for.

Comment 2

[Mark Capella [:capella]]

Attachment: New mock-package.zip file

For review with the patch.

Comment 3

[:Margaret Leibovic]

Comment on attachment 718668 [details] [diff] [review]
Patch (v1)

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

Nice! Thanks for working on this.

As a point of general feedback, have you considered just making this part of testDistribution? It looks like there's a decent amount of duplicated code that would be nice to share. Perhaps you can refactor testDistribution() to call a testPreferences() method, then cleans up things it needs to, then calls a testLocalizedPreferences() method? This would definitely require a bunch of refactoring, but I think it would be best to share as much stuff as possible.

::: mobile/android/base/tests/testLocaleDistribution.java.in
@@ +67,5 @@
> +        }
> +
> +        mAsserter.is(getPref(TEST_PREF_ABOUT), "Afiliado de Prueba", "check " + TEST_PREF_ABOUT);
> +        mAsserter.is(getPref(TEST_PREF_LOCALIZEABLE), "http://test.org/es-MX/es-MX/", "check " + TEST_PREF_LOCALIZEABLE);
> +        mAsserter.is(getPref(TEST_PREF_LOCALIZEABLE_OVERRIDE), "http://queso.com", "check " + TEST_PREF_LOCALIZEABLE_OVERRIDE);

I appreciate the attempt to factor out this getPref method, but it feels like a lot of overhead to send a separate preferences message for each of these preferences, when instead you could just send one message to get them all at once, similarly to how we currently do it in testDistribution.

@@ +79,5 @@
> +    }
> +
> +    // Sets the distribution locale preference for the test
> +    private void setTestLocale() {
> +        String TEST_REQUEST_ID = "setTestLocale";

As a convention, we usually reserve all-caps for static member variables, so it confused me that this was a local variable (especially that it's used in another method as well). I also think that if the constants are only referenced from within the same method (like TEST_LOCALE and TEST_LOCALE_PREF), they can just be declared as local variables in the method.

Comment 4

[Mark Capella [:capella]]

I created a new test since I found I had to set the locale pref, then do an entire distribution install test (like the first test ... mockpackage and all) for the prefs to be set localized.

This seemed logically cohesive enough to package as a single discrete test that cookie cuts generic functions from other tests (testDistribution vs. testLocaleDistribution).

This strikes me as better, but I acknowledge eventually needing your approval. I'm also aware of that last distribution test bug 841255, though I don't know where it would fit in this (one or two) tests.

The other things you mention I have no problem with changing.

Further thoughts?

Comment 5

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #4)
> I created a new test since I found I had to set the locale pref, then do an
> entire distribution install test (like the first test ... mockpackage and
> all) for the prefs to be set localized.
> 
> This seemed logically cohesive enough to package as a single discrete test
> that cookie cuts generic functions from other tests (testDistribution vs.
> testLocaleDistribution).

I understand the logic for creating a separate test, but looking at this patch, so much of the code is duplicated that I'd really prefer not to land this as-is. The things you're sharing are:

* the logic to call Distribution.init() (you could split this out into a separate method)
* clearDistributionPref()
* getMockPackagePath()
* setUp()
* tearDown()

If you think it's too messy to put this together into a single test, we could also explore making an abstract DistributionTest class that both these tests can inherit from, but that also feels a bit heavy-handed.

> This strikes me as better, but I acknowledge eventually needing your
> approval. I'm also aware of that last distribution test bug 841255, though I
> don't know where it would fit in this (one or two) tests.

I'm also not sure exactly what we should do about that one. I extended ContentProviderTest assuming we would use that to test the bookmark logic, but we'd have to make sure to set up the distribution before setting up the content provider. In any case, testing this would also require all the same logic for setting up/clearing a distribution.

As an example of a test that uses shared methods to test a whole bunch of different things, you could have a look at testBrowserProvider.

Comment 6

[Mark Capella [:capella]]

Attachment: Patch (v2)

Combining the tests looks a little like this ...

Comment 7

[:Margaret Leibovic]

Comment on attachment 718815 [details] [diff] [review]
Patch (v2)

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

Nice, I like this a lot more. I'll leave the few comments I have now before you go to rebase!

::: mobile/android/base/tests/testDistribution.java.in
@@ +85,5 @@
> +    private void clearDistributionPref() {
> +        SharedPreferences settings = mActivity.getSharedPreferences("GeckoApp", Activity.MODE_PRIVATE);
> +        String keyName = mActivity.getPackageName() + ".distribution_state";
> +        settings.edit().remove(keyName).commit();
> +    }

To reduce diff churn, could you keep getMockPackagePath() and clearDistributionPref() down below? If you think it's better to reorganize, it would be easier to review if that's in a separate patch.

@@ +195,5 @@
> +                data = new JSONObject(eventExpecter.blockForEventData());
> +                requestId = data.getString("requestId");
> +            }
> +
> +        } catch (Exception ex) { 

Nit: Trailing whitespace. Also s/ex/e/ to be consistent with the other catch statements in here.

@@ +238,5 @@
> +                    mAsserter.is(pref.getString("value"), "Afiliado de Prueba", "check " + prefAbout);
> +                } else if (name.equals(prefLocalizeable)) {
> +                    mAsserter.is(pref.getString("value"), "http://test.org/es-MX/es-MX/", "check " + prefLocalizeable);
> +                } else if (name.equals(prefLocalizeableOverride)) {
> +                    mAsserter.is(pref.getString("value"), "http://queso.com", "check " + prefLocalizeableOverride);

Another thing to think about (which might be better to do in a follow-up bug), is coming up with a way to test the "LocalizablePreferences.<locale>" section. That section is used to override prefs in the "LocalizablePreferences" section for a specific locale. I used en-US to test this locally, but you can update mock-package.zip if you'd like.

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v3)

Re-based, all requested changes done / tested and it looks prettier ... :P

re: |coming up with a way to test the "LocalizablePreferences.<locale>" section| ... is this something that was covered in the comment 0 above?

I don't mind getting something else useful in here, but have to admit my very incomplete knowledge of the localization processes, and just to get this far I mostly felt my way through it.

Maybe you can elaborate a little more?

Comment 9

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #8)

> re: |coming up with a way to test the "LocalizablePreferences.<locale>"
> section| ... is this something that was covered in the comment 0 above?

Yeah, that was point 3 in the description.

> I don't mind getting something else useful in here, but have to admit my
> very incomplete knowledge of the localization processes, and just to get
> this far I mostly felt my way through it.
> 
> Maybe you can elaborate a little more?

I'm sorry, I know this isn't well documented. The logic is based on what desktop did with distribution.ini [1]. I still need to finish fleshing out our documentation [2], but to really understand exactly what's happening, you can read the code here:
http://hg.mozilla.org/mozilla-central/file/0a91da5f5eab/mobile/android/chrome/content/browser.js#l8239

[1] https://wiki.mozilla.org/Distribution_INI_File
[2] https://wiki.mozilla.org/Mobile/Distribution_Files#Preferences

Comment 10

[:Margaret Leibovic]

Comment on attachment 719267 [details] [diff] [review]
Patch (v3)

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

This is looking really nice. I just have a few nits about variable names and error messages.

Let me know if you need help figuring out how to test the localized pref override.

::: mobile/android/base/tests/testDistribution.java.in
@@ +28,5 @@
>   *     bookmarks.json
>   */
>  public class testDistribution extends ContentProviderTest {
>      private static final String MOCK_PACKAGE = "mock-package.zip";
> +    private static final String TEST_DISTRIBUTION_REQUEST_ID = "testDistribution";

TEST_DISTRIBUTION is redundant here, since we're in the testDistribution class. How about PREF_REQUEST_ID? Since that makes it clearer what this is for.

@@ +47,3 @@
>          clearDistributionPref();
> +        initDistribution(mockPackagePath);
> +        runTestDistribution();

This method is testing preferences, so I think it would be clearer if it was named something like "testPreferences" (once again, I think putting distribution in the name is redundant).

@@ +51,5 @@
> +        // Pre-clear distribution pref, and run Localized Distribution Test
> +        clearDistributionPref();
> +        setTestLocale("es-MX");
> +        initDistribution(mockPackagePath);
> +        runTestLocalizedDistribution();

And then this method could be something like "testLocalizedPreferences".

@@ +163,5 @@
> +                requestId = data.getString("requestId");
> +            }
> +
> +        } catch (Exception e) {
> +            mAsserter.ok(false, "exception setting preferences locale", e.toString());

Nit: s/preferences/test/

@@ +234,5 @@
> +
> +            mockPackagePath = outFile.getPath();
> +
> +        } catch (Exception e) {
> +            mAsserter.ok(false, "exception installing mock distribution package", e.toString());

Nit: We're not really installing anything here. I think a better message would be something like "exception copying mock distribution package to data directory".

Comment 11

[Mark Capella [:capella]]

Attachment: Patch (v4)

See if these tests perform a little more explicitly ... I'm (cautiously) optimistic so I've flagged for review?  ;)

Comment 12

[Mark Capella [:capella]]

Attachment: Patch (v5)

I renamed testPreferences() and testLocalizedPreferences() to checkPreferences() and checkLocalizedPreferences() as I noticed in the output log that JUnit has some issues with private members whose name start with testFoo.... Our tests would work ok, but we'de get messages like:

org.mozilla.fennec.tests.testDistribution:.
junit.framework.TestSuite$1:
Failure in warning:
junit.framework.AssertionFailedError: Test method isn't public: testPreferences(org.mozilla.fennec.tests.testDistribution)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:190)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:175)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1619)

Test results for FennecInstrumentationTestRunner=..F
Time: 7.199

FAILURES!!!
Tests run: 2,  Failures: 1,  Errors: 0


Simply changing testPreferences() and testLocalizedPreferences() to public, provoked odd behaviour, involving our tests failing as if methods were being called out of order. (???)

Anyhow, it's late and this is new to me so I'll withdraw the review? request and look at JUnit and this more tomorrow.


FYI, the new tests that I believe incorporate the third item from the original comment 0 request look like:

Robocop derived process name: org.mozilla.fennec
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testDistribution
2 INFO TEST-PASS | testDistribution | check distribution.id - test-partner should equal test-partner
3 INFO TEST-PASS | testDistribution | check distribution.about - Test Partner should equal Test Partner
4 INFO TEST-PASS | testDistribution | check distribution.version - 1 should equal 1
5 INFO TEST-PASS | testDistribution | check distribution.test.boolean - true should equal true
6 INFO TEST-PASS | testDistribution | check distribution.test.string - test should equal test
7 INFO TEST-PASS | testDistribution | check distribution.test.int - 5 should equal 5
8 INFO TEST-PASS | testDistribution | check distribution.about - Test Partner should equal Test Partner
9 INFO TEST-PASS | testDistribution | check distribution.test.localizeable - http://test.org/en-US/en-US/ should equal http://test.org/en-US/en-US/
10 INFO TEST-PASS | testDistribution | check distribution.test.localizeable-override - http://cheese.com should equal http://cheese.com
11 INFO TEST-PASS | testDistribution | check distribution.about - Afiliado de Prueba should equal Afiliado de Prueba
12 INFO TEST-PASS | testDistribution | check distribution.test.localizeable - http://test.org/es-MX/es-MX/ should equal http://test.org/es-MX/es-MX/
13 INFO TEST-PASS | testDistribution | check distribution.test.localizeable-override - http://test.org/es-MX/ should equal http://test.org/es-MX/
14 INFO TEST-PASS | testDistribution | clean up mock package - deleted /data/data/org.mozilla.fennec/mock-package.zip
15 INFO TEST-PASS | testDistribution | clean up distribution files - deleted /data/data/org.mozilla.fennec/distribution/preferences.json
16 INFO TEST-PASS | testDistribution | clean up distribution files - deleted /data/data/org.mozilla.fennec/distribution/bookmarks.json
17 INFO TEST-PASS | testDistribution | clean up distribution directory - deleted distribution directory
18 INFO TEST-END | testDistribution | finished in 5049ms
19 INFO TEST-START | Shutdown
20 INFO Passed: 16
21 INFO Failed: 0
22 INFO Todo: 0
23 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:00:10.109541
INFO | automation.py | Reading PID log: /tmp/tmpJrUg6Ppidlog
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

INFO | runtests.py | Running tests: end.
INFO | runtests.py | Test summary: start.
0 INFO SimpleTest START
19 INFO TEST-START | Shutdown
20 INFO Passed: 16
21 INFO Failed: 0
22 INFO Todo: 0
23 INFO SimpleTest FINISHED
INFO | runtests.py | Test summary: end.

Comment 13

[Mark Capella [:capella]]

Comment on attachment 719822 [details] [diff] [review]
Patch (v5)

This looks to be a JUnit 3 limitation ...
http://grepcode.com/file/repo1.maven.org/maven2/junit/junit/4.11/junit/framework/TestSuite.java#TestSuite.%3Cinit%3E%28%29

The attached may be the correct implementation ...

Comment 14

[:Margaret Leibovic]

Comment on attachment 719822 [details] [diff] [review]
Patch (v5)

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

This is looking pretty good. Wanna push to try to see how it does?

::: mobile/android/base/tests/testDistribution.java.in
@@ +219,5 @@
> +                    if (aLocale.equals("en-US"))
> +                        mAsserter.is(pref.getString("value"), "http://cheese.com", "check " + prefLocalizeableOverride);
> +                    else
> +                    if (aLocale.equals("es-MX"))
> +                        mAsserter.is(pref.getString("value"), "http://test.org/es-MX/", "check " + prefLocalizeableOverride);

It feels like it would be nice to abstract away the expected values for each locale, rather than including a second if/else inside each existing if/else block.

However, if you are going to go this route, please use braces and keep the "else if" on one line, to stay consistent with out other styles.

Comment 15

[Mark Capella [:capella]]

push to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=da24b60f8169

Comment 16

[Mark Capella [:capella]]

hmmm ... lotsa of these in the logs .... 
03-01 18:50:18.145 D/GeckoAppShell(10582): Gecko event sync taking too long: 1364ms
03-01 18:50:18.145 D/Robocop (10582): unblocked on expecter for Preferences:Data
03-01 18:50:18.145 D/Robocop (10582): unblocked on expecter for Preferences:Data
03-01 18:50:18.145 D/Robocop (10582): unblocked on expecter for Preferences:Data

Comment 17

[Mark Capella [:capella]]

Comment on attachment 719822 [details] [diff] [review]
Patch (v5)

Other than that all looks good ...

Comment 18

[:Margaret Leibovic]

(In reply to Mark Capella [:capella] from comment #16)
> hmmm ... lotsa of these in the logs .... 
> 03-01 18:50:18.145 D/GeckoAppShell(10582): Gecko event sync taking too long:
> 1364ms
> 03-01 18:50:18.145 D/Robocop (10582): unblocked on expecter for
> Preferences:Data
> 03-01 18:50:18.145 D/Robocop (10582): unblocked on expecter for
> Preferences:Data
> 03-01 18:50:18.145 D/Robocop (10582): unblocked on expecter for
> Preferences:Data

That's bug 846158.

Comment 19

[:Margaret Leibovic]

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

> However, if you are going to go this route, please use braces and keep the
> "else if" on one line, to stay consistent with out other styles.

Not sure if you missed this comment. Address this, and then I'll give you an r+ :)

Comment 20

[Mark Capella [:capella]]

Attachment: Patch (v6)

Ah sorry ... the changeset I pushed to TRY had the correction, but I didn't repost as attachment for the review? request ...

Comment 21

[:Margaret Leibovic]

Comment on attachment 720580 [details] [diff] [review]
Patch (v6)

Nice.

Comment 22

[Mark Capella [:capella]]

and awaaaaaaaaaaaaaaaaaaay we go!
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd362dcf94a5

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd362dcf94a5