882191 - Android 2.2 opt builds take a second to load nested preference screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

This means that the preferences on the screen are not enabled when we try to click, so they fail to load.

failures on Android 2.2 opt (r2) - https://tbpl.mozilla.org/?tree=Try&rev=6dcd325fcc73
INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | checking clear button - clear button exists
INFO TEST-UNEXPECTED-FAIL | testMasterPassword | Exception caught - junit.framework.AssertionFailedError: No Button with text OK is found!

Fixed by adding 2 sec of sleep before clicking: https://tbpl.mozilla.org/?tree=Try&rev=27a89c8d6485

Comment 1

[Geoff Brown [:gbrown]]

To be clear, this only happens with the patches for bug 872329, right?

Comment 2

[Chenxia Liu [:liuche]]

Yes; also to note, adding a 2-second sleep is probably not the right answer, because that will make long tests take even longer.

Comment 3

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

Are we waiting for "Gecko:Ready" in those tests? In the app, we disable the "Settings" menu until "Gecko:Ready" message is sent.

Comment 4

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

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Are we waiting for "Gecko:Ready" in those tests? In the app, we disable the
> "Settings" menu until "Gecko:Ready" message is sent.

Yes, we seem to be waiting:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testMasterPassword.java.in#17
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testClearPrivateData.java.in#15

Comment 5

[Chenxia Liu [:liuche]]

The screenshot failure shows successful navigation into the Settings menu, so I don't think it's an issue with waiting for "Gecko:Ready" - we wouldn't be able to successfully click into the settings.

Anyways, after looking through some android/robotium docs, I think there's a non-awkward way to check for enabled/disabled, so I'll give that a try and close this bug if it works!

Comment 6

[Geoff Brown [:gbrown]]

I can see this happening on my Galaxy S: the Privacy settings are displayed, all text is visible, but some (all?) items are greyed-out for a second or two. The test waits in selectSettingsItem for "Clear private data", clicks on the still-disabled item, and then eventually times out. 

selectSettingsItem's mSolo.waitForText succeeds, and mSolo.clickOnText has no return value -- a sleep is the only obvious/simple solution.

I am hoping to write a "waitForEnabledText" function that can wait effectively...but have not succeeded yet.

Comment 7

[Geoff Brown [:gbrown]]

Attachment: wait for settings item to be enabled before clicking

This patch applies on top of the patches for bug 872329 and will need to land at the same time.


This patch adds BaseTest.waitForEnabledText, which acts like waitForText but also waits for the matched text view to be enabled.

waitForEnabledText allows selectSettingsItem to wait effectively for the Settings to be enabled before clicking -- that fixes testClearPrivateData.

I still encountered test failures in testMasterPassword, which seem to be unrelated: https://tbpl.mozilla.org/?tree=Try&rev=a497617d71e3. I have included fixes for those problems in this patch.

I also see failures in testSystemPages, but I think that is because Settings > Mozilla > About Fennec does not display the About Fennec page -- it just shows the Settings.

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

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 762908 [details] [diff] [review]
wait for settings item to be enabled before clicking

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

Looks good - I'm still a little skeptical about the TextView cast, but let's see what the try builds return and I'll see how these run on top of the settings patches (which introduce these timing problems in 2.2). So r+ upon addressing TextView and BACK comments (and pending try build of course).

I'll rebase on top of the patches for bug 872329, which have bit-rotted a tiny bit, and send off a try build. I will actually pull the changes in the testMasterPassword wrt to "Privacy" vs "Privacy & Security" into the test patch for that bug, so it's consistent. Sorry for missing those!

::: mobile/android/base/tests/BaseTest.java.in
@@ +409,5 @@
> +                // Solo.getText() could be used here, except that it sometimes
> +                // hits an assertion when the requested text is not found.
> +                ArrayList<View> views = mSolo.getCurrentViews();
> +                for (View view : views) {
> +                    if (view instanceof TextView) {

Does casting to a TextView work? I previously tried getText() which gets the TextView, but kept seeing errors because not all the things I was checking are TextViews. This could be work better when iterating through getCurrentViews(), though...

::: mobile/android/base/tests/testMasterPassword.java.in
@@ +73,5 @@
>          mAsserter.ok(mSolo.waitForText("^Use master password$"), "Checking if Use master password is present", "Use master password is present");
>          mSolo.clickOnText("^Use master password$");
>          mAsserter.ok(mSolo.waitForText("Remove Master Password"), "Checking if the password is enabled", "The password is enabled");
> +        clickOnButton("Cancel"); // Go back to Privacy settings
> +        waitForText("^Privacy$");

Thanks, I must have missed this!

@@ +74,5 @@
>          mSolo.clickOnText("^Use master password$");
>          mAsserter.ok(mSolo.waitForText("Remove Master Password"), "Checking if the password is enabled", "The password is enabled");
> +        clickOnButton("Cancel"); // Go back to Privacy settings
> +        waitForText("^Privacy$");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);// Close the Privacy settings

I've seen this in some places, but I don't think this works for testing on tablets because the top level of header categories is always visible on tablets, so we don't need to two levels of "BACK". Perhaps we should wrap this "BACK" in a "if devType==phone"?

@@ +109,5 @@
>          mSolo.clickOnText("^Use master password$");
>          mAsserter.ok(waitForText("^Create Master Password$"), "Checking if the password is disabled", "The password is disabled");
> +        clickOnButton("Cancel"); // Go back to Privacy settings
> +        waitForText("^Privacy$");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);// Close the Privacy settings

Same comment as above.

@@ +121,5 @@
>          mActions.sendSpecialKey(Actions.SpecialKey.BACK); // Close the VKB
>      }
>  
>      public void noDoorhangerDisplayed(String LOGIN_URL) {
> +        waitForText("Browser Blank Page 01|Enter Search or Address");

The use of waitForText and mSolo.waitForText seems inconsistent throughout files - which should be the correct call?

@@ +170,5 @@
>          mSolo.clickOnText("^Use master password$");
>          mAsserter.ok(mSolo.searchText("^Remove Master Password$"), "Checking if the master password was disabled by clearing private data", "The master password is not disabled by clearing private data");
>          clickOnButton("Cancel"); // Close the Master Password menu
> +        waitForText("^Privacy$");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);// Close the Privacy settings

Again, comment above re: tablets and headers and back.

Comment 9

[Geoff Brown [:gbrown]]

Attachment: wait for settings item to be enabled before clicking

Review comments addressed...

>> +                for (View view : views) {
>> +                    if (view instanceof TextView) {
>
> Does casting to a TextView work?

Yes! Here, "views" has all kinds of views; the instanceof check reduces that to TextViews, and then view can be casted just fine.

> I've seen this in some places, but I don't think this works for testing on 
> tablets because the top level of header categories is always visible on 
> tablets, so we don't need to two levels of "BACK". Perhaps we should wrap 
> this "BACK" in a "if devType==phone"?

Absolutely -- I have made that change in this patch.

At the same time, testing on tablets, I noticed the setting category is "Privacy & Security" on tablets -- updated for that also.

> The use of waitForText and mSolo.waitForText seems inconsistent throughout 
> files - which should be the correct call?

BaseTest.waitForText is just like mSolo.waitForText but also dumps a warning if the text is not found. I prefer to use BaseTest.waitForText unless:
 - I expect the text to not be found, or;
 - the caller is checking the return code and reporting an error/warning when the text is not found.


New try run is here: https://tbpl.mozilla.org/?tree=Try&rev=5033c81a3792


> I will actually pull the changes in the testMasterPassword wrt to "Privacy"
> vs "Privacy & Security" into the test patch for that bug, so it's consistent.

That makes sense. In fact, you may want to roll most/all of these changes into your patches -- whatever works for you!

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Patch: wait for settings item to be enabled before clicking v3

Thanks Geoff! I extracted a lot of the individual test fixes for settings that I'd missed into the tests patch for bug 872329, and I'm adding the fix for timing that you wrote into this bug. Will do a try push to make sure everything works!

Comment 11

[Chenxia Liu [:liuche]]

Try build on top of bug 872329: https://tbpl.mozilla.org/?tree=Try&rev=9d119ffdb171

This will have to land on top of bug 872329, and then we should be able to close bug 843947.

Comment 12

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8255862d8b2d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8255862d8b2d