Closed Bug 923109 Opened 8 years ago Closed 6 years ago

[robocop] Come up with a reliable way to load a url directly in a test


(Firefox for Android Graveyard :: General, defect)

Not set


(firefox39 fixed)

Firefox 39
Tracking Status
firefox39 --- fixed


(Reporter: Margaret, Assigned: gbrown)




(1 file, 1 obsolete file)

In bug 905591, we decided to use BaseTest.loadUrl instead of inputAndLoadUrl in PixelTest.loadAndGetPainted in order to reduce the input method flakiness associated with manually entering a url in the urlbar.

However, that caused a problem where sometimes the home pager would never be hidden, which resulted in the quite frequent failures seen in bug 910106.

One thing to think about is that I don't think we ever actually directly call Tabs.loadUrl to load a url in the current tab anywhere in our app code, so this may have issues that we don't know about.
The inputAndLoadUrl method (sending key events) was very reliable before the about:home re-write and I don't think anyone understands why that changed. Even if we opt to use Tabs.loadUrl or a similar solution for loading most pages efficiently, I think we should use inputAndLoadUrl for at least one test, to simulate and verify manual entry of urls. I would like to see someone determine the root cause of the inputAndLoadUrl flakiness.
Yeah, I definitely think we want at least one test to exercise the key events code path. I believe we just introduced the loadUrl method because it was faster and more direct, so it was useful for tests that just wanted a page to load, not to test the manual key entry path.

I'm still catching up on post-summit bugmail, but I haven't heard anything about intermittent failures caused by inputAndLoadUrl... maybe whatever issue caused bug 905591 somehow resolved itself?
(In reply to :Margaret Leibovic from comment #2)
> I'm still catching up on post-summit bugmail, but I haven't heard anything
> about intermittent failures caused by inputAndLoadUrl... maybe whatever
> issue caused bug 905591 somehow resolved itself?

I agree -- that does not seem to be an issue any longer. Hooray!
...but inputAndLoadUrl is still implicated in many robocop bugs:

"waiting for urlbar text to gain focus - urlbar text gained focus" like bug 1135172
NullPointerException in inputAndLoadUrl like bug 1118264

I think it will be easier to use loadUrl now that x86 robocop is not running.
Attached patch wip - seems promising (obsolete) — Splinter Review
Assignee: nobody → gbrown
This changes most instances of inputAndLoadUrl to loadUrl (or the new loadUrlAndWait). I left a few inputAndLoadUrl instances:
 - testAwesomebar as a single, simple case to exercise inputAndLoadUrl
 - testAddSearchEngine and testDistribution because these tests failed when I tried switching to loadUrl; maybe these can be dealt with in a follow-up
 - testHistory, testMasterPassword, etc because these tests are disabled and I don't want to modify them while they are disabled. suggests this patch does no harm, at least!
Attachment #8580302 - Attachment is obsolete: true
Attachment #8580723 - Flags: review?(margaret.leibovic)
Comment on attachment 8580723 [details] [diff] [review]
use loadUrl instead of inputAndLoadUrl in most instances

Review of attachment 8580723 [details] [diff] [review]:

If it's all green, works for me :)

::: mobile/android/base/tests/
@@ +23,5 @@
>          String blank1 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
>          String title = StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE;
>          // Loading a page
> +        loadUrl(blank1);

Why can we get away with loadUrl in some of these cases, but not loadUrlAndWait? Is the idea that these are cases where we don't care about the content of the page?
Attachment #8580723 - Flags: review?(margaret.leibovic) → review+
The strategy I used was to use loadUrl if it was followed by verifyUrlBarTitle or some other reliable wait-and-verify call; I used loadUrlAndWait otherwise (usually a simple waitForText followed). I did not check to see if loadUrl could be used in more cases -- this change seems risky enough!
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Do you want to post to mobile-firefox-dev about this change? This seems like a good thing for robocop test writers to know.
Flags: needinfo?(gbrown)
Thanks Margaret -- done!
Flags: needinfo?(gbrown)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.