935264 - [robocop] Add more test cases to testHomeBanner

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[:Margaret Leibovic]

Follow-up to bug 934678.

Comment 1

[:Margaret Leibovic]

Expanding the scope of this bug to account for some of the issues encountered with bug 960359.

So, things to test:
* Banner doesn't show up if there are no messages (both before a message is added, and after a message is removed)
* Banner goes away if you navigate to a new page

Comment 2

[:Margaret Leibovic]

Attachment: WIP

I commented out the chunks of this that are failing. Unfortunately, it looks like Solo just fails an assertion when it doesn't find a view, instead of returning a null view. Should I try doing findViewById instead?

Also, the test to swipe between panels isn't working. However, we plan to land an updated version of the patch in bug 960359, and if that happens we will actually be hiding the view itself, so maybe we can just wait for that (also, until that patch lands, this check isn't actually important).

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8374365 [details] [diff] [review]
WIP

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

Nice - it looks like it's going in the right direction

::: mobile/android/base/tests/helpers/NavigationHelper.java
@@ +37,5 @@
>          url = adjustUrl(url);
> +        enterAndLoadRawUrl(url);
> +    }
> +
> +    public static void enterAndLoadRawUrl(String url) {

I'm going to look into changing adjustUrl so that this is not necessary.

I think we can just check if the url starts with "/robocop/", and change the content then, rather than not changing "about:" links.

::: mobile/android/base/tests/testHomeBanner.java
@@ +19,5 @@
> +        // Make sure the banner is not visible.
> +        mAboutHome.assertVisible();
> +        assertBannerNotVisible();
> +
> +        // Load the roboextender page to add a message to the banner.

nit: helper method.

@@ +30,5 @@
> +        NavigationHelper.enterAndLoadUrl("about:home");
> +        eventExpecter.blockForEvent();
> +
> +        // Verify that the banner was shown with the correct text.
> +        assertTrue("correct text appeared in banner", getSolo().waitForText(TEXT));

I think we want to try to avoid using Solo directly in the tests. If we move the banner functionality to AboutHomeComponent, we can call `mAboutHome.waitForHomeBanner()`.

Also, we should probably use waitForView rather than waitForText (http://robotium.googlecode.com/svn/doc/index.html): http://robotium.googlecode.com/svn/doc/com/robotium/solo/Solo.html#waitForView%28java.lang.Class%29

@@ +39,5 @@
> +        assertBannerVisible();
> +        mAboutHome.swipeToPanelOnRight();
> +        assertBannerNotVisible();
> +        mAboutHome.swipeToPanelOnLeft();
> +        assertBannerVisible();

I'd swipe at least twice right, just to be safe.

I wonder if we shouldn't add a "swipe through all pages" method to AboutHomeComponent (then we could effortlessly swipe through all pages on startup, after installation, and after uninstallation).

@@ +43,5 @@
> +        assertBannerVisible();
> +*/
> +        // Test to make sure the onclick handler is called.
> +        eventExpecter = getActions().expectGeckoEvent("TestHomeBanner:MessageClicked");
> +        getSolo().clickOnText(TEXT);

Can we click on the view here instead? Also, preferably in AboutHomeComponent, like the comment above.

@@ +46,5 @@
> +        eventExpecter = getActions().expectGeckoEvent("TestHomeBanner:MessageClicked");
> +        getSolo().clickOnText(TEXT);
> +        eventExpecter.blockForEvent();
> +
> +/* XXX: Failing because view id is not found. Solo fails an assertion, rather than returning a null view.

So it seems! Yeah, maybe findViewId is the best choice here.

This makes me second-guess the UITest code because I had the assumption that it would return null - nice find! :P

If I find anything better after auditing the UITest code, I'll let you know.

@@ +52,5 @@
> +        // Verify that the banner isn't visible after navigating away from about:home.
> +        NavigationHelper.enterAndLoadUrl("about:firefox");
> +        assertBannerNotVisible();
> +*/
> +        // Load the roboextender page to remove the message from the banner.

nit: helper method.

@@ +65,3 @@
>      }
>  
> +    private void assertBannerVisible() {

We should move these assertions to AboutHomeComponent (there you can use mSolo).

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #3)
> I think we can just check if the url starts with "/robocop/", and change the
> content then, rather than not changing "about:" links.

Better yet, the link we're loading here is a "chrome:" url. Can you modify NavigationHelper.adjustUrl to not change the URL if the scheme is also "chrome:"? Do you see any reasons not to do that?

Comment 5

[:Margaret Leibovic]

Attachment: Add more test cases to testHomeBanner

I decided to keep it simple and leave out the panel swiping bit for now.

I followed your suggestions for moving things into AboutHomeComponent, and I like how clean this turned out.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=01883ddbd236

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8375851 [details] [diff] [review]
Add more test cases to testHomeBanner

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

We should also test X-ing out of the banner. File a followup?

lgtm w/ nits.

::: mobile/android/base/tests/components/AboutHomeComponent.java
@@ +15,5 @@
>  import com.jayway.android.robotium.solo.Solo;
>  
>  import android.support.v4.view.PagerAdapter;
>  import android.support.v4.view.ViewPager;
> +import android.widget.TextView;

nit: Alphabetize.

@@ +63,5 @@
>          return (ViewPager) mSolo.getView(R.id.home_pager);
>      }
>  
> +    private View getHomeBannerView() {
> +        return (View) mSolo.getView(R.id.home_banner);

nit: Unnecessary cast.

@@ +102,5 @@
> +
> +    public AboutHomeComponent assertBannerText(String text) {
> +        assertBannerVisible();
> +
> +        final TextView textView = (TextView) getHomeBannerView().findViewById(R.id.text);

Rather than casting here, you should probably have getHomeBannerView() return a TextView and cast there.

::: mobile/android/base/tests/testHomeBanner.java
@@ +16,5 @@
> +    public void testHomeBanner() {
> +        GeckoHelper.blockForReady();
> +
> +        // Make sure the banner is not visible.
> +        mAboutHome.assertVisible().assertBannerNotVisible();

nit: .assertVisible()
     .assertBannerNotVisible();

I think it's a bit more readable.

@@ +45,5 @@
> +        removeBannerMessage();
> +
> +        // Verify that the banner no longer appears.
> +        NavigationHelper.enterAndLoadUrl("about:home");
> +        mAboutHome.assertVisible().assertBannerNotVisible();

nit: .assert
     .assert

@@ +51,5 @@
>  
> +    /**
> +     * Loads the roboextender page to add a message to the banner.
> +     */
> +    private void addBannerMessage() {

To keep the actual test code cleaner and easier to read, I wonder if this shouldn't restore the initially loaded url after loading the roboextender page. It would limit the perceived side effects of calling addBannerMessage, though I imagine it can also be confusing (i.e. if we're still on about:home and a JS API adds a banner, does the HomeBanner appear or do we have to reload? - this distinction would be unclear).

If you go this route, you may want to do removeHomeBanner too.

@@ +53,5 @@
> +     * Loads the roboextender page to add a message to the banner.
> +     */
> +    private void addBannerMessage() {
> +        final Actions.EventExpecter eventExpecter = getActions().expectGeckoEvent("TestHomeBanner:MessageAdded");
> +        NavigationHelper.enterAndLoadUrl(TEST_URL + "#addMessage");

nit: The test would be less fragile if you passed in the banner text through the URL but it's probably not worth the time.

Comment 7

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #6)

> We should also test X-ing out of the banner. File a followup?

Should be handled by bug 963561.

> @@ +102,5 @@
> > +
> > +    public AboutHomeComponent assertBannerText(String text) {
> > +        assertBannerVisible();
> > +
> > +        final TextView textView = (TextView) getHomeBannerView().findViewById(R.id.text);
> 
> Rather than casting here, you should probably have getHomeBannerView()
> return a TextView and cast there.

The cast is for the findViewById part (the TextView inside the banner, not the banner itself).

> @@ +51,5 @@
> >  
> > +    /**
> > +     * Loads the roboextender page to add a message to the banner.
> > +     */
> > +    private void addBannerMessage() {
> 
> To keep the actual test code cleaner and easier to read, I wonder if this
> shouldn't restore the initially loaded url after loading the roboextender
> page. It would limit the perceived side effects of calling addBannerMessage,
> though I imagine it can also be confusing (i.e. if we're still on about:home
> and a JS API adds a banner, does the HomeBanner appear or do we have to
> reload? - this distinction would be unclear).
> 
> If you go this route, you may want to do removeHomeBanner too.

Yeah, I don't love that these helper methods have state implications, but I'm willing to live with it, since they're private and this test is small.

Comment 8

[:Margaret Leibovic]

Try run was green. Pushed with comments addressed:
https://hg.mozilla.org/integration/fx-team/rev/3dd09d1e89d8

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/3dd09d1e89d8

Comment 10

[:Margaret Leibovic]

Uplifted these test-only changes to aurora, since other patches I needed to uplift had tests that built on top of these (also, yay test coverage!).

https://hg.mozilla.org/releases/mozilla-aurora/rev/fcbc2e04b8c8