Last Comment Bug 830755 - Robocop: Add test for 'Settings Menu Items' feature
: Robocop: Add test for 'Settings Menu Items' feature
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 19 Branch
: ARM Android
: -- normal (vote)
: Firefox 21
Assigned To: Paul Feher
:
:
Mentors:
Depends on: 843947
Blocks: 744859
  Show dependency treegraph
 
Reported: 2013-01-15 06:53 PST by Paul Feher
Modified: 2013-02-28 03:46 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
SettingsMenuItems test V1 (6.99 KB, patch)
2013-01-15 07:10 PST, Paul Feher
no flags Details | Diff | Splinter Review
SettingsMenuItems test V1.1 (6.97 KB, patch)
2013-01-15 07:15 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SettingsMenuItems test V2 (6.87 KB, patch)
2013-01-16 02:47 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SettingsMenuItems test V3 (6.98 KB, patch)
2013-01-17 00:32 PST, Paul Feher
no flags Details | Diff | Splinter Review
SettingsMenuItems test V4 (6.89 KB, patch)
2013-01-18 06:03 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SettingsMenuItems test V5 (6.88 KB, patch)
2013-01-24 01:20 PST, Paul Feher
jmaher: review+
Details | Diff | Splinter Review
SettingsMenuItems test V6 (6.81 KB, patch)
2013-01-30 05:29 PST, Paul Feher
gbrown: review+
Details | Diff | Splinter Review
SettingsMenuItems test V7 (6.82 KB, patch)
2013-02-01 01:45 PST, Paul Feher
gbrown: review+
Details | Diff | Splinter Review
SettingsMenuItems test V8 (6.82 KB, patch)
2013-02-04 02:20 PST, Paul Feher
gbrown: review+
Details | Diff | Splinter Review

Description Paul Feher 2013-01-15 06:53:11 PST
This bug tracks the Robocop tests part of Settings Menu Items' feature on Firefox for Android.
The tests check all the menu section and the sub-menus options available and their default values.
This patch integrates the following test cases:
Settings menu items 
Automatic updates - default option
Open and dismiss the Settings screen
Plugins option
Automatic updates menu
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-product=4&filter-productversion=55&filter-suite=68
Comment 1 Paul Feher 2013-01-15 07:10:01 PST
Created attachment 702300 [details] [diff] [review]
SettingsMenuItems test V1

Modified the selectMenuItem method in BaseTest.java.in to cover the changes of the menu layout (the tools submenu) on ICS+ devices.
Created tests to cover the tests done on Settings MenuItems.
Comment 2 Paul Feher 2013-01-15 07:15:52 PST
Created attachment 702304 [details] [diff] [review]
SettingsMenuItems test V1.1

Cleaned up some spaces
Comment 3 Joel Maher ( :jmaher) 2013-01-15 10:28:44 PST
I am unable to apply this patch:

jmaher@jmaher-MacBookPro:~/mozilla/src/objdir-droid/dist/test-package-stage/mochitest$ hg qpush
applying Bug830755.patch
patching file mobile/android/base/tests/BaseTest.java.in
bad hunk #1 @@ -335,26 +335,33 @@ abstract class BaseTest extends Activity
 (27 26 33 33)
patch failed, rejects left in working dir
errors during apply, please fix and refresh Bug830755.patch
Comment 4 Joel Maher ( :jmaher) 2013-01-15 10:38:28 PST
Comment on attachment 702304 [details] [diff] [review]
SettingsMenuItems test V1.1

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +348,1 @@
>          // Look for the 'More' menu if this device/OS uses it

please add appropriate spaces for indentation.

@@ +352,5 @@
> +            else {
> +	         mSolo.clickOnText("^Tools$");
> +            }
> +               mSolo.waitForText(itemName);
> +               mSolo.clickOnText(itemName);

too many spaces in these two lines.

::: mobile/android/base/tests/testSettingsMenuItems.java.in
@@ +16,5 @@
> +
> +// This patch tests the Sections present in the Settings Menu and the default values for them
> +public class testSettingsMenuItems extends PixelTest {
> +     int x;
> +     int y;

x,y are not descriptive.  These are defined as x/2 and y/2.  please use midWidth and midHeight.

@@ +26,5 @@
> +
> +    public void testSystemPages() {
> +        blockForGeckoReady();
> +	x = mDriver.getGeckoHeight()/2;
> +	y = mDriver.getGeckoWidth()/2;

x/y are flipflopped here.

@@ +41,5 @@
> +	{ "Plugins", "Enable", "Tap to play", "Disable"},
> +	{ "Cookies", "Enable", "Enabled, excluding 3rd party", "Disable"},
> +	{ "Clear private data", "Browsing & download history", "Form & search history", "Cookies & active logins", "Saved passwords", "Cache", "Offline website data", "Site preferences"},
> +	{ "Import from Android",  "Bookmarks", "History"}
> +	};

please add some blank lines between these to make reading easier.

@@ +51,5 @@
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +	selectMenuItem("Settings");
> +	checkMenuSections(menuItems);
> +	mSolo.scrollUp(); // Scolling up the page to have in view the top sections
> +	mSolo.scrollUp();

please add a comment here as to why you do this twice?

@@ +53,5 @@
> +	checkMenuSections(menuItems);
> +	mSolo.scrollUp(); // Scolling up the page to have in view the top sections
> +	mSolo.scrollUp();
> +	checkMenuSections(defaultSectionsValues);
> +	mSolo.scrollUp();

why do you have to scroll up again?

@@ +63,5 @@
> +            }
> +        }
> +    // This verifies the multiple option sections sub menus
> +    public void checkMenuSubSections(String multipleSectionsOptions [][]) {
> +	MotionEventHelper meh = new MotionEventHelper(getInstrumentation(), mDriver.getGeckoLeft(), mDriver.getGeckoTop());

we only use this if it is a phone, lets be smarter here and not initialize this if we don't need to.

@@ +65,5 @@
> +    // This verifies the multiple option sections sub menus
> +    public void checkMenuSubSections(String multipleSectionsOptions [][]) {
> +	MotionEventHelper meh = new MotionEventHelper(getInstrumentation(), mDriver.getGeckoLeft(), mDriver.getGeckoTop());
> +	int i,j = 0;
> +            for(i = 0; i <= multipleSectionsOptions [j].length; i++) {

nit add space between 'for('

@@ +69,5 @@
> +            for(i = 0; i <= multipleSectionsOptions [j].length; i++) {
> +		String item = multipleSectionsOptions [i][j];
> +		mAsserter.ok(mSolo.waitForText(item), "Waiting for  " + item + "  optin", "The " + item + "  optin is present");
> +		mSolo.clickOnText(item);
> +		for(j = 1; j < multipleSectionsOptions [i].length; j++)	{

nit add space between 'for('

@@ +71,5 @@
> +		mAsserter.ok(mSolo.waitForText(item), "Waiting for  " + item + "  optin", "The " + item + "  optin is present");
> +		mSolo.clickOnText(item);
> +		for(j = 1; j < multipleSectionsOptions [i].length; j++)	{
> +		    String subMenu = multipleSectionsOptions [i][j];
> +                    mAsserter.ok(mSolo.waitForText(subMenu), "Waiting for  " + subMenu + "  optin", "The " + subMenu + "  optin is present");    

nit: extra spaces at the end of the above line.

@@ +73,5 @@
> +		for(j = 1; j < multipleSectionsOptions [i].length; j++)	{
> +		    String subMenu = multipleSectionsOptions [i][j];
> +                    mAsserter.ok(mSolo.waitForText(subMenu), "Waiting for  " + subMenu + "  optin", "The " + subMenu + "  optin is present");    
> +                    // When reaching the specified sub menu section, for phone devices you have to scroll down the list in order to check all the options
> +		    if(subMenu == "Offline website data"){

nit, add spaces between 'if (' and '){'
Comment 5 Paul Feher 2013-01-16 02:47:52 PST
Created attachment 702739 [details] [diff] [review]
SettingsMenuItems test V2

Made all the changes asked for in the review.
Comment 6 Joel Maher ( :jmaher) 2013-01-16 07:00:41 PST
this still fails for me on my local tegra:
0 INFO SimpleTest START
1 INFO TEST-START | testSettingsMenuItems
2 INFO TEST-PASS | testSettingsMenuItems | Awesomebar URL stayed the same - about:home should equal about:home
Exception caught during test!
junit.framework.AssertionFailedError: The text: ^More$ 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.BaseTest.selectMenuItem(BaseTest.java:354)
	at org.mozilla.fennec_jmaher.tests.testSettingsMenuItems.testSystemPages(testSettingsMenuItems.java:48)
	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)
3 INFO TEST-UNEXPECTED-FAIL | testSettingsMenuItems | Exception caught - junit.framework.AssertionFailedError: The text: ^More$ is not found!
4 INFO TEST-END | testSettingsMenuItems | finished in 66783ms
5 INFO TEST-START | Shutdown
6 INFO Passed: 1
7 INFO Failed: 1
8 INFO Todo: 0
9 INFO SimpleTest FINISHED
Comment 7 Joel Maher ( :jmaher) 2013-01-16 07:04:48 PST
Comment on attachment 702739 [details] [diff] [review]
SettingsMenuItems test V2

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

r- for failure, less style issues than before.  thanks for fixing what was mentioned last time.

::: mobile/android/base/tests/BaseTest.java.in
@@ +352,4 @@
>  
> +            // Look for the 'More' menu if this device/OS uses it
> +	    if (mDevice.version.equals("2.x")) {
> +                 mSolo.clickOnText("^More$");

I believe this is causing problems.  Our tegras are 2.3, but I believe they run in tablet mode.

@@ +357,5 @@
> +            else {
> +	         mSolo.clickOnText("^Tools$");
> +            }
> +               mSolo.waitForText(itemName);
> +               mSolo.clickOnText(itemName);

these two lines are indented more than they should be.

::: mobile/android/base/tests/testSettingsMenuItems.java.in
@@ +37,5 @@
> +	{ "Clear private data", "Browsing & download history", "Form & search history", "Cookies & active logins", "Saved passwords", "Cache", "Offline website data", "Site preferences"},
> +
> +	{ "Import from Android",  "Bookmarks", "History"}
> +
> +	};

the spacing here is too much.  I usually see a blank line before comment and line(s) of code.

@@ +55,5 @@
> +    }
> +    public void checkMenuSections(String menuItems []) {
> +            for (String item:menuItems) {
> +                mAsserter.ok(mSolo.waitForText(item), "Waiting for  " + item + "  optin", "The " + item + "  optin is present");
> +            }

this is indented too far.

@@ +60,5 @@
> +        }
> +    // This verifies the multiple option sections sub menus
> +    public void checkMenuSubSections(String multipleSectionsOptions [][]) {
> +	int i,j = 0;
> +            for (i = 0; i <= multipleSectionsOptions [j].length; i++) {

this entire for block is indented too far.

@@ +68,5 @@
> +		for (j = 1; j < multipleSectionsOptions [i].length; j++)	{
> +		    String subMenu = multipleSectionsOptions [i][j];
> +                    mAsserter.ok(mSolo.waitForText(subMenu), "Waiting for  " + subMenu + "  optin", "The " + subMenu + "  optin is present");
> +                    // When reaching the specified sub menu section, for phone devices you have to scroll down the list in order to check all the options
> +		    if (subMenu == "Offline website data") {

subMenu.equal("Offline website data")
Comment 8 Paul Feher 2013-01-17 00:32:49 PST
Created attachment 703199 [details] [diff] [review]
SettingsMenuItems test V3

This pach include the same changes in BaseTest.java like in the patch from Bug 822256. The changes in BaseTest.java. are made to avoid errors on tegras with Android 2.3, wich run in tablet mode.
Comment 9 Joel Maher ( :jmaher) 2013-01-17 07:47:53 PST
to make this work on the tegra, we need to have 2.x/tablet which uses the 'More' button:
diff -r 76f9578a9942 mobile/android/base/tests/BaseTest.java.in
--- a/mobile/android/base/tests/BaseTest.java.in	Wed Jan 16 12:34:54 2013 +0200
+++ b/mobile/android/base/tests/BaseTest.java.in	Thu Jan 17 10:46:58 2013 -0500
@@ -352,7 +352,8 @@
 	      Device mDevice = new Device();
 
               // Look for the 'More' menu if this device/OS uses it
-	      if (mDevice.version.equals("2.x") && mDevice.type.equals("phone")) {
+              // Our current tegras are 2.x/tablet and use "More".
+	      if (mDevice.version.equals("2.x")) {
                   mSolo.clickOnText("^More$");
 	      }
               else {


Please comment if you have concerns with this.
Comment 10 Paul Feher 2013-01-18 06:03:54 PST
Created attachment 703887 [details] [diff] [review]
SettingsMenuItems test V4

I've simplified the selectMenuItem in BaseTest.java and I hope that now it will work properly. The idea that you suggested was included in the previous patch version for this bug, so I was required to use another, simpler approached in modifying the BaseTest.java
Comment 11 Joel Maher ( :jmaher) 2013-01-23 10:58:41 PST
Comment on attachment 703887 [details] [diff] [review]
SettingsMenuItems test V4

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

I have seen this pass locally most of the time.  It fails on a panda board (android 4.0/tablet) all the time and it fails on try server:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19004095&tree=Try&full=1
Comment 12 Paul Feher 2013-01-24 01:20:26 PST
Created attachment 705779 [details] [diff] [review]
SettingsMenuItems test V5

I've made the necessary adjustments. Please verify
Comment 13 Joel Maher ( :jmaher) 2013-01-24 15:34:27 PST
Comment on attachment 705779 [details] [diff] [review]
SettingsMenuItems test V5

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

this is green on try.

::: mobile/android/base/tests/BaseTest.java.in
@@ +352,5 @@
> +	}
> +	else {
> +	    if (mSolo.searchText("(^More$|^Tools$)")) {
> +                mSolo.clickOnText("(^More$|^Tools$)");
> +	      }

nit: spacing of the }

::: mobile/android/base/tests/testSettingsMenuItems.java.in
@@ +19,5 @@
> +	midHeight = mDriver.getGeckoHeight()/2;
> +
> +        // In this First Section I've declared the strings that contain the Menu sections
> +	// The first string contains menu sections that do not have multiple option
> +	String menuItems [] = { "General", "About", "Sync", "Content", "Pinch to reflow text", "Privacy & Security", "Tell sites not to track me", "Remember passwords", "Use master password", "Show search suggestions", "Show product announcements", "Import & Export"};

nit: add a blank line here

@@ +21,5 @@
> +        // In this First Section I've declared the strings that contain the Menu sections
> +	// The first string contains menu sections that do not have multiple option
> +	String menuItems [] = { "General", "About", "Sync", "Content", "Pinch to reflow text", "Privacy & Security", "Tell sites not to track me", "Remember passwords", "Use master password", "Show search suggestions", "Show product announcements", "Import & Export"};
> +	// This string contains every menu section that have multiple selection options followed by the option that is selected by default
> +	String defaultSectionsValues [] = { "Automatic updates", "Only over Wi-Fi", "Character encoding", "Don't show menu","Plugins", "Tap to play", "Text size", "Tiny", "Cookies", "Enable"};

nit: add a blank line here

@@ +35,5 @@
> +	{ "Cookies", "Enable", "Enabled, excluding 3rd party", "Disable"},
> +
> +	{ "Clear private data", "Browsing & download history", "Form & search history", "Cookies & active logins", "Saved passwords", "Cache", "Offline website data", "Site preferences"},
> +
> +	{ "Import from Android",  "Bookmarks", "History"}

nit: remove blank lines between multipleSectionsOptions[][]

@@ +41,5 @@
> +
> +        selectMenuItem("Settings");
> +	// Dismiss the Settings screen and verify that the view is returned to about:home page
> +	mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +	verifyUrl("about:home");

nit: add a blank line here

@@ +45,5 @@
> +	verifyUrl("about:home");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +	mSolo.waitForText("Enter search"); // Waiting for page title to appear to be sure that is fully loaded before opening the menu
> +	selectMenuItem("Settings");
> +	checkMenuSections(menuItems);

nit: add a blank line here

@@ +48,5 @@
> +	selectMenuItem("Settings");
> +	checkMenuSections(menuItems);
> +	mSolo.scrollUp(); // Scolling up the page to have in view the top sections
> +	mSolo.scrollUp(); // Repeat scroll up because the top sections were not reached
> +	checkMenuSections(defaultSectionsValues);

nit: add a blank line here

@@ +54,5 @@
> +	checkMenuSubSections(multipleSectionsOptions);
> +    }
> +    public void checkMenuSections(String menuItems []) {
> +        for (String item:menuItems) {
> +             mAsserter.ok(mSolo.waitForText(item), "Waiting for  " + item + "  optin", "The " + item + "  optin is present");

nit: change optin to option

@@ +62,5 @@
> +    public void checkMenuSubSections(String multipleSectionsOptions [][]) {
> +	int i,j = 0;
> +        for (i = 0; i <= multipleSectionsOptions [j].length; i++) {
> +             String item = multipleSectionsOptions [i][j];
> +	     mAsserter.ok(mSolo.waitForText(item), "Waiting for  " + item + "  optin", "The " + item + "  optin is present");

nit: change optin to option

@@ +66,5 @@
> +	     mAsserter.ok(mSolo.waitForText(item), "Waiting for  " + item + "  optin", "The " + item + "  optin is present");
> +	     mSolo.clickOnText(item);
> +	     for (j = 1; j < multipleSectionsOptions [i].length; j++)	{
> +		  String subMenu = multipleSectionsOptions [i][j];
> +                  mAsserter.ok(mSolo.waitForText(subMenu), "Waiting for  " + subMenu + "  optin", "The " + subMenu + "  optin is present");

nit: change optin to option
Comment 14 Paul Feher 2013-01-30 05:29:45 PST
Created attachment 708093 [details] [diff] [review]
SettingsMenuItems test V6

I've made the required adjustments.
Comment 15 Geoff Brown [:gbrown] 2013-01-31 19:15:14 PST
Comment on attachment 708093 [details] [diff] [review]
SettingsMenuItems test V6

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

Taking the review since :jmaher is away for a little while.

Thanks Paul!

::: mobile/android/base/tests/testSettingsMenuItems.java.in
@@ +40,5 @@
> +	// Dismiss the Settings screen and verify that the view is returned to about:home page
> +	mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +	verifyUrl("about:home");
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +	mSolo.waitForText("Enter search"); // Waiting for page title to appear to be sure that is fully loaded before opening the menu

I think this is timing out because "search" should be "Search" (capitalize the S).

@@ +44,5 @@
> +	mSolo.waitForText("Enter search"); // Waiting for page title to appear to be sure that is fully loaded before opening the menu
> +
> +	selectMenuItem("Settings");
> +	checkMenuSections(menuItems);
> +	mSolo.scrollUp(); // Scolling up the page to have in view the top sections

nit: Scolling -> Scrolling

@@ +48,5 @@
> +	mSolo.scrollUp(); // Scolling up the page to have in view the top sections
> +	mSolo.scrollUp(); // Repeat scroll up because the top sections were not reached
> +
> +	checkMenuSections(defaultSectionsValues);
> +	mSolo.scrollUp(); // Scolling up the page to have in view the top sections again

"Scrolling"!
Comment 16 Paul Feher 2013-02-01 01:45:05 PST
Created attachment 708988 [details] [diff] [review]
SettingsMenuItems test V7

Thanks Geoff for the review. I corrected what was needed.
Comment 17 Geoff Brown [:gbrown] 2013-02-01 09:33:19 PST
Comment on attachment 708988 [details] [diff] [review]
SettingsMenuItems test V7

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

::: mobile/android/base/tests/testSettingsMenuItems.java.in
@@ +12,5 @@
> +    protected int getTestType() {
> +        return TEST_MOCHITEST;
> +    }
> +
> +    public void testSystemPages() {

Sorry - I missed this before: 

testSystemPages -> testSettingsMenuItems

...it doesn't harm anything, but could be confusing!
Comment 18 Paul Feher 2013-02-04 02:20:34 PST
Created attachment 709652 [details] [diff] [review]
SettingsMenuItems test V8

Yes, Sorry for that. I made the necessary corrections
Comment 19 Ryan VanderMeulen [:RyanVM] 2013-02-04 16:34:08 PST
https://hg.mozilla.org/mozilla-central/rev/d2e7bcb51f16

Note You need to log in before you can comment on or make changes to this bug.