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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[Geoff Brown [:gbrown]]

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.

Comment 2

[:Margaret Leibovic]

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?

Comment 3

[Geoff Brown [:gbrown]]

(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!

Comment 4

[Geoff Brown [:gbrown]]

...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.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: wip - seems promising

Comment 6

[Geoff Brown [:gbrown]]

Attachment: use loadUrl instead of inputAndLoadUrl in most instances

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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43b1dea0971e suggests this patch does no harm, at least!

Comment 7

[: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/testTitleBar.java
@@ +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?

Comment 8

[Geoff Brown [:gbrown]]

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!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e609a7d24a15

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[:Margaret Leibovic]

Do you want to post to mobile-firefox-dev about this change? This seems like a good thing for robocop test writers to know.

Comment 11

[Geoff Brown [:gbrown]]

Thanks Margaret -- done!