947527 - Correct waitForPageIndex output for device

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

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

Currently, it uses Page.values()[index], which changes based on device - it should be updated to check for the device type. Alternatively, wait until bug 940565 and its siblings are implemented.

Note that the intermittent in bug 947177 should be updated as it will likely have a different message when this occurs.

Comment 1

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

Attachment: Correct waitForPageIndex output per device.

Comment 2

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

Comment on attachment 8344892 [details] [diff] [review]
Correct waitForPageIndex output per device.

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

Sharing the review love - feel free to hand it off to lucasr if you think he'd be a more appropriate reviewer.

Comment 3

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

I should note, I also took the liberty of adding some debug statements.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8344892 [details] [diff] [review]
Correct waitForPageIndex output per device.

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

LGTM, but let's using HomePager.Page if we can. The plan outlined in bug 942231 comment 13 is to try to kill off that enum eventually, so getting rid of a dependency here would be a nice start!

::: mobile/android/base/tests/components/AboutHomeComponent.java
@@ +123,5 @@
> +     * Gets the page index in the device specific Page enum for the given index in the
> +     * HomePager.Page enum.
> +     */
> +    private int getPageIndexForDevice(final int pageIndex) {
> +        final String pageName = Page.values()[pageIndex].name();

I think we should drop our dependency on HomePager.Page and just use the values in PhonePage/TabletPage instead.

Yes, this means we will need to update these enums if the set of default pages changes, but I actually think that's a fine thing to hard-code into a test, rather than pull from the application, since then the test checks to make sure the application is doing what we hope its doing.

I could imagine in the future we could have another test for extra pages that use our add-on APIs to add a page, then compare the set of current pages against some other expected enum.

Comment 5

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

> I think we should drop our dependency on HomePager.Page and just use the values
> in PhonePage/TabletPage instead.

(Without reading bug 942231 comment 13 because it seems outside the scope of this) The problem there is then when we assertCurrentPage(Page), we have to pass in specifically PhonePage or TabletPage. To fix this we could make a "PageType" enum, however, this is just duplicating information because HomePager.Page should actually be HomePager.PageType (or at least, that's how I assumed it would work here for this patch).

Comment 6

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #5)
> > I think we should drop our dependency on HomePager.Page and just use the values
> > in PhonePage/TabletPage instead.
> 
> (Without reading bug 942231 comment 13 because it seems outside the scope of
> this) The problem there is then when we assertCurrentPage(Page), we have to
> pass in specifically PhonePage or TabletPage. To fix this we could make a
> "PageType" enum, however, this is just duplicating information because
> HomePager.Page should actually be HomePager.PageType (or at least, that's
> how I assumed it would work here for this patch).

To re-iterate what I said on IRC yesterday, I didn't look closely enough at where we currently use HomePager.Page in our tests, so you're right that we can't get rid of it unless we rewrite assertCurrentyPage.

It's fine to land this patch as-is.

Comment 7

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

https://hg.mozilla.org/integration/fx-team/rev/399ecd6a6187

Comment 8

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

To prevent the excess messages to bug 947550 and tbpl.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/399ecd6a6187

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7375f34250ea