888277 - Robocop: Add test for Private Browsing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: test private browsing

This bug will cover the progress made on creating a Robocop test to cover the Private Browsing feature

The test will open a page in private tab, a new private tab from a link's context menu and will check that the 2 pages opened in private tab will not be saved in the Browsers history.

Tryrun for the first attempt patch: https://tbpl.mozilla.org/?tree=Try&rev=1e70470b9503

Comment 1

[Geoff Brown [:gbrown]]

Comment on attachment 768936 [details] [diff] [review]
test private browsing

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

::: mobile/android/base/tests/BaseTest.java.in
@@ +702,5 @@
> +            tabEventExpecter.blockForEvent();
> +            contentEventExpecter.blockForEvent();
> +            loadUrl(url);
> +        } else {
> +            if (type.equals("normal")) {

I would much prefer to see:

if (type.equals("private")) {
   ...
} else if (type.equals("normal")) {
   ...
} else {
   ...
}

That reduces the indent level in your else's.

@@ +708,5 @@
> +                waitForText("Enter Search or Address");
> +                mActions.sendKeys(url);
> +                hitEnterAndWait();
> +                tabEventExpecter.blockForEvent();
> +                contentEventExpecter.blockForEvent();

You should .unregisterListener() these.

@@ +710,5 @@
> +                hitEnterAndWait();
> +                tabEventExpecter.blockForEvent();
> +                contentEventExpecter.blockForEvent();
> +            } else {
> +                mAsserter.dumpLog("Please select the new tab type - private or normal");

Better to assert here (mAsserter.ok(false, ...) -- we would not want this to go unnoticed.

@@ +715,5 @@
> +            }
> +        }
> +    }
> +
> +    public ArrayList<String> parseEventData(String eventData, String dataDelimiters) {

A descriptive comment here might encourage re-use.

@@ +731,5 @@
> +
> +    /**
> +     * Use:
> +     * data = "bookmarks" to get the bookmarks list
> +     * data = "history" to get the history list

It would be nice to describe the returned data as well.

@@ +753,5 @@
> +                    if (cursor.getString(cursor.getColumnIndex("url")) != null) {
> +                        firefoxData.add(cursor.getString(cursor.getColumnIndex("url")));
> +                    }
> +                    if(!cursor.isLast()) {
> +                            cursor.moveToNext();

Fix the spacing here please.

::: mobile/android/base/tests/testPrivateBrowsing.java.in
@@ +5,5 @@
> +import android.app.Activity;
> +import android.view.View;
> +import java.util.ArrayList;
> +
> +/* A simple test that creates 2 new tabs and checks that the tab count increases. */

This looks exactly the same as the comment for testNewTab!

@@ +31,5 @@
> +
> +        Activity activity = getActivity();
> +        tabCount = mDriver.findElement(activity, "tabs_counter");
> +        String tabCountValue = tabCount.getText();
> +        mAsserter.is(Integer.parseInt(tabCountValue), 1, "Tab count is 1 for Private tabs");

It seems odd that the test checks the tab count just this once...would be more complete to verify the tab count after opening subsequent tabs.

@@ +43,5 @@
> +        verifyContextMenuContent();
> +
> +        Actions.EventExpecter privateTabEventExpector = mActions.expectGeckoEvent("Tab:Added");
> +        mSolo.clickOnText("Open Link in Private Tab");
> +        String eventData = privateTabEventExpector.blockForEventData();

Don't forget to privateTabEventExpector.unregisterListener()!

@@ +47,5 @@
> +        String eventData = privateTabEventExpector.blockForEventData();
> +        parsedEventData = parseEventData(eventData, ",");
> +        mAsserter.ok(isTabPrivate(parsedEventData), "Checking if the new tab opened from the context menu was a private tab", "The tab was a private tab");
> +
> +        // Open a normal tab to check later that it was registered int he Firefox Browser History

typo: "in the"

@@ +53,5 @@
> +
> +        // Get the history list and check that the links open in private browsing are not saved
> +        ArrayList<String> firefoxHistory = getFirefoxData("history");
> +        mAsserter.ok(!firefoxHistory.contains(url), "Check that the link opened in the first private tab was not saved", url + " was not added to history");
> +        mAsserter.ok(!firefoxHistory.contains(blank), "Check that the link opened in the in the private tab from the context menu was not saved", blank + " was not added to history");

typo: "in the in the"

Comment 2

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

Comment on attachment 768936 [details] [diff] [review]
test private browsing

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

while most of gbrown's comments are small nits, I would like to review this after the patch has been updated.  Actually I will be out of town for a while and you could have gbrown review this patch.

Comment 3

[Adrian Tamas (:AdrianT)]

Attachment: test private browsing v1.1

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=66135874a659

Added extra descriptions for the new methods added in BaseTest, fixed all the neats, added the unregisteListeners and also added the checks for the tab count as requested.

Comment 4

[Geoff Brown [:gbrown]]

Unfortunately, testPrivateBrowsing is not running in the try run for Android 4.0, because of bug 891889.

Comment 5

[Geoff Brown [:gbrown]]

Comment on attachment 773292 [details] [diff] [review]
test private browsing v1.1

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

This looks fine.

Please wait for bug 891889 to be resolved and a clean try run with this test actually running on Android 4.0 before checking in.

Comment 6

[Adrian Tamas (:AdrianT)]

Now that bug 891889 has been fixed there still are no fails on the test: https://tbpl.mozilla.org/?tree=Try&rev=5e0f325c215d. Unfortunately there are a lot of restarts on pandas and I don't know if it's still a work in progress or if my test influeces this. Geoff should we land this test or wait until the run gets cleaner?

Comment 7

[Geoff Brown [:gbrown]]

That try looks good to me, other than the retries, and the retries should be an existing problem (bug 883539) -- I think your test is fine. However, there is some interesting work happening this week on those retries, and I would prefer not to complicate matters with a new test. If you don't mind, can we hold off on landing the new test for a few more days?

Comment 8

[Adrian Tamas (:AdrianT)]

I added myself to cc on all the bug and I will do a rerun on the tryserver after this is fixed. We can wait to integrate this when we are sure it is ok and does not cause any restarts

Comment 9

[Adrian Tamas (:AdrianT)]

Attachment: privateBrowsing.patch

Updated the test to work with the new about:home.

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=ae1f04592db5

Comment 10

[Geoff Brown [:gbrown]]

Comment on attachment 814868 [details] [diff] [review]
privateBrowsing.patch

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

Well done -- looks like a good test!

::: mobile/android/base/tests/StringHelper.java.in
@@ +17,5 @@
>      };
>      public static final int DEFAULT_BOOKMARKS_COUNT = DEFAULT_BOOKMARKS_TITLES.length;
>  
> +    // Context Menu menu items
> +    public static final String linkMenuItemsInPrivateTab [] = {"Open Link in Private Tab", "Copy Link", "Share Link", "Bookmark Link"};

Let's use the same style as existing string arrays in this file:
 - UPPER_CASE
 - new String[] { ...
and maybe use "context menu" in the name?

I'm thinking of something like:

public static final String[] CONTEXT_MENU_ITEMS_IN_PRIVATE_TAB = new String[] {
   "Open Link in Private Tab", 
   "Copy Link", 
   "Share Link", 
   "Bookmark Link"
};

::: mobile/android/base/tests/testPrivateBrowsing.java.in
@@ +18,5 @@
> + */
> +public class testPrivateBrowsing extends ContentContextMenuTest {
> +    private Element tabCount = null;
> +    private Element tabs = null;
> +    private int tabCountInt = 0;

Remove unused member variables here.

@@ +28,5 @@
> +
> +    public void testPrivateBrowsing() {
> +        String url = getAbsoluteUrl(StringHelper.ROBOCOP_BIG_LINK_URL);
> +        String blank = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> +        String blank2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_02_URL);

nit - I would prefer if these variables were named bigLinkUrl, blank1Url, blank2Url, or similar.

@@ +32,5 @@
> +        String blank2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_02_URL);
> +
> +        blockForGeckoReady();
> +
> +        inputAndLoadUrl("about:blank");

Why load about:blank here?

nit - use a StringHelper instead of the literal "about:blank"

@@ +35,5 @@
> +
> +        inputAndLoadUrl("about:blank");
> +
> +        addTab(url, true);
> +        waitForText(StringHelper.ROBOCOP_BIG_LINK_TITLE);

Consider calling waitForText inside addTab (pass the expected title as another parameter of addTab).

Consider asserting on waitForText also (mAsserter.ok(waitForText(title)...) -- so that a page load failure / irregularity is not missed.

@@ +43,5 @@
> +        // Open the link context menu and verify the options
> +        verifyContextMenuItems(StringHelper.linkMenuItemsInPrivateTab);
> +
> +        // Check that "Open Link in New Tab" is not in the menu
> +        mAsserter.ok(!mSolo.searchText("Open Link in New Tab"), "Checking that 'Open Link in New Tab' is not displayed in the context menu", "'Open Link in New Tab' is not displayed in the context menu");

Another StringHelper candidate? "Open Link in New Tab"

Comment 11

[Adrian Tamas (:AdrianT)]

Attachment: privateBrowsing.patch

(In reply to Geoff Brown [:gbrown] from comment #10)
> Comment on attachment 814868 [details] [diff] [review]
> privateBrowsing.patch
> 
> Review of attachment 814868 [details] [diff] [review]:
> @@ +32,5 @@
> > +        String blank2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_02_URL);
> > +
> > +        blockForGeckoReady();
> > +
> > +        inputAndLoadUrl("about:blank");
> 
> Why load about:blank here?
> 
> nit - use a StringHelper instead of the literal "about:blank"
Added a few about: pages in the StringHelper.
I am loading about:blank here to have a page loaded in the normal tab and not about:home


> @@ +43,5 @@
> > +        // Open the link context menu and verify the options
> > +        verifyContextMenuItems(StringHelper.linkMenuItemsInPrivateTab);
> > +
> > +        // Check that "Open Link in New Tab" is not in the menu
> > +        mAsserter.ok(!mSolo.searchText("Open Link in New Tab"), "Checking that 'Open Link in New Tab' is not displayed in the context menu", "'Open Link in New Tab' is not displayed in the context menu");
> 
> Another StringHelper candidate? "Open Link in New Tab"
Added the String array CONTEXT_MENU_ITEMS_IN_NORMAL_TAB for normal tab context menu content


New tryrun: https://tbpl.mozilla.org/?tree=Try&rev=9d4175d4b49c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/093a5e2b407c

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/093a5e2b407c