1247366 - NavigationHelper.goForward() fails with too many menu items

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[:Margaret Leibovic]

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.

Comment 2

[Geoff Brown [:gbrown]]

(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.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Let's see how the other tests behave if we just remove the waitForText. I'll push a patch to try.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98d3d2b34b13

Comment 5

[:Margaret Leibovic]

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.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3b3a45a8fca

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75874db2a8cc

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f0b5c7d5be3

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1247366 - AppMenuComponent.findAppMenuItemView(): Wait for views and not for text to appear. r?margaret

Review commit: https://reviewboard.mozilla.org/r/34919/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34919/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

This patch does not search for possibly not existing text (and scroll the menu) but instead searches (and waits for) the actual views.

Comment 11

[:Margaret Leibovic]

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.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/80398286fc25639999d8750a27526e63813f453c
Bug 1247366 - AppMenuComponent.findAppMenuItemView(): Wait for views and not for text to appear. r=margaret

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80398286fc25

Comment 15

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Thanks for taking care of this, Sebastian!

Comment 16

[:Margaret Leibovic]

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

Comment 17

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking affected for 46 in case the experiment goes live in 46.

Comment 18

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/10be70b85cb1