Closed Bug 830755 Opened 11 years ago Closed 11 years ago

Robocop: Add test for 'Settings Menu Items' feature

Categories

(Firefox for Android Graveyard :: General, defect)

19 Branch
ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 21

People

(Reporter: paul.feher, Assigned: paul.feher)

References

Details

Attachments

(1 file, 8 obsolete files)

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
Attached patch SettingsMenuItems test V1 (obsolete) — Splinter Review
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.
Attached patch SettingsMenuItems test V1.1 (obsolete) — Splinter Review
Cleaned up some spaces
Attachment #702300 - Attachment is obsolete: true
Attachment #702304 - Flags: review?(jmaher)
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 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 '){'
Attachment #702304 - Flags: review?(jmaher) → review-
Attached patch SettingsMenuItems test V2 (obsolete) — Splinter Review
Made all the changes asked for in the review.
Attachment #702304 - Attachment is obsolete: true
Attachment #702739 - Flags: review?(jmaher)
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 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")
Attachment #702739 - Flags: review?(jmaher) → review-
Attached patch SettingsMenuItems test V3 (obsolete) — Splinter Review
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.
Attachment #702739 - Attachment is obsolete: true
Attachment #703199 - Flags: review?(jmaher)
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.
Attached patch SettingsMenuItems test V4 (obsolete) — Splinter Review
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
Attachment #703199 - Attachment is obsolete: true
Attachment #703199 - Flags: review?(jmaher)
Attachment #703887 - Flags: review?(jmaher)
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
Attachment #703887 - Flags: review?(jmaher) → review-
Attached patch SettingsMenuItems test V5 (obsolete) — Splinter Review
I've made the necessary adjustments. Please verify
Attachment #703887 - Attachment is obsolete: true
Attachment #705779 - Flags: review?(jmaher)
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
Attachment #705779 - Flags: review?(jmaher) → review+
Attached patch SettingsMenuItems test V6 (obsolete) — Splinter Review
I've made the required adjustments.
Attachment #705779 - Attachment is obsolete: true
Attachment #708093 - Flags: review?(jmaher)
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"!
Attachment #708093 - Flags: review?(jmaher) → review+
Attached patch SettingsMenuItems test V7 (obsolete) — Splinter Review
Thanks Geoff for the review. I corrected what was needed.
Attachment #708093 - Attachment is obsolete: true
Attachment #708988 - Flags: review?(jmaher)
Attachment #708988 - Flags: review?(jmaher) → review?(gbrown)
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!
Attachment #708988 - Flags: review?(gbrown) → review+
Yes, Sorry for that. I made the necessary corrections
Attachment #708988 - Attachment is obsolete: true
Attachment #709652 - Flags: review?(gbrown)
Attachment #709652 - Flags: review?(gbrown) → review+
https://hg.mozilla.org/mozilla-central/rev/d2e7bcb51f16
Assignee: nobody → paul.feher
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Depends on: 843947
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: