Closed Bug 959382 Opened 10 years ago Closed 10 years ago

Create BaseTest.setPreferenceAndWaitForChange helper method

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: mcomella, Assigned: vivek)

Details

(Whiteboard: [mentor=mcomella][lang=java])

Attachments

(1 file, 7 obsolete files)

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.
Can I work on this bug?
Flags: needinfo?(michael.l.comella)
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! ^_^
Assignee: nobody → vivekb.balakrishnan
Flags: needinfo?(michael.l.comella)
Thanks for the pointers, 
I've a working build now and will start working on the issue.
Hey, Vivek! Are you still working on this?
Flags: needinfo?(vivekb.balakrishnan)
Yes, I'm still working on this. Sorry for the delay and any inconveniences caused.
Flags: needinfo?(vivekb.balakrishnan)
No worries - there's no rush here! I just wanted to make sure you were still on it! :)
Attachment #8379342 - Flags: review?(michael.l.comella)
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.
Attachment #8379342 - Flags: review?(michael.l.comella) → review-
Attachment #8379342 - Attachment is obsolete: true
Attachment #8383239 - Flags: review?(michael.l.comella)
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.
Attachment #8383239 - Flags: review?(michael.l.comella) → feedback+
Attached patch 959382.patch (obsolete) — Splinter Review
Review comments incorporated
Attachment #8383239 - Attachment is obsolete: true
Attachment #8384795 - Flags: review?(michael.l.comella)
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!
Attachment #8384795 - Flags: review?(michael.l.comella) → review-
Attached patch patch with nits corrected (obsolete) — Splinter Review
Attachment #8384795 - Attachment is obsolete: true
Attachment #8387653 - Flags: review?(michael.l.comella)
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
Attachment #8387653 - Flags: review?(michael.l.comella) → review-
Attached patch 959382.patch (obsolete) — Splinter Review
patch correction as per discussion in irc.
patch builds successfully after a hg pull -u
Attachment #8387653 - Attachment is obsolete: true
Attachment #8388795 - Flags: review?(michael.l.comella)
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).
Attachment #8388795 - Flags: review?(michael.l.comella) → review-
Attached patch 959382.patch (obsolete) — Splinter Review
removed the ambiguous comment in isSatisfied, since, the code is self explainatory
Attachment #8388795 - Attachment is obsolete: true
Attachment #8388825 - Flags: review?(michael.l.comella)
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?
Attachment #8388825 - Flags: review?(michael.l.comella) → review-
Attached patch 959382.patch (obsolete) — Splinter Review
get and set preference uses the same preference name from json passed
Attachment #8388825 - Attachment is obsolete: true
Attachment #8390195 - Flags: review?(michael.l.comella)
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.
Attachment #8390195 - Flags: review?(michael.l.comella) → review-
Attached patch 959382.patchSplinter Review
changes according to review comments
Attachment #8390195 - Attachment is obsolete: true
Attachment #8390793 - Flags: review?(michael.l.comella)
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?
Attachment #8390793 - Flags: review?(michael.l.comella) → review+
As this is my first checkin, can you please help me with it
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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/9dfaae981a9b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [mentor=mcomella][lang=java] → [mentor=mcomella][lang=java][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9dfaae981a9b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=mcomella][lang=java][fixed-in-fx-team] → [mentor=mcomella][lang=java]
Target Milestone: --- → Firefox 30
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.