Last Comment Bug 822256 - Robocop: Add test for 'System Pages' feature
: Robocop: Add test for 'System Pages' feature
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: 18 Branch
: ARM Android
: -- normal (vote)
: Firefox 21
Assigned To: Paul Feher
:
:
Mentors:
Depends on:
Blocks: 744859
  Show dependency treegraph
 
Reported: 2012-12-17 07:09 PST by Paul Feher
Modified: 2013-02-04 16:33 PST (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
SystemPages test V1 (7.77 KB, patch)
2012-12-18 04:46 PST, Paul Feher
no flags Details | Diff | Splinter Review
SystemPages test V1.1 (7.72 KB, patch)
2012-12-18 04:56 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SystemPages test V2 (6.21 KB, patch)
2012-12-20 04:55 PST, Paul Feher
no flags Details | Diff | Splinter Review
SystemPages test V2.1 (6.20 KB, patch)
2012-12-20 05:01 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SystemPages test V3 (6.06 KB, patch)
2013-01-03 06:21 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SystemPages test V4 (6.20 KB, patch)
2013-01-04 02:11 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SystemPages test V5 (5.73 KB, patch)
2013-01-16 00:21 PST, Paul Feher
no flags Details | Diff | Splinter Review
SystemPages test V6 (6.48 KB, patch)
2013-01-16 23:33 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SettingsMenuItems test V7 (6.20 KB, patch)
2013-01-24 04:40 PST, Paul Feher
jmaher: review-
Details | Diff | Splinter Review
SystemPages test V8 (6.42 KB, patch)
2013-01-31 04:09 PST, Paul Feher
gbrown: review-
Details | Diff | Splinter Review
SystemPages test V9 (6.58 KB, patch)
2013-02-01 05:03 PST, Paul Feher
gbrown: review+
Details | Diff | Splinter Review

Description Paul Feher 2012-12-17 07:09:47 PST
This bug tracks the Robocop tests part of System Pages feature on Firefox for Android.
The tests check first the loading of system pages from the awesome bar and then from Firefox menu.

about:rights https://moztrap.mozilla.org/manage/case/1029/
Opening a local page (about:) in another local page should open in the same tab: https://moztrap.mozilla.org/manage/case/1030/
The search input is not in focus https://moztrap.mozilla.org/manage/case/1033/
about:home and about:firefox: https://moztrap.mozilla.org/manage/case/1027/
Comment 1 Paul Feher 2012-12-18 04:46:42 PST
Created attachment 693333 [details] [diff] [review]
SystemPages 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 system pages.
Comment 2 Paul Feher 2012-12-18 04:56:25 PST
Created attachment 693335 [details] [diff] [review]
SystemPages test V1.1

Cleaned up some spaces
Comment 3 Joel Maher ( :jmaher) 2012-12-18 07:53:59 PST
Comment on attachment 693335 [details] [diff] [review]
SystemPages test V1.1

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

Great start on this.

A few nits:
* clean up all whitespace.  Look for pink in the splinter review
* ensure all indentation is correct at 4 spaces for each level of code

As mentioned in the rest of the review, lets refactor the test to make it more compact and reuse code as much as possible by utilizing simple loops.

::: mobile/android/base/tests/BaseTest.java.in
@@ -337,2 @@
>          //build the item name ready to be used
> -        String itemName = "^" + menuItemName + "$";

we did we remove the ^ and $ from item name?  that will ensure we match on the name and only the name.

@@ +350,4 @@
>  
>          mSolo.waitForText(itemName);
> +        mSolo.clickOnText(itemName);  	
> +	}

please indent this properly and remove the trailing whitespace.

::: mobile/android/base/tests/testSystemPages.java.in
@@ +36,5 @@
> +	loadAndPaint(aboutFirefox);
> +       verifyTabCount(expectedTabCount);
> +
> +	// I've used here verifyUrl because verifyPageTitle fails at this step
> +	verifyUrl(aboutFirefox);

is there a bug about this?  What is the title?  I am thinking we could make this a simple loop:

for (url in urls) {
    title = url
    if (url == about:firefox)
        title = '';
    loadAndPaint(url);
    verifyPageTitle(title);
    verifyTabCount(expectedTabCount);
}

@@ +72,5 @@
> +	mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +
> +	PaintedSurface painted = waitForPaint(paintExpecter);
> +
> +	/* Verify that the search field is not in the focus by verifying that when 

nit: extra space after the 'when'

@@ +87,5 @@
> +
> +	// Set up listeners to catch the page load we're about to do
> +	Actions.EventExpecter tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
> +       Actions.EventExpecter contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");	
> +        

nit: extra white space on this line and at the end of the previous line.

@@ +118,5 @@
> +	verifyTabCount(expectedTabCount);
> +
> +	// Set up listeners to catch the page load we're about to do
> +	tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
> +       contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");

for page in [download, apps, about]:
    tabEventExpecter = ...
    contentEventExpecter = ...
    //Load page
    select MenuItem
    expectedTabcount++
    // wait for the new tab
    tabEventExpecter.blockForEvent()
    contentEventExpecter.blockForEvent()

This can simplify repeated code.
Comment 4 Paul Feher 2012-12-20 04:55:06 PST
Created attachment 694346 [details] [diff] [review]
SystemPages test V2

Made all the changes asked for in the review.
Comment 5 Paul Feher 2012-12-20 05:01:14 PST
Created attachment 694347 [details] [diff] [review]
SystemPages test V2.1

Cleaned up some spaces
Comment 6 Joel Maher ( :jmaher) 2012-12-20 09:10:26 PST
Comment on attachment 694347 [details] [diff] [review]
SystemPages test V2.1

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

I really like how cleaned up this is.  The looping makes this scalable and easier to read.

r- because this fails, although it might be a simple fix.

::: mobile/android/base/tests/testSystemPages.java.in
@@ +12,5 @@
> +Actions.EventExpecter tabEventExpecter;
> +Actions.EventExpecter contentEventExpecter;
> +Actions.RepeatedEventExpecter paintExpecter;
> +
> +final int expectedTabCount = 1;

indentation of these member variables

@@ +22,5 @@
> +
> +    public void testSystemPages() {
> +        blockForGeckoReady();
> +
> +        String Urls [] = {  "about:firefox", "about:rights", "about:home", "about:addons", "about:downloads", "about:buildconfig", "about:", "about:config"};

nit: lowercase: urls

@@ +24,5 @@
> +        blockForGeckoReady();
> +
> +        String Urls [] = {  "about:firefox", "about:rights", "about:home", "about:addons", "about:downloads", "about:buildconfig", "about:", "about:config"};
> +        String expectedUrls [] = { "about:apps", "about:downloads", "about:addons", "about:"};
> +        String MenuItems [] = { "Apps", "Downloads", "Add-ons", "Settings"};

nit: menuItems

@@ +28,5 @@
> +        String MenuItems [] = { "Apps", "Downloads", "Add-ons", "Settings"};
> +
> +        /* This first section loads system pages from the awesome bar
> +	and checks that the pages are loaded in the same tab */
> +        ceckUrl(Urls);

can we call this checkUrl?

@@ +35,5 @@
> +        the awesomebar and the keyboard, then verify that the previous about: page is loaded */
> +        paintExpecter = mActions.expectPaint();// Set up listener to catch the page load
> +
> +	mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);

is there a chance we could close the browser if we were not in the right place?

@@ +38,5 @@
> +	mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +
> +	PaintedSurface painted = waitForPaint(paintExpecter);
> +	verifyUrl("about:");

I get this on all my test runs:
12-21 00:45:09.366 I/Robocop (28041): 26 INFO TEST-UNEXPECTED-FAIL | testSystemPages | Awesomebar URL stayed the same - got about:config, expected about:

Is it possible that after clearing the keyboard and awesomebar that we are at the lats page in the url list "about:config" and this is working properly?

@@ +49,5 @@
> +     public void ceckUrl(String Urls []) {
> +            for (String url:Urls) {
> +                loadAndPaint(url);
> +                verifyTabCount(expectedTabCount);
> +                verifyUrl(url);

didn't we have to do something different for about:firefox last time?

@@ +53,5 @@
> +                verifyUrl(url);
> +            }
> +        }
> +     public void loadFromMenu(String MenuItems [], String expectedUrls []) {
> +            int mExpectedTabCount = expectedTabCount;

please use mExpectedTabCount for the class level variable:
int expectedTabCount = mExpectedTabCount;

@@ +58,5 @@
> +            for (String item:MenuItems) {
> +                mExpectedTabCount++;
> +                int i = Arrays.asList(MenuItems).indexOf(item);
> +                if (item != "Settings") {
> +                   mActions.sendSpecialKey(Actions.SpecialKey.BACK);

we send the BACK key for both conditions, lets pull this out of the if/else clauses.
Comment 7 Paul Feher 2013-01-03 06:21:08 PST
Created attachment 697434 [details] [diff] [review]
SystemPages test V3

Made all the changes asked for in the review.
Comment 8 Joel Maher ( :jmaher) 2013-01-03 07:40:07 PST
Comment on attachment 697434 [details] [diff] [review]
SystemPages test V3

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

overall this is great.  A couple small code style things, and the obvious thing that it fails to loadmenu items.

::: mobile/android/base/tests/testSystemPages.java.in
@@ +32,5 @@
> +        checkUrl(urls);
> +
> +	/* Verify that the search field is not in the focus by pressing Back to dismiss
> +        the keyboard, then verify that the previous about: page is loaded */
> +	loadUrl("about:config");  

nit: whitespace after the ;

@@ +57,5 @@
> +                expectedTabCount++;
> +		mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +
> +                int i = Arrays.asList(menuItems).indexOf(item);
> +                if (item != "Settings") {

can we do:
if (!item.equals("Settings")) {

@@ +63,5 @@
> +                   // Set up listeners to catch the page load we're about to do
> +                   tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
> +                   contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
> +
> +                   selectMenuItem(item);

this is failing for me.  It is failing to find '^Tools$'.  This is on the panda board.  From the adb logcat, it appears to be in the awesomebar, I tried tweaking this to add another menu.back and it still didn't work;
Comment 9 Paul Feher 2013-01-04 02:11:21 PST
Created attachment 697855 [details] [diff] [review]
SystemPages test V4

Made all the changes asked for in the review. Also I introduced a delay before the menu item is searched to be sure that the page loads.
I've verify this patch on several devices with different OS's:
 Samsung galaxy R - Android 2.3.4
 Samsung galaxy S2 - Android 4.0.3
 Acer Iconia Tab A500 - Android 3.1
 Asus EE Transformer TF101 - Android 4.0.3

Works every time for me.
Comment 10 Joel Maher ( :jmaher) 2013-01-04 09:02:19 PST
Comment on attachment 697855 [details] [diff] [review]
SystemPages test V4

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

the code looks great, but the test still fails.  It passes on the panda board with android 4.0.4, but it is failing on our tegra platform.  I tried to debug this and there is no Tools available.  I found that if I hacked around in the changes to BaseTest.java.in I could click the item all the time except for settings.  This is odd because the failure I see is (from logcat,  but in mochitest log):
01-05 00:58:42.596 I/Robocop (25731): 30 INFO TEST-PASS | testSystemPages | Awesomebar URL stayed the same - about:addons should equal about:addons
01-05 00:59:35.396 D/Robotium(25731): ^Tools$ not found. Have found: Tabs
01-05 00:59:35.396 D/Robotium(25731): ^Tools$ not found. Have found: Private
01-05 00:59:35.396 D/Robotium(25731): ^Tools$ not found. Have found: Synced
01-05 00:59:35.396 D/Robotium(25731): ^Tools$ not found. Have found: About Fennec
01-05 00:59:35.406 D/Robotium(25731): ^Tools$ not found. Have found: Apps
01-05 00:59:35.406 D/Robotium(25731): ^Tools$ not found. Have found: Downloads
01-05 00:59:35.406 D/Robotium(25731): ^Tools$ not found. Have found: Add-ons
01-05 00:59:35.406 D/Robotium(25731): ^Tools$ not found. Have found: Add-ons
01-05 00:59:35.406 D/Robotium(25731): ^Tools$ not found. Have found: 4
01-05 00:59:35.406 I/Robocop (25731): Exception caught during test!
01-05 00:59:35.406 I/Robocop (25731): junit.framework.AssertionFailedError: The text: ^Tools$ is not found!
01-05 00:59:35.406 I/Robocop (25731): 	at junit.framework.Assert.fail(Assert.java:47)
01-05 00:59:35.406 I/Robocop (25731): 	at junit.framework.Assert.assertTrue(Assert.java:20)
01-05 00:59:35.406 I/Robocop (25731): 	at com.jayway.android.robotium.solo.Clicker.clickOnText(Clicker.java:291)
01-05 00:59:35.406 I/Robocop (25731): 	at com.jayway.android.robotium.solo.Solo.clickOnText(Solo.java:823)
01-05 00:59:35.406 I/Robocop (25731): 	at org.mozilla.fennec_jmaher.tests.BaseTest.selectMenuItem(BaseTest.java:357)
01-05 00:59:35.406 I/Robocop (25731): 	at org.mozilla.fennec_jmaher.tests.testSystemPages.loadFromMenu(testSystemPages.java:78)
01-05 00:59:35.406 I/Robocop (25731): 	at org.mozilla.fennec_jmaher.tests.testSystemPages.testSystemPages(testSystemPages.java:43)
01-05 00:59:35.406 I/Robocop (25731): 	at java.lang.reflect.Method.invokeNative(Native Method)
01-05 00:59:35.406 I/Robocop (25731): 	at java.lang.reflect.Method.invoke(Method.java:521)
01-05 00:59:35.406 I/Robocop (25731): 	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
01-05 00:59:35.406 I/Robocop (25731): 	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
01-05 00:59:35.406 I/Robocop (25731): 	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
01-05 00:59:35.406 I/Robocop (25731): 	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:130)
01-05 00:59:35.406 I/Robocop (25731): 	at junit.framework.TestCase.runBare(TestCase.java:127)
01-05 00:59:35.406 I/Robocop (25731): 	at junit.framework.TestResult$1.protect(TestResult.java:106)
01-05 00:59:35.406 I/Robocop (25731): 	at junit.framework.TestResult.runProtected(TestResult.java:124)
01-05 00:59:35.406 I/Robocop (25731): 	at junit.framework.TestResult.run(TestResult.java:109)
01-05 00:59:35.406 I/Robocop (25731): 	at junit.framework.TestCase.run(TestCase.java:118)
01-05 00:59:35.406 I/Robocop (25731): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
01-05 00:59:35.406 I/Robocop (25731): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
01-05 00:59:35.406 I/Robocop (25731): 	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
01-05 00:59:35.406 I/Robocop (25731): 	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
01-05 00:59:35.406 I/Robocop (25731): 31 INFO TEST-UNEXPECTED-FAIL | testSystemPages | Exception caught - junit.framework.AssertionFailedError: The text: ^Tools$ is not found!
01-05 00:59:35.406 I/Robocop (25731): 32 INFO TEST-END | testSystemPages | finished in 250642ms
01-05 00:59:35.406 I/Robocop (25731): 33 INFO TEST-START | Shutdown
01-05 00:59:35.416 I/Robocop (25731): 34 INFO Passed: 29
01-05 00:59:35.416 I/Robocop (25731): 35 INFO Failed: 1
01-05 00:59:35.416 I/Robocop (25731): 36 INFO Todo: 0
01-05 00:59:35.416 I/Robocop (25731): 37 INFO SimpleTest FINISHED
Comment 11 Paul Feher 2013-01-16 00:21:40 PST
Created attachment 702697 [details] [diff] [review]
SystemPages test V5

Modified the selectMenuItem method in BaseTest.java.in to avoid this error. I've checked on different Android versions and it works for me every time now. Please verify again.
Comment 12 Joel Maher ( :jmaher) 2013-01-16 06:28:50 PST
22 INFO TEST-PASS | testSystemPages | Awesomebar URL stayed the same - about: should equal about:
23 INFO TEST-PASS | testSystemPages | Awesomebar URL typed properly - about:config should equal about:config
24 INFO TEST-PASS | testSystemPages | Awesomebar URL stayed the same - about: should equal about:
25 INFO TEST-PASS | testSystemPages | The correct number of tabs are opened - 2 should equal 2
26 INFO TEST-PASS | testSystemPages | Awesomebar URL stayed the same - about:apps should equal about:apps
27 INFO TEST-PASS | testSystemPages | The correct number of tabs are opened - 3 should equal 3
28 INFO TEST-PASS | testSystemPages | Awesomebar URL stayed the same - about:downloads should equal about:downloads
29 INFO TEST-PASS | testSystemPages | The correct number of tabs are opened - 4 should equal 4
30 INFO TEST-PASS | testSystemPages | Awesomebar URL stayed the same - about:addons should equal about:addons
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.testSystemPages.loadFromMenu(testSystemPages.java:78)
	at org.mozilla.fennec_jmaher.tests.testSystemPages.testSystemPages(testSystemPages.java:43)
	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)
31 INFO TEST-UNEXPECTED-FAIL | testSystemPages | Exception caught - junit.framework.AssertionFailedError: The text: ^More$ is not found!
32 INFO TEST-END | testSystemPages | finished in 217491ms
33 INFO TEST-START | Shutdown
34 INFO Passed: 29
35 INFO Failed: 1
36 INFO Todo: 0
37 INFO SimpleTest FINISHED
Comment 13 Joel Maher ( :jmaher) 2013-01-16 06:29:57 PST
seems like we switched from not finding tools to not finding more.
Comment 14 Paul Feher 2013-01-16 23:33:49 PST
Created attachment 703180 [details] [diff] [review]
SystemPages test V6

I've made  changes in BaseTest.java. to avoid errors on tegras with Android 2.3, wich run in tablet mode.
Comment 15 Joel Maher ( :jmaher) 2013-01-23 11:00:12 PST
Comment on attachment 703180 [details] [diff] [review]
SystemPages test V6

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

this is failing on try server:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19004095&tree=Try&full=1

on try server we have android 2.3/tablet mode.  This passes for me locally though.  Also this fails similarly on the panda boards (android 4.0/tablet).
Comment 16 Paul Feher 2013-01-24 04:40:56 PST
Created attachment 705817 [details] [diff] [review]
SettingsMenuItems test V7

I've made the necessary adjustments including the modifications in BaseTest.java. Please verify
Comment 17 Joel Maher ( :jmaher) 2013-01-24 15:35:01 PST
Comment on attachment 705817 [details] [diff] [review]
SettingsMenuItems test V7

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

this is green, r- for the sleeps, feel free to argue your case if you really need them.

::: mobile/android/base/tests/testSystemPages.java.in
@@ +48,5 @@
> +            for (String url:urls) {
> +                loadAndPaint(url);
> +                verifyTabCount(mExpectedTabCount);
> +                verifyUrl(url);
> +            }

nit: 4 space indentation, the for loop is too far indented.

@@ +49,5 @@
> +                loadAndPaint(url);
> +                verifyTabCount(mExpectedTabCount);
> +                verifyUrl(url);
> +            }
> +        }

nit: } is indented too much.

@@ +62,5 @@
> +
> +                   // Set up listeners to catch the page load we're about to do
> +                   tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
> +                   contentEventExpecter = mActions.expectGeckoEvent("DOMContentLoaded");
> +		   mSolo.sleep(4000); // waiting for the page to load

Can we waitForText(item) or something like that?

@@ +74,5 @@
> +                   verifyUrl(expectedUrls[i]);
> +                 }
> +                 // Make sure the about: page was loaded without opening a new tab and verify the page Url
> +                 else {
> +		   mSolo.sleep(4000); // waiting for the page to load

can we do something else besides mSolo.sleep() here?  maybe waitfortext(???) on something in the page we are loading?

@@ +78,5 @@
> +		   mSolo.sleep(4000); // waiting for the page to load
> +                   selectMenuItem(item);
> +		   mSolo.waitForText("About");
> +                   mSolo.clickOnText("About");
> +		   mSolo.sleep(4000); // waiting for the page to load

can we waitForText(About)? or something similar?

@@ +83,5 @@
> +                   expectedTabCount--; // Decreasing since we don't expect this in a new tab
> +                   verifyTabCount(expectedTabCount);
> +                   verifyUrl(expectedUrls[i]); // Since the page is already loaded this should be instantly
> +                }
> +            }

nit: block is indented too far, 4 spaces only.
Comment 18 Paul Feher 2013-01-31 04:09:53 PST
Created attachment 708526 [details] [diff] [review]
SystemPages test V8

I've made the necessary adjustments. Please verify
Comment 19 Geoff Brown [:gbrown] 2013-01-31 16:46:38 PST
Comment on attachment 708526 [details] [diff] [review]
SystemPages test V8

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

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

This is looking really good and seems quite reliable. I am tempted to r+ but would like to see the BACK explained and spacing corrected (check the indent level throughout please: 4 spaces for each indent level).

::: mobile/android/base/tests/BaseTest.java.in
@@ +347,4 @@
>          //build the item name ready to be used
>          String itemName = "^" + menuItemName + "$";
>          mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> +	if (mSolo.waitForText(itemName)) {

I am a little uneasy about this because, in the fairly-common case that the item is hidden behind "More", we time-out on the first waitForText, adding 15 seconds or so to the test time. I cannot think of a more reliable option though...perhaps it is best this way.

@@ +352,5 @@
> +	}
> +	else {
> +	    if (mSolo.searchText("(^More$|^Tools$)")) {
> +                mSolo.clickOnText("(^More$|^Tools$)");
> +	      }

nit: spacing/indent level is off here

::: mobile/android/base/tests/testSystemPages.java.in
@@ +36,5 @@
> +	loadUrl("about:config");
> +
> +	paintExpecter = mActions.expectPaint();// Set up listener to catch the page load
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +	PaintedSurface painted = waitForPaint(paintExpecter);

This BACK + waitForPaint works fine...but it seems a little unusual. Can you add a comment explaining what's happening here?

@@ +54,5 @@
> +         }
> +     }
> +
> +     public void loadFromMenu(String menuItems [], String expectedUrls []) {
> +            int expectedTabCount = mExpectedTabCount;

nit: indent/spacing

@@ +59,5 @@
> +            for (String item:menuItems) {
> +		paintExpecter = mActions.expectPaint();// Set up listener to catch the page load
> +
> +                expectedTabCount++;
> +		mActions.sendSpecialKey(Actions.SpecialKey.BACK);

Again, comment on BACK.
Comment 20 Paul Feher 2013-02-01 05:03:48 PST
Created attachment 709027 [details] [diff] [review]
SystemPages test V9

> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +347,4 @@
> >          //build the item name ready to be used
> >          String itemName = "^" + menuItemName + "$";
> >          mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> > +	if (mSolo.waitForText(itemName)) {
> 
> I am a little uneasy about this because, in the fairly-common case that the
> item is hidden behind "More", we time-out on the first waitForText, adding
> 15 seconds or so to the test time. I cannot think of a more reliable option
> though...perhaps it is best this way.

Yes you are right but In some cases the time-out is more than welcome because it avoids accessing items from menu before this is properly loaded and populated. There where several problems, especially with the low end devices concerning this waiting period.

I added comments for the back actions also.
Thanks Geoff.
Comment 21 Geoff Brown [:gbrown] 2013-02-01 10:02:08 PST
Comment on attachment 709027 [details] [diff] [review]
SystemPages test V9

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

Thanks Paul. Looks good.
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-02-04 16:33:55 PST
https://hg.mozilla.org/mozilla-central/rev/b5185016ba60

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