Open Bug 830834 Opened 7 years ago Updated Last year

Robocop: Add test for 'Show Search Suggestions' feature

Categories

(Firefox for Android :: General, defect)

21 Branch
ARM
Android
defect
Not set

Tracking

()

People

(Reporter: xti, Unassigned)

References

Details

Attachments

(1 file, 25 obsolete files)

20.70 KB, patch
jmaher
: review-
Details | Diff | Splinter Review
This bug tracks the Robocop tests for the Show Search Suggestions feature on Firefox for Android.

The first one will test if the Show Search Suggestions preference is unchecked at start up in Settings Menu and then if the Show Suggestions notification is triggered in Awesomescreen. The notification is dismissed by tapping on the "Yes" button and the test will go back in Settings to check if the Show Search Suggestions preference is now checked. In the end, the test will verify if the Show Suggestions notification is not triggered again in Awesomescreen.

For the second test, there will be the same steps except the part where the notification is dismissed by tapping on the "No" button and the checkbox status according to this option.

These patches integrates the following testcases:
https://moztrap.mozilla.org/manage/case/2739/
https://moztrap.mozilla.org/manage/case/2740/
Note: The following lines were changed since the test profile had the "Show Search Suggestions" pref enabled by default. A regular profile has it disabled at start-up.

524 options.extraPrefs.append('browser.search.suggest.enabled=false')
525 options.extraPrefs.append('browser.search.suggest.prompted=false')
Attachment #702373 - Flags: review?(jmaher)
Comment on attachment 702373 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V1.0

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

::: mobile/android/base/tests/testBrowserSearchSuggestionsYes.java.in
@@ +32,5 @@
> +        // Verify how many checkboxes are visible on the screen
> +        mAsserter.ok(checkBoxes.size() > 0, "There are checkboxes displayed in the view", String.valueOf(checkBoxes.size()) + " are in view");
> +
> +        // Verify if "Show search suggestions" pref is unchecked       
> +        mAsserter.ok(!mSolo.isCheckBoxChecked(checkBoxes.size()-2), "Checkbox not checked", "Show search suggestions is not checked");

This seems like it might break easily: If a checkbox setting is removed, added, or re-arranged. Worse, if a setting is changed, there is a good chance that the test will continue to pass, even though it is examining the wrong checkbox!

@@ +44,5 @@
> +        mAsserter.ok(mSolo.searchText("Would you like to turn on Google search suggestions?"), "Notification is triggered", "Google search engine notification is triggered");
> +
> +        mSolo.clickOnText("Yes");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mSolo.sleep(1000);

I would like to see a comment explaining why the pause is necessary.

::: testing/mochitest/runtestsremote.py
@@ +521,5 @@
>              fennec_ids = options.robocopIds
>          dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt"))
>          options.extraPrefs.append('robocop.logfile="%s/robocop.log"' % deviceRoot)
> +        options.extraPrefs.append('browser.search.suggest.enabled=false')
> +        options.extraPrefs.append('browser.search.suggest.prompted=false')

Will this adversely affect testSearchSuggestions?
Attachment #702386 - Flags: review?(jmaher)
(In reply to Geoff Brown [:gbrown] from comment #2)
> Comment on attachment 702373 [details] [diff] [review]
> Search Suggestion opt-in prompt: Yes V1.0
> 
> Review of attachment 702373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like it might break easily: If a checkbox setting is removed,
> added, or re-arranged. Worse, if a setting is changed, there is a good
> chance that the test will continue to pass, even though it is examining the
> wrong checkbox!

Indeed, if a checkbox is added/removed from Settings, then this line will be changed:
mAsserter.ok(!mSolo.isCheckBoxChecked(checkBoxes.size()-2), and there will be a checkBoxes.size()-x, according to the new changes.

Another way to do this would be to scroll down to the "Show search suggestions" pref and the code will be always: checkBoxes.size()-1, except for the 10" tablets (in portrait) where it will be always checkBoxes.size()-2 (when Settings menu is opened, this pref is already in view and also the next checkbox too, by default).

> > +        mSolo.sleep(1000);
> 
> I would like to see a comment explaining why the pause is necessary.

I noticed that sometimes mActions.sendSpecialKey(Actions.SpecialKey.MENU) (which follows after this pause) doesn't work immediately after the Awesomescreen is dismissed and it seems to work always with this pause here.

> ::: testing/mochitest/runtestsremote.py
> @@ +521,5 @@
> >              fennec_ids = options.robocopIds
> >          dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt"))
> >          options.extraPrefs.append('robocop.logfile="%s/robocop.log"' % deviceRoot)
> > +        options.extraPrefs.append('browser.search.suggest.enabled=false')
> > +        options.extraPrefs.append('browser.search.suggest.prompted=false')
> 
> Will this adversely affect testSearchSuggestions?

The Search Suggestions notification is triggered only if this pref is disabled at start-up. Any other changes to this pref through the Settings Menu will result in a permanent disable of the Search Suggestions notification, so both tests will be invalid in this case.
FIRE PROC: '"MOZ_CRASHREPORTER=1,XPCOM_DEBUG_BREAK=stack,MOZ_HIDE_RESULTS_TABLE=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1,MOZ_PROCESS_LOG=/tmp/tmpNHk1QSpidlog,XPCOM_MEM_BLOAT_LOG=/tmp/tmptgq4X7/runtests_leaks.log" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsNo org.mozilla.roboexample.test/org.mozilla.fennec_jmaher.FennecInstrumentationTestRunner'
Robocop derived process name: org.mozilla.fennec_jmaher
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testBrowserSearchSuggestionsNo
2 INFO TEST-PASS | testBrowserSearchSuggestionsNo | Check for Show Search  Suggestions pref - Show search suggestions exists
3 INFO TEST-PASS | testBrowserSearchSuggestionsNo | There are checkboxes displayed in the view - 6 are in view
4 INFO TEST-PASS | testBrowserSearchSuggestionsNo | Checkbox not checked - Show search suggestions is not checked
5 INFO TEST-PASS | testBrowserSearchSuggestionsNo | Notification is triggered - Google search engine notification is triggered
Exception caught during test!
junit.framework.AssertionFailedError: The text: Settings is not found!
	at junit.framework.Assert.fail(Assert.java:47)
	at junit.framework.Assert.assertTrue(Assert.java:20)
	at com.jayway.android.robotium.solo.Clicker.clickOnText(Clicker.java:291)
	at com.jayway.android.robotium.solo.Solo.clickOnText(Solo.java:823)
	at org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsNo.testBrowserSearchSuggestionsNo(testBrowserSearchSuggestionsNo.java:53)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:129)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
6 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsNo | Exception caught - junit.framework.AssertionFailedError: The text: Settings is not found!
7 INFO TEST-END | testBrowserSearchSuggestionsNo | finished in 57172ms
8 INFO TEST-START | Shutdown
9 INFO Passed: 4
10 INFO Failed: 1
11 INFO Todo: 0
12 INFO SimpleTest FINISHED
FIRE PROC: '"MOZ_CRASHREPORTER=1,XPCOM_DEBUG_BREAK=stack,MOZ_HIDE_RESULTS_TABLE=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1,MOZ_PROCESS_LOG=/tmp/tmpUCGLAtpidlog,XPCOM_MEM_BLOAT_LOG=/tmp/tmpMvzmkp/runtests_leaks.log" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsYes org.mozilla.roboexample.test/org.mozilla.fennec_jmaher.FennecInstrumentationTestRunner'
Robocop derived process name: org.mozilla.fennec_jmaher
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testBrowserSearchSuggestionsYes
2 INFO TEST-PASS | testBrowserSearchSuggestionsYes | Check for Show Search  Suggestions pref - Show search suggestions exists
3 INFO TEST-PASS | testBrowserSearchSuggestionsYes | There are checkboxes displayed in the view - 6 are in view
4 INFO TEST-PASS | testBrowserSearchSuggestionsYes | Checkbox not checked - Show search suggestions is not checked
5 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Notification is triggered - Google search engine notification is triggered
Exception caught during test!
junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Notification is triggered - Google search engine notification is triggered
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsYes.testBrowserSearchSuggestionsYes(testBrowserSearchSuggestionsYes.java:44)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:129)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
6 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Exception caught - junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Notification is triggered - Google search engine notification is triggered
7 INFO TEST-END | testBrowserSearchSuggestionsYes | finished in 48559ms
8 INFO TEST-START | Shutdown
9 INFO Passed: 3
10 INFO Failed: 2
11 INFO Todo: 0
12 INFO SimpleTest FINISHED
Comment on attachment 702373 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V1.0

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

::: mobile/android/base/tests/testBrowserSearchSuggestionsYes.java.in
@@ +31,5 @@
> +
> +        // Verify how many checkboxes are visible on the screen
> +        mAsserter.ok(checkBoxes.size() > 0, "There are checkboxes displayed in the view", String.valueOf(checkBoxes.size()) + " are in view");
> +
> +        // Verify if "Show search suggestions" pref is unchecked       

nit: whitespace after the above line.

@@ +44,5 @@
> +        mAsserter.ok(mSolo.searchText("Would you like to turn on Google search suggestions?"), "Notification is triggered", "Google search engine notification is triggered");
> +
> +        mSolo.clickOnText("Yes");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mSolo.sleep(1000);

maybe a waitfortext instead of a sleep?

::: testing/mochitest/runtestsremote.py
@@ +521,5 @@
>              fennec_ids = options.robocopIds
>          dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt"))
>          options.extraPrefs.append('robocop.logfile="%s/robocop.log"' % deviceRoot)
> +        options.extraPrefs.append('browser.search.suggest.enabled=false')
> +        options.extraPrefs.append('browser.search.suggest.prompted=false')

these should be turned off for good as we should not have any outside network access required.  This does appear to cause testSearchSuggestions to fail.  We should revisit testSearchSuggestions to see how valid it is.
Attachment #702373 - Flags: review?(jmaher) → review-
Comment on attachment 702386 [details] [diff] [review]
Search Suggestion opt-in prompt: No V1.0

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

marking as r- since it fails and I would prefer to see this combined into one test case.
Attachment #702386 - Flags: review?(jmaher) → review-
Attached patch Search Suggestion opt-in prompt (obsolete) — Splinter Review
Attachment #702373 - Attachment is obsolete: true
Attachment #702386 - Attachment is obsolete: true
testSearchSuggestions was modified due to the changes performed in runtestsremote.py and it's not failing anymore.
Attachment #703921 - Attachment is obsolete: true
Attachment #704475 - Flags: review?(jmaher)
Attachment #704477 - Flags: review?(jmaher)
Comment on attachment 704475 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V2.0

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

this fails on android 2.3/tablet mode:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19004095&tree=Try&full=1
Attachment #704475 - Flags: review?(jmaher) → review-
Attachment #704475 - Attachment is obsolete: true
Attachment #704477 - Attachment is obsolete: true
Attachment #704477 - Flags: review?(jmaher)
Attachment #706356 - Flags: review?(jmaher)
Attachment #706357 - Flags: review?(jmaher)
Comment on attachment 706356 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V3.0

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

this passes on try server and locally!  congrats.  I am marking this a r- because of the long sleeps.  We need comments to justify why we have them, or find other solutions.

::: mobile/android/base/tests/testBrowserSearchSuggestionsYes.java.in
@@ +76,5 @@
> +            }
> +        }
> +
> +        // Verify if all Settings options are listed
> +        mAsserter.ok(true, "Settings List = ", options.toString());

this will never fail

@@ +79,5 @@
> +        // Verify if all Settings options are listed
> +        mAsserter.ok(true, "Settings List = ", options.toString());
> +
> +        // Verify if all checkboxes are listed
> +        mAsserter.ok(true, "CheckBox List = ", checkBoxText.toString());

this will never fail

@@ +126,5 @@
> +
> +    private ListView getSettingsList() {
> +
> +    selectMenuItem("Settings");
> +    mSolo.sleep(10000);

ouch, this is a long sleep.  For other settings menu work, I don't recall needing to do this.

::: mobile/android/base/tests/testSearchSuggestions.java.in
@@ +38,5 @@
> +        // Enable the "Show Search Suggestions" pref in Settings
> +        selectMenuItem("Settings");
> +        mSolo.searchText("Show search suggestions");
> +        mSolo.clickOnText("Show search suggestions");
> +        mSolo.sleep(1000);

can we do something else besides sleep?
Attachment #706356 - Flags: review?(jmaher) → review-
Comment on attachment 706357 [details] [diff] [review]
Search Suggestion opt-in prompt: No V3.0

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

Can we share code between the Yes|No case here?  I think 99% of the code is the same.  A couple common routines would be very beneficial.  I understand why we need two test cases.
Attachment #706357 - Flags: review?(jmaher) → review-
Attachment #706356 - Attachment is obsolete: true
Attachment #706357 - Attachment is obsolete: true
Attachment #709022 - Flags: review?(jmaher)
Attachment #709023 - Flags: review?(gbrown)
Attachment #709022 - Flags: review?(jmaher) → review?(gbrown)
(In reply to Joel Maher (:jmaher) from comment #16)
> Comment on attachment 706357 [details] [diff] [review]
> Search Suggestion opt-in prompt: No V3.0
> 
> Review of attachment 706357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we share code between the Yes|No case here?  I think 99% of the code is
> the same.  A couple common routines would be very beneficial.  I understand
> why we need two test cases.

I moved the main common code to BaseTest. It should be fine now.
Comment on attachment 709022 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V4.0

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

r- for the build failure.

::: mobile/android/base/tests/BaseTest.java.in
@@ +626,5 @@
> +        selectMenuItem("Settings");
> +        mSolo.waitForText("General");
> +        final ArrayList<ListView> views = mSolo.getCurrentListViews();
> +
> +        list = null;

Where is list declared? ** This does not compile for me. **

@@ +634,5 @@
> +                  list = view;
> +                  return true;
> +               }
> +               return false;
> +            }

Why is waitForTest used here? It looks like nothing can change for the duration of waitForTest. I would either move the getCurrentListViews() call inside test(), or remove the waitForTest().

@@ +644,5 @@
> +        ArrayList<String> checkBoxText = new ArrayList();
> +        View option, Opt;
> +        final ArrayList<CheckBox> checkBoxes =  new ArrayList();
> +        final ArrayList<String> options = new ArrayList();
> +        //final ArrayList<String> checkBoxText = new ArrayList();

delete the unused (commented out) line

::: mobile/android/base/tests/testBrowserSearchSuggestionsYes.java.in
@@ +23,5 @@
> +    }
> +
> +    public void testBrowserSearchSuggestionsYes() {
> +        int index;
> +        ArrayList<String> checkBoxText = new ArrayList();

nit: I think this would be easier to read if you changed "checkBoxText" -> "checkBoxTextList"
Attachment #709022 - Flags: review?(gbrown) → review-
Attachment #709022 - Attachment is obsolete: true
Attachment #709023 - Attachment is obsolete: true
Attachment #709023 - Flags: review?(gbrown)
Attachment #710191 - Flags: review?(jmaher)
Attachment #710193 - Flags: review?(jmaher)
Comment on attachment 710191 [details] [diff] [review]
Search Suggestion opt-in prompt V4.1

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

a few small nits.  
this passes on the tegras in production, but the testSystemPages seems to fail a lot
r- for failing to pass on panda boards.
https://tbpl.mozilla.org/?tree=Try&rev=348fc13b7ba9&noignore=1


FIRE PROC: '"MOZ_CRASHREPORTER=1,XPCOM_DEBUG_BREAK=stack,MOZ_HIDE_RESULTS_TABLE=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1,MOZ_PROCESS_LOG=/tmp/tmpvtF8Vopidlog,XPCOM_MEM_BLOAT_LOG=/mnt/sdcard/tests/profile/runtests_leaks.log" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec.tests.testBrowserSearchSuggestionsYes org.mozilla.roboexample.test/org.mozilla.fennec.FennecInstrumentationTestRunner'
Robocop derived process name: org.mozilla.fennec
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testBrowserSearchSuggestionsYes
2 INFO TEST-PASS | testBrowserSearchSuggestionsYes | Check for Show Search  Suggestions pref - Show search suggestions exists
3 INFO TEST-PASS | testBrowserSearchSuggestionsYes | Checkbox not checked - Show search suggestions is not checked
4 INFO TEST-PASS | testBrowserSearchSuggestionsYes | Notification is triggered - Google search engine notification is triggered
5 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Checkbox checked - Show search suggestions is checked
Exception caught during test!
junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Checkbox checked - Show search suggestions is checked
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec.tests.testBrowserSearchSuggestionsYes.testBrowserSearchSuggestionsYes(testBrowserSearchSuggestionsYes.java:62)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:135)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
6 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Exception caught - junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Checkbox checked - Show search suggestions is checked
7 INFO TEST-END | testBrowserSearchSuggestionsYes | finished in 23251ms
8 INFO TEST-START | Shutdown
9 INFO Passed: 3
10 INFO Failed: 2
11 INFO Todo: 0
12 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:00:28.141330
INFO | automation.py | Reading PID log: /tmp/tmpvtF8Vopidlog
getting files in '/mnt/sdcard/tests/profile/minidumps/'
WARNING | automationutils.processLeakLog() | refcount logging is off, so leaks can't be detected!

::: mobile/android/base/tests/BaseTest.java.in
@@ +645,5 @@
>              }
>          }
>      }
> +
> +    public ListView getSettingsList() {      

nit: whitespace at the end here.

@@ +656,5 @@
> +        }
> +        return list;
> +    }
> +
> +    public ArrayList<String> getSettingsCheckBoxes() {

Can you add a comment above this as to why we would use this method or how it is used?

@@ +662,5 @@
> +        View option, Opt;
> +        final ArrayList<CheckBox> checkBoxes =  new ArrayList();
> +        final ArrayList<String> options = new ArrayList();
> +        ListView settingsList = getSettingsList();
> +        for (int i = 0; i < settingsList.getAdapter().getCount();i++) {

nit: space between ; and i++

@@ +668,5 @@
> +            if (settingsItem instanceof android.widget.LinearLayout) {
> +                ViewGroup settingsEntry = (ViewGroup)settingsItem;
> +                for (int j = 0; j < settingsEntry.getChildCount(); j++) {
> +                    View setting = settingsEntry.getChildAt(j);
> +                    if (setting instanceof android.widget.RelativeLayout){

nit: space between ){

::: mobile/android/base/tests/testSearchSuggestions.java.in
@@ +40,5 @@
> +        mSolo.searchText("Show search suggestions");
> +        mSolo.clickOnText("Show search suggestions");
> +
> +        /* Adding the following line as a delay to be sure that "Show search suggestion"
> +        checkbox is cheked */

nit: /cheked/checked/, also remove the extra newline in the comment

::: testing/mochitest/runtestsremote.py
@@ +527,5 @@
>              fennec_ids = options.robocopIds
>          dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt"))
>          options.extraPrefs.append('robocop.logfile="%s/robocop.log"' % deviceRoot)
> +        options.extraPrefs.append('browser.search.suggest.enabled=false')
> +        options.extraPrefs.append('browser.search.suggest.prompted=false')

nit: fix the indentation here.
Attachment #710191 - Flags: review?(jmaher) → review-
Comment on attachment 710193 [details] [diff] [review]
Search Suggestion opt-in prompt Yes & No V4.1

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

just a couple nits.

::: mobile/android/base/tests/testBrowserSearchSuggestionsNo.java.in
@@ +30,5 @@
> +
> +        checkBoxTextList = getSettingsCheckBoxes();
> +
> +        // Verify if the "Show Search Suggestion" pref exists
> +        mAsserter.ok(mSolo.searchText("Show search suggestions"), "Check for Show Search  Suggestions pref", "Show search suggestions exists");

nit: extra white space between 'Search  Suggestions'.  Also please keep it lowercase except for the first word.

@@ +54,5 @@
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mSolo.waitForText("Enter Search or Address");
> +
> +        /* Go back to Settings and verify if "Show search suggestions" pref is still
> +        unchecked */

nit: make the comment fit on one line.

::: mobile/android/base/tests/testBrowserSearchSuggestionsYes.java.in
@@ +30,5 @@
> +
> +        checkBoxTextList = getSettingsCheckBoxes();
> +
> +        // Verify if the "Show Search Suggestion" pref exists
> +        mAsserter.ok(mSolo.searchText("Show search suggestions"), "Check for Show Search  Suggestions pref", "Show search suggestions exists");

nit: extra white space between 'Search  Suggestions'.  Also please keep it lowercase except for the first word.
Attachment #710193 - Flags: review?(jmaher) → review-
Attachment #710191 - Attachment is obsolete: true
Attachment #710193 - Attachment is obsolete: true
Attachment #710683 - Flags: review?(jmaher)
Attachment #710684 - Flags: review?(jmaher)
(In reply to Joel Maher (:jmaher) from comment #23)
> Comment on attachment 710191 [details] [diff] [review]
> Search Suggestion opt-in prompt V4.1
> 
> Review of attachment 710191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a few small nits.  
> this passes on the tegras in production, but the testSystemPages seems to
> fail a lot
> r- for failing to pass on panda boards.
> https://tbpl.mozilla.org/?tree=Try&rev=348fc13b7ba9&noignore=1
> 
These patches shouldn't affect the System Pages test.

> ::: testing/mochitest/runtestsremote.py
> @@ +527,5 @@
> >              fennec_ids = options.robocopIds
> >          dm.pushFile(fennec_ids, os.path.join(deviceRoot, "fennec_ids.txt"))
> >          options.extraPrefs.append('robocop.logfile="%s/robocop.log"' % deviceRoot)
> > +        options.extraPrefs.append('browser.search.suggest.enabled=false')
> > +        options.extraPrefs.append('browser.search.suggest.prompted=false')
> 
> nit: fix the indentation here.

I tried to fix it and I've got this error:

  File "_tests/testing/mochitest/runtestsremote.py", line 527
    fennec_ids = options.robocopIds
             ^
IndentationError: expected an indented block
Comment on attachment 710684 [details] [diff] [review]
Search Suggestion opt-in prompt: No V4.2

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

this works great on the tegras, but is failing on the panda boards:
FIRE PROC: '"MOZ_CRASHREPORTER=1,XPCOM_DEBUG_BREAK=stack,MOZ_HIDE_RESULTS_TABLE=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1,MOZ_PROCESS_LOG=/tmp/tmpO8ZHKGpidlog,XPCOM_MEM_BLOAT_LOG=/tmp/tmpDRIE51/runtests_leaks.log" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsNo org.mozilla.roboexample.test/org.mozilla.fennec_jmaher.FennecInstrumentationTestRunner'
Robocop derived process name: org.mozilla.fennec_jmaher
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testBrowserSearchSuggestionsNo
2 INFO TEST-PASS | testBrowserSearchSuggestionsNo | Check for Show search suggestions pref - Show search suggestions exists
3 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsNo | Checkbox not checked - Show search suggestions is not checked
Exception caught during test!
junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsNo | Checkbox not checked - Show search suggestions is not checked
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsNo.testBrowserSearchSuggestionsNo(testBrowserSearchSuggestionsNo.java:41)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:135)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
4 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsNo | Exception caught - junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsNo | Checkbox not checked - Show search suggestions is not checked
5 INFO TEST-END | testBrowserSearchSuggestionsNo | finished in 9367ms
6 INFO TEST-START | Shutdown
7 INFO Passed: 1
8 INFO Failed: 2
9 INFO Todo: 0
10 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:00:16.312130


the panda boards run ICS 4.0.4 in landscape mode.  I will have to do some screenshots to look at what is happening.
Attachment #710684 - Flags: review?(jmaher) → review-
Comment on attachment 710683 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V4.2

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

this passes great on the tegras, but fails on the panda (ICS 4.0.4):
FIRE PROC: '"MOZ_CRASHREPORTER=1,XPCOM_DEBUG_BREAK=stack,MOZ_HIDE_RESULTS_TABLE=1,MOZ_CRASHREPORTER_NO_REPORT=1,NO_EM_RESTART=1,MOZ_PROCESS_LOG=/tmp/tmp_rGX7Jpidlog,XPCOM_MEM_BLOAT_LOG=/tmp/tmpQmx5op/runtests_leaks.log" am instrument -w -e deviceroot /mnt/sdcard/tests -e class org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsYes org.mozilla.roboexample.test/org.mozilla.fennec_jmaher.FennecInstrumentationTestRunner'
Robocop derived process name: org.mozilla.fennec_jmaher
INFO | automation.py | Application pid: 0
0 INFO SimpleTest START
1 INFO TEST-START | testBrowserSearchSuggestionsYes
2 INFO TEST-PASS | testBrowserSearchSuggestionsYes | Check for Show search suggestions pref - Show search suggestions exists
3 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Checkbox not checked - Show search suggestions is not checked
Exception caught during test!
junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Checkbox not checked - Show search suggestions is not checked
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec_jmaher.tests.testBrowserSearchSuggestionsYes.testBrowserSearchSuggestionsYes(testBrowserSearchSuggestionsYes.java:41)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:135)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551)
4 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Exception caught - junit.framework.AssertionFailedError: 3 INFO TEST-UNEXPECTED-FAIL | testBrowserSearchSuggestionsYes | Checkbox not checked - Show search suggestions is not checked
5 INFO TEST-END | testBrowserSearchSuggestionsYes | finished in 10043ms
6 INFO TEST-START | Shutdown
7 INFO Passed: 1
8 INFO Failed: 2
9 INFO Todo: 0
10 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:00:17.657962
Attachment #710683 - Flags: review?(jmaher) → review-
I believe the failure is that the "Show Search Suggestions" needs to be panned into view?  I don't see it on a screenshot.
Attachment #710683 - Attachment is obsolete: true
Attachment #710684 - Attachment is obsolete: true
Attachment #712380 - Flags: review?(jmaher)
Attachment #712381 - Flags: review?(jmaher)
(In reply to Joel Maher (:jmaher) from comment #30)
> I believe the failure is that the "Show Search Suggestions" needs to be
> panned into view?  I don't see it on a screenshot.

I hope that this time will work fine. I added mSolo.searchForText when I try to verify the checkbox status for the 2nd time.
Attachment #712381 - Attachment is obsolete: true
Attachment #712381 - Flags: review?(jmaher)
Attachment #712390 - Flags: review?(jmaher)
Comment on attachment 712380 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V4.3

this fails in the same way it did last time.  I would recommend getting try server access and going that route.
Attachment #712380 - Flags: review?(jmaher) → review-
Attachment #712390 - Flags: review?(jmaher) → review-
Status: NEW → ASSIGNED
These tests seems to fail on the Panda board and Samsung Galaxy Tab2 7" - both on ICS. However, the tests never fails on the tablet if I place it in portrait.

I've checked if all existing checkboxes are indexed in landscape mode and I got the same results as for portrait. Also on the Panda board I noticed in the screenshots that the Show search suggestions pref is uncheck, but for some reasons the results point out that the pref is checked.
Attachment #712380 - Attachment is obsolete: true
Attachment #712390 - Attachment is obsolete: true
Attachment #719962 - Flags: review?(jmaher)
Attachment #719963 - Flags: review?(jmaher)
Comment on attachment 719962 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V4.4

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

this fails on the tegra platform:
https://tbpl.mozilla.org/?tree=Try&rev=d7e4964d6a3d&noignore=1
also the testSearchSuggestionsNo patch is missing a } at the end of the file.
Attachment #719962 - Flags: review?(jmaher) → review-
Attachment #719963 - Flags: review?(jmaher) → review-
I hope that this time will work fine both on tegra and panda boards.
Attachment #719962 - Attachment is obsolete: true
Attachment #719963 - Attachment is obsolete: true
Attachment #730612 - Flags: review?(jmaher)
Attachment #730613 - Flags: review?(jmaher)
Comment on attachment 730612 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V4.5

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

in general the code is good, the tests are failing both on tegra (unexpected-pass) and on the panda board (unexpected-fail)
Attachment #730612 - Flags: review?(jmaher) → review-
Comment on attachment 730613 [details] [diff] [review]
Search Suggestion opt-in prompt: No V4.5

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

in general the code is good, the tests are failing both on tegra (unexpected-pass) and on the panda board (unexpected-fail)
Attachment #730613 - Flags: review?(jmaher) → review-
Comment on attachment 730612 [details] [diff] [review]
Search Suggestion opt-in prompt: Yes V4.5

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +51,5 @@
>      protected String mBaseUrl;
>      protected String mRawBaseUrl;
>      private String mLogFile;
>      protected String mProfile;
> +    private ListView list;

I think this need not be a member variable.

Use mList (mSettingsList?) if a member variable is needed.

@@ +762,5 @@
>      }
> +
> +    public ListView getSettingsList() {
> +        selectMenuItem("Settings");
> +        mSolo.waitForText("General");

Use waitForText (from BaseTest) or check the return value from mSolo.waitForText.

::: mobile/android/base/tests/testBrowserSearchSuggestionsYes.java.in
@@ +52,5 @@
> +        mAsserter.ok(mSolo.searchText("Would you like to turn on Google search suggestions?"), "Notification is triggered", "Google search engine notification is triggered");
> +
> +        mSolo.clickOnText("Yes");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mSolo.waitForText("Enter Search or Address");

waitForText or check return from mSolo.waitForText

@@ +67,5 @@
> +            }
> +        }
> +
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mSolo.waitForText("Enter Search or Address");

waitForText or check return from mSolo.waitForText

::: mobile/android/base/tests/testSearchSuggestions.java.in
@@ +53,5 @@
> +            }
> +        }
> +
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mSolo.waitForText("Enter Search or Address");

waitForText or check return from mSolo.waitForText
Attached patch Show Search Suggestions (obsolete) — Splinter Review
Due to the issue with verifying if the checkbox is checked (bug 854872) and the instability of the isCheckBoxChecked method (sometimes works and sometimes not on ICS) I added an if to verify if the option was checked. It it is ok then it will be a PASS else it would be a KNOWN-FAIL. This also solves the issue without having to run checks for the device OS since on Tegra isCheckBoxChecked works as expected

Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=0c7ee4a48bc6
Attachment #730612 - Attachment is obsolete: true
Attachment #730613 - Attachment is obsolete: true
Attachment #741223 - Flags: review?(jmaher)
Assignee: nobody → adrian.tamas
Comment on attachment 741223 [details] [diff] [review]
Show Search Suggestions

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

please get this up on try and retrigger the job that runs this test case (I assume rc2) about 10 times to look for consistency.

::: mobile/android/base/tests/testSearchSuggestions.java.in
@@ +42,5 @@
> +        mSolo.searchText("Show search suggestions");
> +        mSolo.clickOnText("Show search suggestions");
> +
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mSolo.waitForText("Enter Search or Address");

can we use waitForText from basetest here?
Attachment #741223 - Flags: review?(jmaher) → review+
Attached patch Show Search Suggestions v1 (obsolete) — Splinter Review
Sorry I missed the waitForText there...

New try run: https://tbpl.mozilla.org/?tree=Try&rev=13cacf1b5704
Attachment #741223 - Attachment is obsolete: true
Attached patch Show Search Suggestions v2 (obsolete) — Splinter Review
Changed the code in testSearchSuggestion since in some rare situations the search suggestions options was not checked when clicking on the option text. Now I enabled search suggestions at the beginning of the test by tapping on the "yes" button for the search suggestion dialog.

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=35817dbcbf7c. It looks good now on tegra but unfortunately on pandas there is still the "2400 seconds without output" issue. We should wait for the issue to be fixed gbrown has identified the cause and do a retest then.
Attachment #741252 - Attachment is obsolete: true
Attached patch Show Search Suggestions v3 (obsolete) — Splinter Review
Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=74a3a5322550

Made the necessary changes to the test for pandas since we have the new settings menu UI on tablets.
Attachment #741719 - Attachment is obsolete: true
Attachment #749145 - Flags: review?(jmaher)
Comment on attachment 749145 [details] [diff] [review]
Show Search Suggestions v3

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

this patch looks good.  The try server run doesn't look good right now, although those failures are not from this, we need to clean up our existing failures before adding new tests.
Attachment #749145 - Flags: review?(jmaher) → review+
Retry of the tryrun looks green. Unfortunatlt I am seeing a few restarts. Should we integrate or wait for the restart bugs to be fixed?

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

Also this run was before testImportFromAndroid was integrated. Should we retest and check that the integration does not influence the number of restarts?
Flags: needinfo?(jmaher)
I would like to see a try run after the testImportFromAndroid.  Just looking at that the overall failure rate looked too high, but much better.  I would need to see if the errors were installation/setup prior to the test or not, I will do that on the next try run!
Flags: needinfo?(jmaher)
Attached patch Show Search Suggestions v3.1 (obsolete) — Splinter Review
Try run after testImportFromAndroid was integrated: https://tbpl.mozilla.org/?tree=Try&rev=4d7f882df2a6. Only seeing a few restarts, a few known fails and a fail in testImportFromAndroid which has not happened before.
Attachment #749145 - Attachment is obsolete: true
that try run looks a little too noisy for my taste.  I suspect a little bit of cleanup on other tests and maybe this will look nice and normal late next week.
Added a method to get info about the search engines from the SearchEngine:Data event data. Not critical for this test but may be used in others like for example a rewrite of the AddSearchEngine test.

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=fb92b9581dad
Attachment #753282 - Attachment is obsolete: true
Attachment #761942 - Flags: review?(jmaher)
Comment on attachment 761942 [details] [diff] [review]
Show Search Suggestions v4

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

This is still too noisy on try server.  I looked a lot of the blue runs and they happen inside of the test run itself, not in the setup/cleanup.
Attachment #761942 - Flags: review?(jmaher) → review-
Leaving this as it is until bug 879979 and bug 862793 are fixed. I don't know yet how the search suggestion will tie in with the new about:home and it would not make sense to continue work on this and then just remove the test later if it does not apply anymore
Assignee: adrian.tamas → nobody
No assignee, updating the status.
Status: ASSIGNED → NEW
No assignee, updating the status.
You need to log in before you can comment on or make changes to this bug.