747835 - Robocop: Add automated test for Back and Forward Buttons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[David Guo [:davidg]]

Litmus: https://litmus.mozilla.org/show_test.cgi?id=50242

Comment 1

[Adrian Tamas (:AdrianT)]

Hi David, are you working on this test at the moment? We are trying to get Robocop tests done for the basic smoke tests and I could work on this if you are not currently working on it.

Thanks

Comment 2

[Adrian Tamas (:AdrianT)]

Attachment: Tab History test v1

This is a robocop test that covers the use of the Forward, Back and Reload buttons to navigate the current tab history. By testing the reload button it also covers the test for bug 747837.

The test starts by determining the device OS (pre or post Honeycomb) and then the device size in order to run the test depending on the different UIs. I am assuming in the test that a device with a both the width and height larger then 480 pixels is a tablet.

During the testing of the bug I have noticed the fails covered in bug 798816 on low end devices and this is why I also made changes to the VerifyPageTitle method. The title of the page may not be updated in time when checked and the URL is found instead causing the VerifyPageTitle test to fail. The change in this patch introduces a delay that allows for the title to be updated.

Comment 3

[Adrian Tamas (:AdrianT)]

Attachment: Tab History test v1.1

Cleaned up some extra spaces

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 687718 [details] [diff] [review]
Tab History test v1.1

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

overall this is a good start.  A lot of little details I would like to see fixed before r+.  

This also makes me wonder if we should abstract our menus a bit more.  Also there is a lot of checking for tablet vs phone, if this was abstracted out, we could create a menu or navigation class and instantiate it with phone or tablet.  This would Allow us to call navigation.back(), etc...  This entire concept isn't required for a r+, it just seems like a nice to have or a followup bug.

::: mobile/android/base/tests/testTabHistory.java.in
@@ +11,5 @@
> +
> +public class testTabHistory extends PixelTest {
> +
> +
> +    private Boolean tablet;

maybe icsTablet to be complete.

@@ +12,5 @@
> +public class testTabHistory extends PixelTest {
> +
> +
> +    private Boolean tablet;
> +    private Boolean icsOSphone;

can we call this 'icsPhone' instead?

@@ +29,5 @@
> +
> +    public void testTabHistory() {
> +        blockForGeckoReady();
> +
> +        // Get the screen size - used to determin if tablet or phone

s/determin/determine/

@@ +42,5 @@
> +
> +        // Get the buttons from the tablet UI - will be null if the UI is not tablet
> +        Element fwdBtn = mDriver.findElement(getActivity(), "forward");
> +        Element backBtn = mDriver.findElement(getActivity(), "back");
> +        Element reloadBtn = mDriver.findElement(getActivity(), "reload");

we shouldn't get these unless we know we have a tablet.

@@ +49,5 @@
> +        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> +        if (mSolo.waitForText("More")){
> +        icsOSphone = false;
> +        tablet = false;
> +        }

I don't like making this decision based on the 'More' key, can we just use the resolution mentioned below?  Maybe other factors?

@@ +80,5 @@
> +            mSolo.waitForText("Browser Blank Page 02");
> +        }
> +
> +        // Go to the first page
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);

why not use the backBtn for the tablet?

@@ +94,5 @@
> +            mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> +            mSolo.waitForText("^Share$");
> +            if (icsOSphone) {
> +            fwdMenuBtn.click();
> +            mSolo.waitForText("Browser Blank Page 02");

please indent here

@@ +98,5 @@
> +            mSolo.waitForText("Browser Blank Page 02");
> +            }
> +            else {
> +            mSolo.clickOnText("^Forward$");
> +            mSolo.waitForText("Browser Blank Page 02");

please indent here.

Comment 5

[Adrian Tamas (:AdrianT)]

(In reply to Joel Maher (:jmaher) from comment #4)
> @@ +49,5 @@
> > +        mActions.sendSpecialKey(Actions.SpecialKey.MENU);
> > +        if (mSolo.waitForText("More")){
> > +        icsOSphone = false;
> > +        tablet = false;
> > +        }
> 
> I don't like making this decision based on the 'More' key, can we just use
> the resolution mentioned below?  Maybe other factors?

I can look forward into this but I am not sure there is another way to check for this. The resolution would not be very accurate I think because for instance a Samsung Galaxy S2 can run both Gingerbread (Factory default) or ICS (System update build offered for the device). I can try and look forward into this but at this point this is the only way I could think of to make a difference between the menus for ICS and Gingerbead devices.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Lets file a bug to find a better solution for detecting phone vs tablet.  I also see abstracting this out:

DeviceClass myDevice = detectDevice()
# myDevice.type = tablet
# myDevice.version = ics
# myDevice.width = 400
# myDevice.height = 600
# myDevice.rotate() rotate the display

Then for the navigation stuff, it could be more like this:
MenuNavigation nav = newMenuNavigation(myDevice)
nav.back()
nav.more()
nav.forward()

Comment 7

[Adrian Tamas (:AdrianT)]

(In reply to Joel Maher (:jmaher) from comment #6)
> Lets file a bug to find a better solution for detecting phone vs tablet.  I
> also see abstracting this out:
> 
> DeviceClass myDevice = detectDevice()
> # myDevice.type = tablet
> # myDevice.version = ics
> # myDevice.width = 400
> # myDevice.height = 600
> # myDevice.rotate() rotate the display
> 
> Then for the navigation stuff, it could be more like this:
> MenuNavigation nav = newMenuNavigation(myDevice)
> nav.back()
> nav.more()
> nav.forward()

Created the follow-up bug 818390 for this

Comment 8

[Adrian Tamas (:AdrianT)]

Attachment: Tab History test v2

Kept the changes to fix bug 798816.
Added the Device and Navigation classes in BaseTest to make it available in other tests.
Remade the patch for back, forward (bug 747835) and reload (bug 747837) to use the new classes.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 691265 [details] [diff] [review]
Tab History test v2

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

this is looking really good.  Some whitespace nits here.  Does this run for you?

::: mobile/android/base/tests/BaseTest.java.in
@@ +390,5 @@
> +        public String version; // 2.x or 3.x or 4.x
> +        public String type; // tablet or phone
> +        public int width;
> +        public int height;
> +

can we have a constructor which does a detectDevice()?

@@ +422,5 @@
> +                type = "phone";
> +            }
> +        }
> +
> +        public void rotate(){

nit: space between () and {

@@ +436,5 @@
> +    class Navigation {
> +        private String devType;
> +        private String osVersion;
> +
> +        public Navigation(Device mDevice){

nit: space between () and {

@@ +442,5 @@
> +            osVersion = mDevice.version;
> +        }
> +
> +        public void back(){
> +            if (devType == "tablet"){

nit: space between () and {

@@ +452,5 @@
> +            }
> +        }
> +
> +        public void forward(){
> +            if (devType == "tablet"){

nit: space between () and {

@@ +470,5 @@
> +            }
> +        }
> +
> +        public void reload(){
> +            if (devType == "tablet"){

nit: space between () and {

::: mobile/android/base/tests/testTabHistory.java.in
@@ +4,5 @@
> +import @ANDROID_PACKAGE_NAME@.*;
> +import android.app.Activity;
> +import android.util.Log;
> +import android.widget.ImageButton;
> +import android.widget.ListView;

do we need ImageButton and ListView?

Comment 10

[Adrian Tamas (:AdrianT)]

I originally tested this on an HTC Desire(Android 2.2), a Samsung Galaxy S2 (4.0.4) and a Samsung Galaxy Tab 2 7.0 (Android 4.0.4). This worked on each of the devices and they were correctly identified in each case.

I will make the necessary changes to the patch and update it shortly

Comment 11

[Adrian Tamas (:AdrianT)]

Attachment: Tab History test v2.1

Added the detctDevice method to the Device constructor, added the necessary spaces and cleared the unused imports/variables from testTabHistory.java.in. Retested everything against the 3 devices (HTC Desire, Samsung Galaxy S2 and Samsung Galaxy Tab 2 7.0)

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 692230 [details] [diff] [review]
Tab History test v2.1

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

thanks!

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47edb66a0137

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47edb66a0137

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 692230 [details] [diff] [review]
Tab History test v2.1

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

I just ran into some of this code while looking into bug 814282 and there are some additional things that should be fixed. Please file a new bug for these issues.

::: mobile/android/base/tests/BaseTest.java.in
@@ +18,5 @@
>  import java.io.IOException;
>  import java.lang.reflect.Method;
>  import java.util.HashMap;
> +import android.os.Build;
> +import android.util.DisplayMetrics;

The imports should be ordered as described at https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Java_practices

@@ +389,5 @@
> +        Build.VERSION mBuildVersion;
> +        public String version; // 2.x or 3.x or 4.x
> +        public String type; // tablet or phone
> +        public int width;
> +        public int height;

The version, type, width, and height variables should be made final, and the detectDevice() code should be inlined into the constructor.

@@ +399,5 @@
> +        private void detectDevice() {
> +            // Determine device version
> +            mBuildVersion = new Build.VERSION();
> +            int mSDK = mBuildVersion.SDK_INT;
> +            if (mSDK < Build.VERSION_CODES.HONEYCOMB) {

The "m" prefix on variables should only be used on class variables. mSDK is a local variable and shouldn't have that prefix. mBuildVersion is a class variable but it is never used anywhere other than these two lines, and should become a local variable. Also you don't need to create a new Build.VERSION object, you should just use Build.VERSION.SDK_INT.

Comment 17

[Adrian Tamas (:AdrianT)]

Thanks for the observation on the code. Actually the code in this patch has been changed since it was landed. I'll look over the existing code in the latest versions of the sources and see what changes are needed.

Comment 18

[Adrian Tamas (:AdrianT)]

New bug to cover the code cleanup filed and patch added - bug 864280