Closed Bug 1247366 Opened 7 years ago Closed 7 years ago

NavigationHelper.goForward() fails with too many menu items

Categories

(Firefox for Android Graveyard :: Testing, defect)

defect
Not set
normal

Tracking

(firefox46 fixed, firefox47 fixed, fennec47+)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
fennec 47+ ---

People

(Reporter: Margaret, Assigned: sebastian)

References

Details

Attachments

(1 file)

Breaking this off from bug 1234693. This is a problem with adding History and Bookmarks menu items to our main menu.

In that case, testSessionHistory is failing.

From bug 1234693:

It looks like the problem here is the waitForExactText() in findAppMenuItemView():
https://dxr.mozilla.org/mozilla-central/rev/2dfb45d74f42d2a0010696f5fd47c7a7da94cedb/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java#162

Robotium's waitForText() will search the text in the UI and also scroll components to find it. In this case we are looking for the text "Forward" and even with scrolling the menu we won't find it. After that we are searching the view ourselves and are not only looking for the visible text but also inside contentDescription. This also has the side effect that every time we are searching for forward/back we will always wait for 7.5s (MAX_WAITTIME_FOR_MENU_UPDATE_IN_MS) because Robotium is trying to find the text first and is failing.

The test passes after removing the line (I do not know if a different test case might require the scrolling) and it's a bit faster too.

Another option would be to not use Robotium's search and just scroll up -> search view, scroll down -> search view - assuming that the menu will never be super long. And the perfect solution probably is to implement a searchText with Robotiums tools that will look into contentDescription too.
If either of you feel inspired to write a patch for this, that would help me greatly :)

Otherwise I'll try to write a fix for this some time this week. This blocks the bookmark/history menu item experiment, which is currently blocking us from updating the switchboard server URLs, so I want to get this fixed sooner rather than later.
tracking-fennec: --- → ?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(michael.l.comella)
(In reply to :Margaret Leibovic from comment #0)
> It looks like the problem here is the waitForExactText() in
> findAppMenuItemView():

Calling a wait function in findAppMenuItemView makes me uncomfortable anyway: That's going to be called in isMenuOpen(), which is called by waitForMenuOpen. So we'll be looping and waiting on a Condition, and the Condition may block significantly on each call...I wonder how the timing will work out and if it will have unexpected consequences.
Let's see how the other tests behave if we just remove the waitForText. I'll push a patch to try.
Flags: needinfo?(s.kaspari)
This is blocking us from landing the history/bookmarks menu items, so regardless of the switchboard situation, we'll need to address this to land that change. I would be fine with disabling testSessionHistory for 47 if we need to, but we'll want to fix this eventually.
Assignee: nobody → s.kaspari
tracking-fennec: ? → 47+
Status: NEW → ASSIGNED
This patch does not search for possibly not existing text (and scroll the menu) but instead searches (and waits for) the actual views.
Comment on attachment 8719276 [details]
MozReview Request: Bug 1247366 - AppMenuComponent.findAppMenuItemView(): Wait for views and not for text to appear. r?margaret

https://reviewboard.mozilla.org/r/34919/#review31761

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java:124
(Diff revision 1)
>       * TODO: ^ Maybe we just need to have opened the "More" menu and the current one doesn't matter.

Maybe this fix addresses this TODO item.
Attachment #8719276 - Flags: review?(margaret.leibovic) → review+
(In reply to :Margaret Leibovic from comment #11)
> mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/
> AppMenuComponent.java:124
> (Diff revision 1)
> >       * TODO: ^ Maybe we just need to have opened the "More" menu and the current one doesn't matter.
> 
> Maybe this fix addresses this TODO item.

This looks like a relic we should have removed in bug 1228170. I think since we tried to unify the menus there's no "More" menu anymore.
https://hg.mozilla.org/integration/fx-team/rev/80398286fc25639999d8750a27526e63813f453c
Bug 1247366 - AppMenuComponent.findAppMenuItemView(): Wait for views and not for text to appear. r=margaret
https://hg.mozilla.org/mozilla-central/rev/80398286fc25
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Thanks for taking care of this, Sebastian!
Flags: needinfo?(michael.l.comella)
Blocks: 1250289
Comment on attachment 8719276 [details]
MozReview Request: Bug 1247366 - AppMenuComponent.findAppMenuItemView(): Wait for views and not for text to appear. r?margaret

Approval Request Comment

[Feature/regressing bug #]: bookmark/history menu experiment (bug 1231792)

[User impact if declined]: without this change, tests fail when our experiment goes live (bug 1247366)

[Describe test coverage new/current, TreeHerder]: this is just a test change

[Risks and why]: low-risk, only a test change

[String/UUID change made/needed]: none
Attachment #8719276 - Flags: approval-mozilla-aurora?
Marking affected for 46 in case the experiment goes live in 46.
Comment on attachment 8719276 [details]
MozReview Request: Bug 1247366 - AppMenuComponent.findAppMenuItemView(): Wait for views and not for text to appear. r?margaret

Test only change, ok to uplift to aurora
Attachment #8719276 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.