896574 - [fig] Fix and re-enable testAddSearchEngine

Status: RESOLVED FIXED

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

Comment 1

[Adrian Tamas (:AdrianT)]

Assigning the test to myself since I created it in the first place and made the re-write for 869277

Comment 2

[Adrian Tamas (:AdrianT)]

Attachment: testAddSearchEngine fix try 1

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=1a9b1dab04a4

This passes locally with no issues for me. Unfortunately if I don't run mActions.sendGeckoEvent("SearchEngines:Get", null) after any page load when I wait for the SearchEngine:Data event the blockForEvent times out. Even with long sleeps(30 sec +) on the debug build it takes way to much time to show the engines the first time. Once the engines are shown they are updated without any issues. I'll continue to look into this.

Comment 3

[Adrian Tamas (:AdrianT)]

Attachment: testAddSearchEngine fix

Try run: https://tbpl.mozilla.org/?tree=Try&rev=3b315078af5f

With the first try fix it seems that mSolo.clickLongOnScreen fails a sometimes on pandas (Android 4.0 in general) - known robotium issue: http://code.google.com/p/robotium/issues/detail?id=343. If I run the test multiple times consecutively locally on a FIG build on the Samsung Galaxy Tab 2 The test passes ~ 2 runs and then the next 2-3 it fails to click correctly then again 2-3 runs pass and so on. 

Since we can't upgrade to robotium 4.0 because of the instability ((bug 855978) I choose to add the implementation of clicker.clickLongOnScreen from Robotium 4.0 in the FennecNativeActions as a method and use that to clickLongOnScreen. It works locally very time (~60+ tries).

Adding the feedback flag for Geoff to get his input on this solution and also the need to send the SearchEngines:Get gecko event in order not to timeout on blockForEventData.

Comment 4

[Geoff Brown [:gbrown]]

Comment on attachment 780395 [details] [diff] [review]
testAddSearchEngine fix

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

The clickLongOnScreen work-around seems sensible; we can un-do it when we upgrade robotium.

Are there other tests that use clickLongOnScreen? Maybe we should switch those over at the same time.

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +497,5 @@
> +
> +        eventTime = SystemClock.uptimeMillis();
> +        event = MotionEvent.obtain(downTime, eventTime, MotionEvent.ACTION_MOVE, x + 1.0f, y + 1.0f, 0);
> +        mInstr.sendPointerSync(event);
> +        mSolo.sleep(3000);

It looks to me like robotium uses:
  ViewConfiguration.getLongPressTimeout() * 2.5f
which seems more robust -- why the difference?

@@ +502,5 @@
> +
> +        eventTime = SystemClock.uptimeMillis();
> +        event = MotionEvent.obtain(downTime, eventTime, MotionEvent.ACTION_UP, x, y, 0);
> +        mInstr.sendPointerSync(event);
> +        mSolo.sleep(3000);

It looks to me like robotium only waits for 500 ms here -- why the difference?

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +87,5 @@
>          focusUrlBar();
> +        mActions.sendKeys(SEARCH_TEXT);
> +
> +        // This is needed because it takes a lot of time to display the search engines in the automation build for the first time
> +        // It takes more then the blockForEventData timeout

That timeout is 90 seconds -- it seems like something is not working right here. If you don't send SearchEngines:Get, but wait for more than 90 s (with blockForEventDataWithTimeout), is SearchEngines:Data eventually received?

Comment 5

[Adrian Tamas (:AdrianT)]

(In reply to Geoff Brown [:gbrown] from comment #4)
> Comment on attachment 780395 [details] [diff] [review]
> testAddSearchEngine fix
> 
> Review of attachment 780395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The clickLongOnScreen work-around seems sensible; we can un-do it when we
> upgrade robotium.
> 
> Are there other tests that use clickLongOnScreen? Maybe we should switch
> those over at the same time.
As far as I know only testWebContextMenu test uses this method. I can change it there to but will have to wait for the re-write of the test in order to be re-enabled. I will also check for others that may use this.

> ::: build/mobile/robocop/FennecNativeActions.java.in
> @@ +497,5 @@
> > +
> > +        eventTime = SystemClock.uptimeMillis();
> > +        event = MotionEvent.obtain(downTime, eventTime, MotionEvent.ACTION_MOVE, x + 1.0f, y + 1.0f, 0);
> > +        mInstr.sendPointerSync(event);
> > +        mSolo.sleep(3000);
> 
> It looks to me like robotium uses:
>   ViewConfiguration.getLongPressTimeout() * 2.5f
> which seems more robust -- why the difference?
> 
> @@ +502,5 @@
> > +
> > +        eventTime = SystemClock.uptimeMillis();
> > +        event = MotionEvent.obtain(downTime, eventTime, MotionEvent.ACTION_UP, x, y, 0);
> > +        mInstr.sendPointerSync(event);
> > +        mSolo.sleep(3000);
> 
> It looks to me like robotium only waits for 500 ms here -- why the
> difference?
I'll change the sleeps and retest. I just added the 3000 ms because it seemed a reasonable amount of time.
> ::: mobile/android/base/tests/testAddSearchEngine.java.in
> @@ +87,5 @@
> >          focusUrlBar();
> > +        mActions.sendKeys(SEARCH_TEXT);
> > +
> > +        // This is needed because it takes a lot of time to display the search engines in the automation build for the first time
> > +        // It takes more then the blockForEventData timeout
> 
> That timeout is 90 seconds -- it seems like something is not working right
> here. If you don't send SearchEngines:Get, but wait for more than 90 s (with
> blockForEventDataWithTimeout), is SearchEngines:Data eventually received?
I have added a 30 sec sleep before blocking for event. I'll check with more. Generally there has been an issue with the showing of the search engines on debug builds. As you know there were issues on this test with that from the beginning and this was the reason for the rewrite to use the gecko event to validate the number of search engines. 

When you manually use the build it takes 30+ seconds for the search engines to be displayed. When this is done using automation for some reason it is even slower. I'll also have to look at the implementation of the new about:home since it may react differently to typing text in the url bar and sending them via code. I saw a similar issue with the editable textfield of Find in Page were it would not search until I pressed enter.

Comment 6

[Adrian Tamas (:AdrianT)]

> > ::: mobile/android/base/tests/testAddSearchEngine.java.in
> > @@ +87,5 @@
> > >          focusUrlBar();
> > > +        mActions.sendKeys(SEARCH_TEXT);
> > > +
> > > +        // This is needed because it takes a lot of time to display the search engines in the automation build for the first time
> > > +        // It takes more then the blockForEventData timeout
> > 
> > That timeout is 90 seconds -- it seems like something is not working right
> > here. If you don't send SearchEngines:Get, but wait for more than 90 s (with
> > blockForEventDataWithTimeout), is SearchEngines:Data eventually received?
> I have added a 30 sec sleep before blocking for event. I'll check with more.
> Generally there has been an issue with the showing of the search engines on
> debug builds. As you know there were issues on this test with that from the
> beginning and this was the reason for the rewrite to use the gecko event to
> validate the number of search engines. 
> 
> When you manually use the build it takes 30+ seconds for the search engines
> to be displayed. When this is done using automation for some reason it is
> even slower. I'll also have to look at the implementation of the new
> about:home since it may react differently to typing text in the url bar and
> sending them via code. I saw a similar issue with the editable textfield of
> Find in Page were it would not search until I pressed enter.
Today with some guidance from Mark Capella I investigated this. The blockForEventData timed out because the event was never triggered. 

With the new implementation of about:home the SearchEngine:Get event is broadcasted by BrowserSearch.java in the onCreate method called from BrowserApp.java in it's own onCreate method. Now there are a few conditions for the event to be triggered: The BrowserSearch instance to not exist when the BrowserApp instance is created and for the search engines list (mSearchEngines) in the BrowserSearch class to be null. This new implementation of about:home destroys the BrowserApp instance on exiting about:home but does not destroy the BrowserSearch fragment which remains in use so the SearchEngines:Get event is not sent again unless a new page is loaded in the tab or a new tab is loaded. In the old version of the awesomescreen every time the AllTabsPage was created the event would be broadcasted. So this why it's different on FIG then on m-c and it seems I got it wrong in Comment 2.

Going further with the investigation into why the search engines are not displayed it seems that BrowserSearch.setSearchEngines or updateFromSearchEngine are not called to update and display the search engines, mSearchEngines is an empty list although it should have search engine entries(views). This leads to SearchEngineRow instances not being created from the getView method defined in the SearchAdapter from BrowserSearch.java. Fist I was thinking that filterEditingMode from BrowserApp.java hides the BrowserSearch because it does not recognize the text entered via sendKeys but this is not the case and the BrowserSearch should be displayed (mBrowserSearch.setUserVisibleHint is set to TRUE)).

This is as far as I could go with my investigation with my limited understanding of how the new about:home works and since this is the first time I am seeing this code.

Comment 7

[Adrian Tamas (:AdrianT)]

Comment on attachment 780395 [details] [diff] [review]
testAddSearchEngine fix

Canceling review since this need a bit of rework to incorporate Geoff's feedback and my investigation in the SearchEngine:Data event triggering. I'm trying with opening new tabs for each search so the SearchEngines:Get event is triggered by default. Try run: https://tbpl.mozilla.org/?tree=Try&rev=1ec1b5c18bb0

Comment 8

[Adrian Tamas (:AdrianT)]

Attachment: testAddSearchEngine fix try 2

Fix for the testAddSearchEngine test. This works locally for me but it does not work on gingerbread and froyo devices because of Bug 899550.

Locally I had a lot of problems with the focusUrl method changed the check to be urlEditText.isInputMethodTarget which would return true if the url bar is the focus for the input method and if it is it means it is ready to take text input. I also added a click on the EditText if it is not focused to ensure that edit mode was entered. Other then that I updated the changes I made in Bug 869277 to the testAddSearchEngine test to be as close as possible to the test that works reliably now on m-c

Comment 9

[Adrian Tamas (:AdrianT)]

Attachment: testAddSearchEngine fix try 3

made the changes and added the dependency on the class created in bug 901432

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 785754 [details] [diff] [review]
testAddSearchEngine fix try 3

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

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +116,5 @@
> +                        try {
> +                            ClassLoader classLoader = getActivity().getClassLoader();
> +                            Class searchEngineRow = classLoader.loadClass("org.mozilla.gecko.home.SearchEngineRow");
> +                            for (int i = 0; i < adapter.getCount(); i++ ) {
> +                            View item = view.getChildAt(i);

Will this work reliably? Since i=0..adapter.getCount(), I would prefer to see View item = adapter.getView(i, ...).

Comment 11

[Adrian Tamas (:AdrianT)]

(In reply to Geoff Brown [:gbrown] from comment #10)
> Comment on attachment 785754 [details] [diff] [review]
> testAddSearchEngine fix try 3
> 
> Review of attachment 785754 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/tests/testAddSearchEngine.java.in
> @@ +116,5 @@
> > +                        try {
> > +                            ClassLoader classLoader = getActivity().getClassLoader();
> > +                            Class searchEngineRow = classLoader.loadClass("org.mozilla.gecko.home.SearchEngineRow");
> > +                            for (int i = 0; i < adapter.getCount(); i++ ) {
> > +                            View item = view.getChildAt(i);
> 
> Will this work reliably? Since i=0..adapter.getCount(), I would prefer to
> see View item = adapter.getView(i, ...).

This should work reliably. We did not have any issues with this and it was used previously in all the get awesomescreen section methods used in m-c and in all the methods that get views for pop-up menus. I'll look into changing it to use getView.

Comment 12

[:Margaret Leibovic]

Comment on attachment 785754 [details] [diff] [review]
testAddSearchEngine fix try 3

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

I'd like lucasr to look at this patch as well, since he worked on implementing the new BrowserSearch UI.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 785754 [details] [diff] [review]
testAddSearchEngine fix try 3

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

Needs some more work on the code to check the number of search engine rows in the list. The number of children in a ListView doesn't necessarily match the number of items in the adapter because of view recycling. Looks good otherwise.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +27,5 @@
>      public void testAddSearchEngine() {
>          int height,width;
>          final int initialNumSearchEngines;
>          String blank = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> +        String blank_page_title = "Browser Blank Page 01";

Turn this into a BLANK_PAGE_TITLE constant. Move it together with the other constants.

@@ +32,2 @@
>          String url = getAbsoluteUrl("/robocop/robocop_search.html");
> +        String url_page_title = "Robocop Search Engine";

Same here: turn this into a URL_PAGE_TITLE constant.

@@ +38,4 @@
>  
>          // Get the searchengine data
>          Actions.EventExpecter searchEngineDataEventExpector = mActions.expectGeckoEvent("SearchEngines:Data");
> +        mSolo.clickOnText(blank_page_title);

This can be replaced with focusUrlBar()

@@ +116,5 @@
> +                        try {
> +                            ClassLoader classLoader = getActivity().getClassLoader();
> +                            Class searchEngineRow = classLoader.loadClass("org.mozilla.gecko.home.SearchEngineRow");
> +                            for (int i = 0; i < adapter.getCount(); i++ ) {
> +                            View item = view.getChildAt(i);

This will cause crashes if the adapter has more items than visible listview items. You should probably just check view type here (i.e. the Adapter's getViewType) and only rely on data coming from the adapter.

Comment 14

[:Margaret Leibovic]

Comment on attachment 785754 [details] [diff] [review]
testAddSearchEngine fix try 3

Clearing review until there's a new version.

Comment 15

[Adrian Tamas (:AdrianT)]

Attachment: Add Seardh Engine test v4

I addressed all the changes requested

Comment 16

[:Margaret Leibovic]

Comment on attachment 789521 [details] [diff] [review]
Add Seardh Engine test v4

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

Looking good. I mostly have some feedback related to things that you didn't actually change in this patch :)

I'll let lucasr do the final review, since he was the last one to look at this more closely.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +129,4 @@
>       * Uses a BooleanTest which counts how many SearchEngineRow instances are being displayed
>       * in the Awesomescreen.
>       * @param waitText Text from the loaded page to expect. Used to detect when the Awesomescreen
>       *                 close animation has completed.

It's confusing that this is a parameter. Do you ever expect to pass in something other than BLANK_PAGE_TITLE? If not, I think this method's behavior would be more intuitive if you stopped passing this in as a parameter.

@@ +132,5 @@
>       *                 close animation has completed.
>       * @param expectedCountParam The expected number of search engines.
>       */
>      public void verifyDisplayedSearchEnginesCount(String waitText, int expectedCountParam) {
>          final int expectedCount = expectedCountParam;

Can you just make the parameter itself final instead of declaring this new variable here? If so, you could just rename it expectedCount and use it the same way down below.

@@ +133,5 @@
>       * @param expectedCountParam The expected number of search engines.
>       */
>      public void verifyDisplayedSearchEnginesCount(String waitText, int expectedCountParam) {
>          final int expectedCount = expectedCountParam;
> +        focusUrlBar();

Why do you need to focus the urlbar here? Shouldn't it already be focused?

@@ +156,5 @@
>                              }
>                          } catch (Exception e) {
>                               mAsserter.dumpLog("Exception in verifyDisplayedSearchEnginesCount", e);
>                          }
>                      } 

There's a comment down below here about closing the awesomescreen, you should update that as well.

Comment 17

[Adrian Tamas (:AdrianT)]

Attachment: testAddSearchEngine fix v5

(In reply to :Margaret Leibovic from comment #16)
> Comment on attachment 789521 [details] [diff] [review]
> Add Seardh Engine test v4
> 
> Review of attachment 789521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good. I mostly have some feedback related to things that you didn't
> actually change in this patch :)
> 
> I'll let lucasr do the final review, since he was the last one to look at
> this more closely.
> 
> @@ +132,5 @@
> >       *                 close animation has completed.
> >       * @param expectedCountParam The expected number of search engines.
> >       */
> >      public void verifyDisplayedSearchEnginesCount(String waitText, int expectedCountParam) {
> >          final int expectedCount = expectedCountParam;
> 
> Can you just make the parameter itself final instead of declaring this new
> variable here? If so, you could just rename it expectedCount and use it the
> same way down below.
I can't make the parameter a global final variable because it needs to change. We are expecting first 4 search engines and then 5. It's only final so it can be used in the BooleanTest declared in the waitForTest.

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 790110 [details] [diff] [review]
testAddSearchEngine fix v5

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

Looks good overall but the view type check should be done with getViewType() instead of getView()+instanceOf.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +147,2 @@
>                              for (int i = 0; i < adapter.getCount(); i++ ) {
> +                                View item = adapter.getView(i, null, view);

Using getView is not necessary here if you just want to check the type of view the list has at a certain position. You can simply call the adapter's getViewType() and check the returned value.

Comment 19

[Adrian Tamas (:AdrianT)]

Attachment: testAddSearchEngine fix v6

First I made the search box a lot bigger because sometimes the clickLongOnScreen seemed to fail to open the context menu. Second getViewType did not work since there were multiple view types from different list views that returned the same values (0 -searchRow and 2-searchRow with suggestions). To avoid all the view counting and checking I moved the check to the ListView level, checking that it is a BrowserSearch$HomeSearchListView and returning the adapter child count.

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=ec403ba1c7ef

Comment 20

[Lucas Rocha (:lucasr)]

Comment on attachment 794667 [details] [diff] [review]
testAddSearchEngine fix v6

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

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +141,5 @@
>                  views = mSolo.getCurrentListViews();
>                  for (ListView view : views) {
> +                    try {
> +                        ClassLoader classLoader = getActivity().getClassLoader();
> +                        Class searchEngineView = classLoader.loadClass("org.mozilla.gecko.home.BrowserSearch$HomeSearchListView");

This should probably be done by fetching the listview by tag instead. We don't currently tag the browser search listview but that should be easy to add.

Comment 21

[Adrian Tamas (:AdrianT)]

Attachment: addSearchEngine.patch v7

Changed the test to use the ListView tag added by bug 916107

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=e192a91a284f

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 805319 [details] [diff] [review]
addSearchEngine.patch v7

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

Looks good.

Comment 23

[Adrian Tamas (:AdrianT)]

Attachment: addSearchEngine.patch

Changed the test to use the updated tag from bug 916107

Comment 24

[Lucas Rocha (:lucasr)]

Comment on attachment 808527 [details] [diff] [review]
addSearchEngine.patch

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

Looks nice.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +20,3 @@
>      private final int MAX_WAIT_TEST_MS = 5000;
> +    private final String SEARCH_TEXT = "Firefox for Android";
> +    private final String ADD_SEARCHENGINE_OPTION_TEXT = "Add Search Engine";

Isn't this kind of stuff supposed to be in StringHelper now?

Comment 25

[Adrian Tamas (:AdrianT)]

(In reply to Lucas Rocha (:lucasr) from comment #24)
> Comment on attachment 808527 [details] [diff] [review]
> addSearchEngine.patch
> 
> Review of attachment 808527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks nice.
> 
> ::: mobile/android/base/tests/testAddSearchEngine.java.in
> @@ +20,3 @@
> >      private final int MAX_WAIT_TEST_MS = 5000;
> > +    private final String SEARCH_TEXT = "Firefox for Android";
> > +    private final String ADD_SEARCHENGINE_OPTION_TEXT = "Add Search Engine";
> 
> Isn't this kind of stuff supposed to be in StringHelper now?

As we discussed on irc we should add UI specific strings that would be used in other tests also. It's highly unlikely to have a second test on adding a search engine

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a00aba84b699

Comment 28

[Lucas Rocha (:lucasr)]

No need to hurry to uplift this.