Robocop: Add test for 'System Pages' feature

RESOLVED FIXED in Firefox 21

Status

()

Firefox for Android
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Paul Feher, Assigned: Paul Feher)

Tracking

18 Branch
Firefox 21
ARM
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

(Assignee)

Description

5 years ago
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/
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 693335 [details] [diff] [review]
SystemPages test V1.1

Cleaned up some spaces
Attachment #693333 - Attachment is obsolete: true
Attachment #693335 - Flags: review?(jmaher)
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.
Attachment #693335 - Flags: review?(jmaher) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 694346 [details] [diff] [review]
SystemPages test V2

Made all the changes asked for in the review.
Attachment #693335 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 694347 [details] [diff] [review]
SystemPages test V2.1

Cleaned up some spaces
Attachment #694346 - Attachment is obsolete: true
Attachment #694347 - Flags: review?(jmaher)
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.
Attachment #694347 - Flags: review?(jmaher) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 697434 [details] [diff] [review]
SystemPages test V3

Made all the changes asked for in the review.
Attachment #694347 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #697434 - Flags: review?(jmaher)
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;
Attachment #697434 - Flags: review?(jmaher) → review-
(Assignee)

Comment 9

5 years ago
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.
Attachment #697434 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #697855 - Flags: review?(jmaher)
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
Attachment #697855 - Flags: review?(jmaher) → review-
(Assignee)

Comment 11

5 years ago
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.
Attachment #697855 - Attachment is obsolete: true
Attachment #702697 - Flags: review?(jmaher)
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
seems like we switched from not finding tools to not finding more.
(Assignee)

Comment 14

5 years ago
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.
Attachment #702697 - Attachment is obsolete: true
Attachment #702697 - Flags: review?(jmaher)
Attachment #703180 - Flags: review?(jmaher)
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).
Attachment #703180 - Flags: review?(jmaher) → review-
(Assignee)

Comment 16

5 years ago
Created attachment 705817 [details] [diff] [review]
SettingsMenuItems test V7

I've made the necessary adjustments including the modifications in BaseTest.java. Please verify
Attachment #703180 - Attachment is obsolete: true
Attachment #705817 - Flags: review?(jmaher)
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.
Attachment #705817 - Flags: review?(jmaher) → review-
(Assignee)

Comment 18

4 years ago
Created attachment 708526 [details] [diff] [review]
SystemPages test V8

I've made the necessary adjustments. Please verify
Attachment #705817 - Attachment is obsolete: true
Attachment #708526 - Flags: review?(jmaher)
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.
Attachment #708526 - Flags: review?(jmaher) → review-
(Assignee)

Comment 20

4 years ago
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.
Attachment #708526 - Attachment is obsolete: true
Attachment #709027 - Flags: review?(gbrown)
Comment on attachment 709027 [details] [diff] [review]
SystemPages test V9

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

Thanks Paul. Looks good.
Attachment #709027 - Flags: review?(gbrown) → review+
https://hg.mozilla.org/mozilla-central/rev/b5185016ba60
Assignee: nobody → paul.feher
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.