959382 - Create BaseTest.setPreferenceAndWaitForChange helper method

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

As discovered in bug 958836 comment 7, there are several tests that set some preference and wait for that result to change. This should be factored out into a helper method.

However, there are some flaws with the existing (copy and pasted) code, enumerated in bug 958836 comment 6. They are:
  * ourRequestId should be the value it's least likely to be (i.e. Integer.MIN_VALUE, though if this method is used multiple times in succession, we might want to decrement from 0 or so).
  * We should loop a limited number of times when waiting for the appropriate "Preferences:Data" event, to prevent hitting excessive timeout durations
  * The parameters in the returned json data object should be asserted for the expected values

It seems the copy and pasted methods all have the variable `ourRequestId` in common: https://mxr.mozilla.org/mozilla-central/search?string=ourrequestid&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

The implementer may want to create a similar method for the UITest class, or file a followup.

Comment 1

[Vivek Balakrishnan[:vivek]]

Can I work on this bug?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Sure! I've assigned the bug to you.

Have you already set up a build environment? If not, you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android

The tests you would be fixing are in our Robocop test framework, so you'll need to set that up too: https://wiki.mozilla.org/Auto-tools/Projects/Robocop

When you make changes in the mobile/android/base/tests/ directory, you should only need to recompile Robocop, not the entire browser, for these changes.

While you're developing, I recommend running the tests on a single method at a time because the full robocop test suite takes a long time to run (at least 30 minutes).

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

Comment 3

[Vivek Balakrishnan[:vivek]]

Thanks for the pointers, 
I've a working build now and will start working on the issue.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Hey, Vivek! Are you still working on this?

Comment 5

[Vivek Balakrishnan[:vivek]]

Yes, I'm still working on this. Sorry for the delay and any inconveniences caused.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

No worries - there's no rush here! I just wanted to make sure you were still on it! :)

Comment 7

[Vivek Balakrishnan[:vivek]]

Attachment: BaseTest.setPreferenceAndWaitForChange helper method

Comment 8

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8379342 [details] [diff] [review]
BaseTest.setPreferenceAndWaitForChange helper method

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

Generally the right direction, but I think there are some big things to look over - see the comments below! If you have questions about the motivations for these changes, or if you disagree with them, let me know!

::: mobile/android/base/tests/BaseTest.java
@@ +880,5 @@
> +
> +    /**
> +    * Helper function to wait for confirmation of the pref change before proceeding with the test.
> +    */
> +    public JSONArray setPreferenceAndWaitForChange(final JSONObject jsonPref, String[] prefNames, String sendEvent, String expectEvent) {

expectEvent seems to be the same across all calls - why not just hard-code it (use a private static final).

@@ +890,5 @@
> +                mActions.sendGeckoEvent(sendEvent, null);
> +            }
> +        }
> +
> +        final int ourRequestId = Integer.MIN_VALUE;

Thinking over it, as I alluded to, I think it's best to initialize this value to 0 and decrement it every request.

RequestID is currently only set by PrefsHelper.sUniqueRequestId which is initialized to 1 [1] and increments [2]. We don't want to collide so we should start at 0 and decrement, dealing with the integer overflow when it ever becomes a problem.

Sorry for the confusion.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/PrefsHelper.java?rev=533642b8d6df#27
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/PrefsHelper.java?rev=533642b8d6df#46

@@ +893,5 @@
> +
> +        final int ourRequestId = Integer.MIN_VALUE;
> +        final Actions.RepeatedEventExpecter eventExpecter = mActions.expectGeckoEvent(expectEvent);
> +        mActions.sendPreferencesGetEvent(ourRequestId, prefNames);
> +            

nit: There's a lot of excess whitespace added by this patch (end of line, or on empty lines, as in this case) - do you mind removing it? I'll write `nit: ws` in this file where you've added some, but I'll leave the others up to you.

You can add 

[color]
diff.trailingwhitespace = red_background

or similar to your ~/.hgrc and run `hg diff` to better spot these. Avoid removing pre-existing whitespace because it changes the hg history in undesired ways.

@@ +894,5 @@
> +        final int ourRequestId = Integer.MIN_VALUE;
> +        final Actions.RepeatedEventExpecter eventExpecter = mActions.expectGeckoEvent(expectEvent);
> +        mActions.sendPreferencesGetEvent(ourRequestId, prefNames);
> +            
> +        //Wait until we get the correct "Preferences:Data" event

nit: `// Wait ...`

@@ +902,5 @@
> +
> +            @Override
> +            public boolean isSatisfied() {
> +                try {
> +                    data = new JSONObject(eventExpecter.blockForEventData());

blockForEventData will timeout for its default, no matter how much time we left on the counter - that means if we wait 89999ms the first time around, we'd have to wait another 90000ms for this call and 89999ms more than our constant above (which is a bit misleading). Use `blockForEventDataWithTimeout(long)` and keep the timeout up to date!

@@ +904,5 @@
> +            public boolean isSatisfied() {
> +                try {
> +                    data = new JSONObject(eventExpecter.blockForEventData());
> +                    requestId = data.getInt("requestId");
> +                    prefArr.put(data);                    

Why don't you just return the data of the proper request id? Why are you storing it in an array?

nit: ws

@@ +905,5 @@
> +                try {
> +                    data = new JSONObject(eventExpecter.blockForEventData());
> +                    requestId = data.getInt("requestId");
> +                    prefArr.put(data);                    
> +                    

nit: ws

@@ +907,5 @@
> +                    requestId = data.getInt("requestId");
> +                    prefArr.put(data);                    
> +                    
> +                    return requestId == ourRequestId;
> +                } catch(Exception e) {

You should specifically try to catch the exceptions that you know may be thrown here (e.g. JSONException) rather than catching everything.

@@ +908,5 @@
> +                    prefArr.put(data);                    
> +                    
> +                    return requestId == ourRequestId;
> +                } catch(Exception e) {
> +                    mAsserter.ok(false, "exception in setPreferenceAndWaitForChange", e.toString());

nit: Where the exception is found should be seen in the stack trace, just pass in e, and leave the last string field blank (it's not useful and I'm not sure why it's there).

@@ +910,5 @@
> +                    return requestId == ourRequestId;
> +                } catch(Exception e) {
> +                    mAsserter.ok(false, "exception in setPreferenceAndWaitForChange", e.toString());
> +                }
> +                return false;        

Put this in the catch clause so it's more apparent why it's necessary (if javac lets you). Add an inline comment to the effect of "// javac needs this to compile".

nit: ws

@@ +915,5 @@
> +            }
> +        }, MAX_WAIT_BLOCK_FOR_EVENT_DATA_MS);
> +
> +        eventExpecter.unregisterListener();
> +        return prefArr;    

nit: ws

::: mobile/android/base/tests/testAddonManager.java
@@ +66,5 @@
> +            JSONArray preferences = prefData.getJSONArray("preferences");
> +            JSONObject prefs = (JSONObject) preferences.get(0);
> +            mAsserter.is(jsonPref.get("name"), prefs.getString("name"), "Preferences:name");
> +            mAsserter.is(jsonPref.get("type"), prefs.getString("type"), "Preference:type");
> +            mAsserter.is(jsonPref.get("value"), prefs.getString("value"), "Preference:value");

Do these assertions in the BaseTest method.

Also, make the messages more specific, e.g. "Expecting returned preference value to be the same as the set value".

::: mobile/android/base/tests/testDistribution.java
@@ -120,5 @@
>                                           prefTestBoolean,
>                                           prefTestString,
>                                           prefTestInt };
>  
> -            Actions.RepeatedEventExpecter eventExpecter = mActions.expectGeckoEvent("Preferences:Data");

Ah, I'm sorry, but I missed that this does not actually set the preference - it only gets the preference! You shouldn't need to use the method here (which would alleviate the need for the "sendEvent" parameter).

::: mobile/android/base/tests/testDoorHanger.java
@@ +134,5 @@
>              // Revert offline setting
>              JSONObject jsonPref = new JSONObject();
>              jsonPref.put("name", "offline-apps.allow_by_default");
> +            jsonPref.put("type", "bool");
> +            jsonPref.put("value", String.valueOf(offlineAllowedByDefault));

Why did you change the values put into jsonPref?

@@ +137,5 @@
> +            jsonPref.put("type", "bool");
> +            jsonPref.put("value", String.valueOf(offlineAllowedByDefault));
> +            String[] prefNames = {"offline-apps.allow_by_default"};
> +
> +            //set and wait for preference data to change

nit: This comment is redundant to the method name so you can remove it.

Comment 9

[Vivek Balakrishnan[:vivek]]

Attachment: Bug 959382 review comments incorporated

Comment 10

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8383239 [details] [diff] [review]
Bug 959382 review comments incorporated

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

The timing code looks pretty good now - it's mostly nits left!

::: mobile/android/base/tests/BaseTest.java
@@ +66,4 @@
>  
>      private static Class<Activity> mLauncherActivityClass;
>      private Activity mActivity;
> +    private int mRequestId;

Might as well be explicit with the initialization here.

nit: mRequestID

@@ +879,5 @@
>  
>          return null;
>      }
> +
> +    /*

nit: /**

javadoc.

@@ +890,5 @@
> +        final int ourRequestId = mRequestId--;
> +        final Actions.RepeatedEventExpecter eventExpecter = mActions.expectGeckoEvent("Preferences:Data");
> +        mActions.sendPreferencesGetEvent(ourRequestId, prefNames);
> +
> +        //Wait until we get the correct "Preferences:Data" event

nit: "// Wait"

@@ +892,5 @@
> +        mActions.sendPreferencesGetEvent(ourRequestId, prefNames);
> +
> +        //Wait until we get the correct "Preferences:Data" event
> +        waitForCondition(new Condition() {
> +            int requestId = -1;

nit: requestID

@@ +899,5 @@
> +
> +            @Override
> +            public boolean isSatisfied() {
> +                try {
> +                    long timeout = endTime - SystemClock.uptimeMillis();

Use `SystemClock.elapsedRealtime()`, here and above.

It's possible that this will get called after the official timeout time and timeout will be < 0. Our code eventually calls Object.wait, which will throw in this case [1]. Clamp the value to 0.

[1]: https://developer.android.com/reference/java/lang/Object.html#wait%28long%29

@@ +902,5 @@
> +                try {
> +                    long timeout = endTime - SystemClock.uptimeMillis();
> +                    data = new JSONObject(eventExpecter.blockForEventDataWithTimeout(timeout));
> +                    requestId = data.getInt("requestId");
> +                    if (requestId == ourRequestId) {

For better readability (imo), do:

if (requestID != ourRequestID) {
  return false;
}

// Other code.
return true;

@@ +907,5 @@
> +                        JSONArray preferences = data.getJSONArray("preferences");
> +                        mAsserter.is(1, preferences.length(), "Expecting preference array to have one element");
> +                        JSONObject prefs = (JSONObject) preferences.get(0);
> +                        mAsserter.is(prefs.get("name"), jsonPref.getString("name"),
> +                            "Expecting returned preference name to be the same as the set name");

nit: These new lined elements should be indented two under.

::: mobile/android/base/tests/testDoorHanger.java
@@ +77,5 @@
>  
>  
>          boolean offlineAllowedByDefault = true;
> +        // Save offline-allow-by-default preferences first
> +        final String[] prefNames = { "offline-apps.allow_by_default" };

Nice cleanup, but we might as well cleanup more while we're at it. Pull everything out of the `try` until something throws JSONException.

@@ +104,5 @@
>              // Turn off offline-allow-by-default
>              JSONObject jsonPref = new JSONObject();
>              jsonPref.put("name", "offline-apps.allow_by_default");
>              jsonPref.put("type", "bool");
> +            jsonPref.put("value", "false");

Insert this as a boolean, not a String

@@ +134,5 @@
>              // Revert offline setting
>              JSONObject jsonPref = new JSONObject();
>              jsonPref.put("name", "offline-apps.allow_by_default");
>              jsonPref.put("type", "boolean");
> +            jsonPref.put("value", String.valueOf(offlineAllowedByDefault));

Insert this as a boolean.

Comment 11

[Vivek Balakrishnan[:vivek]]

Attachment: 959382.patch

Review comments incorporated

Comment 12

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8384795 [details] [diff] [review]
959382.patch

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

Looks pretty good! We need to fail on JSONException, but besides that, the rest are nits!

::: mobile/android/base/tests/BaseTest.java
@@ +66,4 @@
>  
>      private static Class<Activity> mLauncherActivityClass;
>      private Activity mActivity;
> +    private int mRequestID = 0;

nit: mRequestID -> mPreferenceRequestID

Sorry, I didn't catch this before. I just realized it's otherwise ambiguous.

@@ +893,5 @@
> +
> +        // Wait until we get the correct "Preferences:Data" event
> +        waitForCondition(new Condition() {
> +            int requestID = -1;
> +            JSONObject data = null;

nit: Declare both of the variables above in the isSatisfied scope.

@@ +894,5 @@
> +        // Wait until we get the correct "Preferences:Data" event
> +        waitForCondition(new Condition() {
> +            int requestID = -1;
> +            JSONObject data = null;
> +            long endTime = SystemClock.elapsedRealtime() + MAX_WAIT_BLOCK_FOR_EVENT_DATA_MS;

nit: Declare this final.

@@ +908,5 @@
> +                    requestID = data.getInt("requestId");
> +                    if (requestID != ourRequestID) {
> +                        // Condition not satisfied, wait until timeout
> +                        return false;
> +                    } else {

nit: Since we return in the first `if`, you don't need the `else` here. Note also this lets you add a newline below the closing space, increasing readability (imo).

@@ +920,5 @@
> +                        mAsserter.is(prefs.get("value"), jsonPref.get("value"),
> +                          "Expecting returned preference value to be the same as the set value");
> +                        return true;
> +                    }
> +                } catch(JSONException e) {

nit: `} catch (JSONException e) {`

@@ +921,5 @@
> +                          "Expecting returned preference value to be the same as the set value");
> +                        return true;
> +                    }
> +                } catch(JSONException e) {
> +                    mAsserter.dumpLog("Exception in setPreferenceAndWaitForChange", e);

We should fail if we catch a JSONException, right? Seeing your java compiler comment, I think you meant to (:) !) but `dumpLog` does not fail, it just prints output. Use `mAsserter.ok(false, "a message", "");` instead.

::: mobile/android/base/tests/testDoorHanger.java
@@ +81,5 @@
> +        final String[] prefNames = { "offline-apps.allow_by_default" };
> +        final int ourRequestId = 0x7357;
> +        JSONObject jsonPref = new JSONObject();
> +        JSONObject data = null;
> +        int requestId = -1;

nit: I'd rather variable declaration be done inside the try if it needs to be later initialized in the try anyway. Does that make sense? This especially applies to jsonPref.

@@ +87,2 @@
>              Actions.RepeatedEventExpecter eventExpecter = mActions.expectGeckoEvent("Preferences:Data");
>              mActions.sendPreferencesGetEvent(ourRequestId, prefNames);

You should also pull out these methods because they won't throw a JSONException. The things to pull out of trys are usually anything that does a considerable amount of work.

The idea of pulling things out of a try are to make it clear what might throw the Exception - the fewer complex operations, the better!

Comment 13

[Vivek Balakrishnan[:vivek]]

Attachment: patch with nits corrected

Comment 14

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8387653 [details] [diff] [review]
patch with nits corrected

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

Looking pretty good - almost there!

Make sure you have pulled the latest code so there are no issues pushing this upstream.

I pushed this to our "try" test servers so we can make sure this changeset runs properly:

https://tbpl.mozilla.org/?tree=Try&rev=59494cbe70f2

::: mobile/android/base/tests/BaseTest.java
@@ +880,5 @@
>          return null;
>      }
> +
> +    /**
> +     * Helper function to wait for confirmation of the pref change before proceeding with the test.

nit: You don't need to say it's a helper method - just say what it does.

@@ +913,5 @@
> +                    JSONArray preferences = data.getJSONArray("preferences");
> +                    mAsserter.is(1, preferences.length(), "Expecting preference array to have one element");
> +                    JSONObject prefs = (JSONObject) preferences.get(0);
> +                    mAsserter.is(prefs.get("name"), jsonPref.getString("name"),
> +                      "Expecting returned preference name to be the same as the set name");

nit: This indent should be 8 spaces (two indent levels) beyond mAsserter. Same with the assertions below.

@@ +920,5 @@
> +                    mAsserter.is(prefs.get("value"), jsonPref.get("value"),
> +                      "Expecting returned preference value to be the same as the set value");
> +                    return true;
> +                } catch(JSONException e) {
> +                    mAsserter.ok(false, "Exception in setPreferenceAndWaitForChange", "");

We should return the exception too. It seems the final String is used to print out exceptions sometimes (e.g. [1]) so let's do that.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/BaseTest.java?rev=b77f7a5ba31e#152

Comment 15

[Vivek Balakrishnan[:vivek]]

Attachment: 959382.patch

patch correction as per discussion in irc.
patch builds successfully after a hg pull -u

Comment 16

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8388795 [details] [diff] [review]
959382.patch

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

One last nit! :)

::: mobile/android/base/tests/BaseTest.java
@@ +928,5 @@
> +
> +                    JSONObject data = new JSONObject(eventExpecter.blockForEventDataWithTimeout(timeout));
> +                    int requestID = data.getInt("requestId");
> +                    if (requestID != ourRequestID) {
> +                        // Condition not satisfied, wait until timeout

nit: Not sure what you mean by this. We're not waiting for the timeout - isSatisfied will get called again, so we're just checking for more data. I'd add that to the comment instead if you want to be explicitly (though I think the code speaks for itself, for the most part).

Comment 17

[Vivek Balakrishnan[:vivek]]

Attachment: 959382.patch

removed the ambiguous comment in isSatisfied, since, the code is self explainatory

Comment 18

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8388825 [details] [diff] [review]
959382.patch

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

Patch would be good but my try push from comment 14 seems to have failed (https://tbpl.mozilla.org/?tree=Try&rev=59494cbe70f2).

Can you take a look?

Comment 19

[Vivek Balakrishnan[:vivek]]

Attachment: 959382.patch

get and set preference uses the same preference name from json passed

Comment 20

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8390195 [details] [diff] [review]
959382.patch

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

Very close, mostly nits! Sorry there have been so many review passes, but I think this should be the last one! Thanks for hanging in there!

try: https://tbpl.mozilla.org/?tree=Try&rev=bf7a9f8d5f04

::: mobile/android/base/tests/BaseTest.java
@@ +908,5 @@
> +     */
> +    public void setPreferenceAndWaitForChange(final JSONObject jsonPref) {
> +        mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString());
> +
> +        // Get the preference name from the json

Tip: You mentioned on IRC that you thought that catching the Exception was ugly and hard to read. In that case, it's usually a good idea to add a helper method that implements the eye sore code.

Another time when I find it's a good idea to use a helper method is when I feel the need to add a comment to explain how code works - the comment can usually become a method name and any additional information that the comment conveys can be written as method javadoc. I find this to be more readable, and better at conveying more information.

Of course, these are guidelines and should not always be followed - use your judgement!

It's not necessary here, but I thought I'd throw those ideas out for your edification.

@@ +909,5 @@
> +    public void setPreferenceAndWaitForChange(final JSONObject jsonPref) {
> +        mActions.sendGeckoEvent("Preferences:Set", jsonPref.toString());
> +
> +        // Get the preference name from the json
> +        String[] prefNames = new String[1];

Either add a comment to explain that we store the string in an array here so we can pass it to a method later on, or just store a string here and construct the array when you send the message.

@@ +913,5 @@
> +        String[] prefNames = new String[1];
> +        try {
> +            prefNames[0] = jsonPref.getString("name");
> +        } catch (JSONException e) {
> +            mAsserter.ok(false, "Exception in setPreferenceAndWaitForChange", BaseTest.getStackTraceString(e));

You shouldn't need to say, "BaseTest.".

@@ +940,5 @@
> +                        return false;
> +                    }
> +
> +                    JSONArray preferences = data.getJSONArray("preferences");
> +                    mAsserter.is(1, preferences.length(), "Expecting preference array to have one element");

These parameters should be swapped (1 and preferences.length()), otherwise the debug output is incorrect.

`mAsserter.is(actual, expected`

See https://mxr.mozilla.org/mozilla-central/source/build/mobile/robocop/Assert.java#7

@@ +950,5 @@
> +                    mAsserter.is(prefs.get("value"), jsonPref.get("value"),
> +                            "Expecting returned preference value to be the same as the set value");
> +                    return true;
> +                } catch(JSONException e) {
> +                    mAsserter.ok(false, "Exception in setPreferenceAndWaitForChange", BaseTest.getStackTraceString(e));

Again, remove BaseTest.

Comment 21

[Vivek Balakrishnan[:vivek]]

Attachment: 959382.patch

changes according to review comments

Comment 22

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Comment on attachment 8390793 [details] [diff] [review]
959382.patch

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

Nice! Good work and thanks for your help!

Do you know about the checkin-needed keyword?

Comment 23

[Vivek Balakrishnan[:vivek]]

As this is my first checkin, can you please help me with it

Comment 24

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

Sure. Since you don't have commit access, you have to have someone commit the patch for you. If you add the string, "checkin-needed" to the "Keywords" field, someone will come along and commit your patch to the main repo.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9dfaae981a9b

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9dfaae981a9b