Closed Bug 896565 Opened 6 years ago Closed 5 years ago

[fig] Re-write testAwesomebarSwipes for new about:home

Categories

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

All
Android
defect

Tracking

()

RESOLVED WONTFIX
Tracking Status
fennec + ---

People

(Reporter: Margaret, Assigned: lucasr)

References

Details

(Whiteboard: abouthome-hackathon)

Attachments

(1 file, 1 obsolete file)

We should rename this test to testHomeSwipes or something like that, and update it to test swiping between the new about:home pages.
Assignee: nobody → lucasr.at.mozilla
Attachment #785835 - Flags: review?(margaret.leibovic)
Comment on attachment 785835 [details] [diff] [review]
Update testAwesomebarSwipes for new about:home

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

::: mobile/android/base/tests/testAboutHomeSwipes.java.in
@@ +27,4 @@
>  
>          // Test normal sliding of the list left and right
>          ViewPager pager = (ViewPager)mSolo.getView(ViewPager.class, 0);
> +        mAsserter.is(pager.getCurrentItem(), 1, "Bookmarks page is selected");

It could be nice to factor out these page indices to constants. It would make the test easier to read, and it would be easier to update the test if we ever change the order of pages.

@@ +58,3 @@
>          mAsserter.is(pager.getCurrentItem(), 1, "Clicking on tab selected bookmarks page");
>  
>          // Test typing in the awesomebar changes tabs and prevents panning

Nit: update comment to get rid of reference to awesomebar.

@@ +64,3 @@
>  
>          mSolo.scrollToSide(Solo.RIGHT);
> +        mAsserter.is(pager.getCurrentItem(), 1, "Dragging right is not allowed when searching");

When typing, aren't we showing the BrowserSearch UI instead of the page UI? I suppose it's true that the page underneath won't change, but maybe we should also add a check to make sure we're showing the BrowserSearch UI.
Attachment #785835 - Flags: review?(margaret.leibovic) → review+
tracking-fennec: --- → ?
Priority: -- → P1
Hardware: ARM → All
tracking-fennec: ? → 26+
(In reply to :Margaret Leibovic from comment #2)
> Comment on attachment 785835 [details] [diff] [review]
> Update testAwesomebarSwipes for new about:home
> 
> Review of attachment 785835 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/testAboutHomeSwipes.java.in
> @@ +27,4 @@
> >  
> >          // Test normal sliding of the list left and right
> >          ViewPager pager = (ViewPager)mSolo.getView(ViewPager.class, 0);
> > +        mAsserter.is(pager.getCurrentItem(), 1, "Bookmarks page is selected");
> 
> It could be nice to factor out these page indices to constants. It would
> make the test easier to read, and it would be easier to update the test if
> we ever change the order of pages.

Done.

> @@ +58,3 @@
> >          mAsserter.is(pager.getCurrentItem(), 1, "Clicking on tab selected bookmarks page");
> >  
> >          // Test typing in the awesomebar changes tabs and prevents panning
> 
> Nit: update comment to get rid of reference to awesomebar.

Done.
 
> @@ +64,3 @@
> >  
> >          mSolo.scrollToSide(Solo.RIGHT);
> > +        mAsserter.is(pager.getCurrentItem(), 1, "Dragging right is not allowed when searching");
> 
> When typing, aren't we showing the BrowserSearch UI instead of the page UI?
> I suppose it's true that the page underneath won't change, but maybe we
> should also add a check to make sure we're showing the BrowserSearch UI.

This is covered in the new testBrowserSearchVisibility.
Attachment #800265 - Flags: review?(margaret.leibovic)
Attachment #785835 - Attachment is obsolete: true
Had to rewrite it to account for latest UI changes.
Comment on attachment 800265 [details] [diff] [review]
Update testAwesomebarSwipes for new about:home

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

This is looking nice, I just have some questions.

::: mobile/android/base/tests/testAboutHomeSwipes.java.in
@@ +54,5 @@
> +    }
> +
> +    private ViewGroup getTabStripView() {
> +        if (TextUtils.equals(mDevice.type, "phone")) {
> +            // On phones, fetch tab strip by class type  

Trailing whitespace.

@@ +68,5 @@
> +    private void selectOnTabsStrip(ViewPager pager, ViewGroup tabStrip, int direction, int expectedPage) {
> +        // On phones, we use a pager strip, first child points to previous page,
> +        // last child points to next page. On tablets, we use a tab strip where
> +        // each child's position matches the corresponding page position in the
> +        // ViewPager. 

Trailing whitespace.

@@ +71,5 @@
> +        // each child's position matches the corresponding page position in the
> +        // ViewPager. 
> +        final int childIndex;
> +        if (TextUtils.equals(mDevice.type, "phone")) {
> +            childIndex = (direction == Solo.LEFT ? 0 : 2);

It doesn't look like this account for the current page index. How does this work for moving from the history page to the bookmarks page?

I'm actually a bit confused about why you need to pass a direction to this method at all, since it's about tapping on specific tabs, not swiping. Could you just pass the index in directly, perhaps using constants for the indices of different pages?

@@ +73,5 @@
> +        final int childIndex;
> +        if (TextUtils.equals(mDevice.type, "phone")) {
> +            childIndex = (direction == Solo.LEFT ? 0 : 2);
> +        } else {
> +            childIndex = pager.getCurrentItem() + (direction == Solo.LEFT ?  -1 : 1); 

Trailing whitespace.
Attachment #800265 - Flags: review?(margaret.leibovic) → feedback+
(In reply to :Margaret Leibovic from comment #7)
> Comment on attachment 800265 [details] [diff] [review]
> Update testAwesomebarSwipes for new about:home
> 
> Review of attachment 800265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is looking nice, I just have some questions.
> 
> ::: mobile/android/base/tests/testAboutHomeSwipes.java.in
> @@ +54,5 @@
> > +    }
> > +
> > +    private ViewGroup getTabStripView() {
> > +        if (TextUtils.equals(mDevice.type, "phone")) {
> > +            // On phones, fetch tab strip by class type  
> 
> Trailing whitespace.

Fixed.
 
> @@ +68,5 @@
> > +    private void selectOnTabsStrip(ViewPager pager, ViewGroup tabStrip, int direction, int expectedPage) {
> > +        // On phones, we use a pager strip, first child points to previous page,
> > +        // last child points to next page. On tablets, we use a tab strip where
> > +        // each child's position matches the corresponding page position in the
> > +        // ViewPager. 
> 
> Trailing whitespace.

Fixed.

> @@ +71,5 @@
> > +        // each child's position matches the corresponding page position in the
> > +        // ViewPager. 
> > +        final int childIndex;
> > +        if (TextUtils.equals(mDevice.type, "phone")) {
> > +            childIndex = (direction == Solo.LEFT ? 0 : 2);
> 
> It doesn't look like this account for the current page index. How does this
> work for moving from the history page to the bookmarks page?

The phone tab strip usually has three children. Tapping on the leftmost title takes you the previous tab, tapping on the rightmost one, takes you to the next tab. There's no way to directly access the Nth tab in the phone UI. 

> I'm actually a bit confused about why you need to pass a direction to this
> method at all, since it's about tapping on specific tabs, not swiping. Could
> you just pass the index in directly, perhaps using constants for the indices
> of different pages?

See explanation above. The phone's tab strip only allows you to go to next/previous tab. The tablet UI, on the other hand, allows direct access to specific tabs.

> @@ +73,5 @@
> > +        final int childIndex;
> > +        if (TextUtils.equals(mDevice.type, "phone")) {
> > +            childIndex = (direction == Solo.LEFT ? 0 : 2);
> > +        } else {
> > +            childIndex = pager.getCurrentItem() + (direction == Solo.LEFT ?  -1 : 1); 
> 
> Trailing whitespace.

Fixed.
(In reply to Lucas Rocha (:lucasr) from comment #8)

> > @@ +71,5 @@
> > > +        // each child's position matches the corresponding page position in the
> > > +        // ViewPager. 
> > > +        final int childIndex;
> > > +        if (TextUtils.equals(mDevice.type, "phone")) {
> > > +            childIndex = (direction == Solo.LEFT ? 0 : 2);
> > 
> > It doesn't look like this account for the current page index. How does this
> > work for moving from the history page to the bookmarks page?
> 
> The phone tab strip usually has three children. Tapping on the leftmost
> title takes you the previous tab, tapping on the rightmost one, takes you to
> the next tab. There's no way to directly access the Nth tab in the phone UI. 

Oh, I get it now. I actually just wrote out a long comment explaining why I was confused, then figured it out. I think it would be worthwhile to add a comment explaining that reasoning here :)

I'm curious though, when you're on the history tab, does the tab strip have the same 3 children, just no value for the one at the 0 index?
Comment on attachment 800265 [details] [diff] [review]
Update testAwesomebarSwipes for new about:home

r+ because you clarified that this is doing the right thing. Just please add a comment before landing it!
Attachment #800265 - Flags: feedback+ → review+
We've got most test ported to new about:home. No need to keep test-related bugs in Fx26 at this point.
tracking-fennec: 26+ → +
testAwesomebarSwipes isn't even in the tree anymore... I'm just going to call this a WONTFIX.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.