880060 - [fig] Update BaseTest to work with new about:home

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[:Margaret Leibovic]

We're getting rid of the AwesomeBar activity in the new about:home, so we need to update our test infrastructure accordingly.

Comment 1

[:Margaret Leibovic]

Changing the scope of this bug to just be about fixing the tests.

Comment 2

[:Margaret Leibovic]

Attachment: WIP to update BaseTest and disable tests that still won't pass

This patch updates BaseTest to no longer depend on the awesomebar activity. This actually fixes most of the failing tests, and I disabled the ones that will need more work to fix (e.g. the tests that actually test the bookmarks/history UI). I think it would be nice to just land something like this, then fix those tests in follow-ups as we make more progress on actually implementing the UI.

I'll push this to try once it opens again.

Comment 3

[:Margaret Leibovic]

https://tbpl.mozilla.org/?tree=Try&rev=3b2d843d166a

Comment 4

[Geoff Brown [:gbrown] (pto Jul 1-19)]

Comment on attachment 772410 [details] [diff] [review]
WIP to update BaseTest and disable tests that still won't pass

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +175,4 @@
>       */
> +    protected final void focusUrlBar() {
> +        // Click on the browser toolbar to enter editing mode
> +        mDriver.findElement(mActivity, BROWSER_TOOLBAR_ID).click();

That's lovely -- nice and simple!

@@ +179,3 @@
>  
> +        // Wait for highlighed text to gain focus
> +        mSolo.sleep(1000);

And that's disappointing! I know it is better than it has been, but still...

This is core functionality for robocop and I would really like to see us get it right this time. Is there some way to check for the focus gain, maybe in a waitForTest?

@@ +184,5 @@
>      protected final void enterUrl(String url) {
> +        focusUrlBar();
> +
> +        // Delete the current URL
> +        mActions.sendKeyCode(KeyEvent.KEYCODE_DEL);

Just for my curiosity: Is the DEL necessary? Better than just typing over the existing url?

::: mobile/android/base/tests/robocop.ini
@@ +4,4 @@
>  [testAwesomebar]
> +# [testAwesomebarSwipes] # disabled on fig - bug 880060
> +# [testBookmark] # disabled on fig - bug 880060
> +# [testBookmarklets] # disabled on fig - bug 880060

This is a lot of disabled tests. I think that's fine for fig, but I would not want to see these disabled on m-c.

::: mobile/android/base/tests/testAboutPage.java.in
@@ +19,5 @@
>          // Load the about: page and verify its title
>          String url = "about:";
>          loadAndPaint(url);
>  
> +        Element awesomebar = mDriver.findElement(getActivity(), URL_BAR_TITLE_ID);

s/awesomebar/urlbar/ ??

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 772410 [details] [diff] [review]
WIP to update BaseTest and disable tests that still won't pass

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

Just nits, looks good. We might want to think about the consistency of urlbar/awesomebar (entrybar?) - I guess the url stuff is strictly "url entry" versus "search engine" or "non-url input". But this is all naming nits.

::: mobile/android/base/tests/BaseTest.java.in
@@ +191,5 @@
>          mActions.sendKeys(url);
> +
> +        // Get the URL text from the url_edit_text EditText view
> +        String urlbarText = mDriver.findElement(mActivity, URL_EDIT_TEXT_ID).getText();
> +        mAsserter.is(urlbarText, url, "URL typed properly");

Even though this matches the style throughout the rest of BaseTest, I think "is" is technically supposed to be is(expected, found). Maybe rename urlbarText to something that's more clearly "foundUrl" or something.

@@ +203,5 @@
>          contentEventExpecter.unregisterListener();
>      }
>  
>      /**
> +     * Load <code>url</code> using the URL bar UI and sending key strokes.

clarity nit: "Load url by sending key strokes to the URL bar UI"

@@ +242,3 @@
>              // wait for a short time for the expected text, in case there is a delay
>              // in updating the view
> +            waitForTest(new VerifyUrlTest(urlEditText, url), VERIFY_URL_TIMEOUT);

You could possibly use a modified version of waitForEnabledText(String text, int timeout) here, or even just a modified waitForText.

@@ +255,2 @@
>              mUrl = url;
>          }

Might as well add a newline here.

Comment 6

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #4)
 
> @@ +179,3 @@
> >  
> > +        // Wait for highlighed text to gain focus
> > +        mSolo.sleep(1000);
> 
> And that's disappointing! I know it is better than it has been, but still...
> 
> This is core functionality for robocop and I would really like to see us get
> it right this time. Is there some way to check for the focus gain, maybe in
> a waitForTest?

Yeah, I wasn't really happy when writing this, but found I needed to do something or else I found robocop would start entering text before the current text was highlighted, causing a concatenated URL. I don't see any examples in our robocop tests for waiting for focus, do you have any suggestions about a good way to do this?

> @@ +184,5 @@
> >      protected final void enterUrl(String url) {
> > +        focusUrlBar();
> > +
> > +        // Delete the current URL
> > +        mActions.sendKeyCode(KeyEvent.KEYCODE_DEL);
> 
> Just for my curiosity: Is the DEL necessary? Better than just typing over
> the existing url?

Good point, the delete isn't necessary. I must have just put that in there when I was trying to figure out the raciness issue. It works fine without it, so I'll get rid of it.

> ::: mobile/android/base/tests/robocop.ini
> @@ +4,4 @@
> >  [testAwesomebar]
> > +# [testAwesomebarSwipes] # disabled on fig - bug 880060
> > +# [testBookmark] # disabled on fig - bug 880060
> > +# [testBookmarklets] # disabled on fig - bug 880060
> 
> This is a lot of disabled tests. I think that's fine for fig, but I would
> not want to see these disabled on m-c.

Yeah, I don't want us to be merging back to m-c in this state. I tried to just disable tests that had non-trivial dependencies on the current awesomescreen UI, and I'd like to see us update/rewrite these tests as we implement more of the new about:home. We can make that a blocker for mergining, but I think it's a large enough task that it would be best to do it in smaller bugs (and other people can jump in to help out :).

> ::: mobile/android/base/tests/testAboutPage.java.in
> @@ +19,5 @@
> >          // Load the about: page and verify its title
> >          String url = "about:";
> >          loadAndPaint(url);
> >  
> > +        Element awesomebar = mDriver.findElement(getActivity(), URL_BAR_TITLE_ID);
> 
> s/awesomebar/urlbar/ ??

Good call, these naming conventions are such a mess!

Comment 7

[:Margaret Leibovic]

Some more try pushes (the second one hopefully addresses some of the failures from the first):
https://tbpl.mozilla.org/?tree=Try&rev=71ccf197cef6
https://tbpl.mozilla.org/?tree=Try&rev=15fa6dbba69f

I filed bug 891579 about the crash that seems to pop up often.

Comment 8

[Geoff Brown [:gbrown] (pto Jul 1-19)]

(In reply to :Margaret Leibovic from comment #6)
> Yeah, I wasn't really happy when writing this, but found I needed to do
> something or else I found robocop would start entering text before the
> current text was highlighted, causing a concatenated URL. I don't see any
> examples in our robocop tests for waiting for focus, do you have any
> suggestions about a good way to do this?

Poll (via waitForTest) for View.hasFocus()? I do not know if that will work...worth a try?

Comment 9

[Adrian Tamas (:AdrianT)]

(In reply to :Margaret Leibovic from comment #6)
> (In reply to Geoff Brown [:gbrown] from comment #4)
>  
> > @@ +179,3 @@
> > >  
> > > +        // Wait for highlighed text to gain focus
> > > +        mSolo.sleep(1000);
> > 
> > And that's disappointing! I know it is better than it has been, but still...
> > 
> > This is core functionality for robocop and I would really like to see us get
> > it right this time. Is there some way to check for the focus gain, maybe in
> > a waitForTest?
> 
> Yeah, I wasn't really happy when writing this, but found I needed to do
> something or else I found robocop would start entering text before the
> current text was highlighted, causing a concatenated URL. I don't see any
> examples in our robocop tests for waiting for focus, do you have any
> suggestions about a good way to do this?

I did not test this but maybe just waiting for "Visited" or "Bookmarks" could work and add enough of a delay and also add a check that the about home is opened. Also in normal m-c the SearchEngines:Data Gecko event is triggered when entering the awesomebar and maybe blocking for it may work.

Comment 10

[:Margaret Leibovic]

Attachment: patch

I updated focusUrlBar to use waitForTest to check if the EditText has focus, which seems to work. I pushed to try to see what happens:
https://tbpl.mozilla.org/?tree=Try&rev=60b0df097639

I feel like we should really just land *something* here sooner rather than later, and we can work to incrementally improve/re-enable the other tests. Almost anything is an improvement over the current state of affairs :/

Comment 11

[:Margaret Leibovic]

https://hg.mozilla.org/projects/fig/rev/f248fe823002

I'll file some follow-ups for re-enabling the tests, and we can file more follow-ups to fix any other failures as we see them pop up on tbpl. In fact, we should start starring the failures on fig with bug numbers. I'll post to the mailing list about that.

Comment 12

[:Margaret Leibovic]

I'm updating the summary of this bug to reflect what it really did.

I filed bug 895673 as a meta bug for fixing and re-enabling the disabled tests.

Comment 13

[Ed Morley [:emorley]]

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