Closed Bug 888277 Opened 7 years ago Closed 6 years ago

Robocop: Add test for Private Browsing

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: AdrianT, Assigned: AdrianT)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch test private browsing (obsolete) — Splinter Review
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
Attachment #768936 - Flags: review?(jmaher)
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 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.
Attachment #768936 - Flags: review?(jmaher) → review-
Attached patch test private browsing v1.1 (obsolete) — Splinter Review
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.
Attachment #768936 - Attachment is obsolete: true
Attachment #773292 - Flags: review?(gbrown)
Unfortunately, testPrivateBrowsing is not running in the try run for Android 4.0, because of bug 891889.
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.
Attachment #773292 - Flags: review?(gbrown) → review+
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?
Flags: needinfo?(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?
Flags: needinfo?(gbrown)
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
Attached patch privateBrowsing.patch (obsolete) — Splinter Review
Updated the test to work with the new about:home.

Tryrun: https://tbpl.mozilla.org/?tree=Try&rev=ae1f04592db5
Attachment #773292 - Attachment is obsolete: true
Attachment #814868 - Flags: review?(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"
Attachment #814868 - Flags: review?(gbrown) → review+
(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
Attachment #814868 - Attachment is obsolete: true
Attachment #815308 - Flags: review?(gbrown)
Attachment #815308 - Flags: review?(gbrown) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/093a5e2b407c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.