745041 - Robocop: Add test for 'Clear History'

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug will track efforts into creating an automated Robocop test that covers the 'clear history' functionality of Fennec Native from Litmus ID: 53679.

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

Comment 1

[Andreea Pod]

Aaron, are you working on this? I was wondering if I could start looking on it and see what I can do here.

Comment 2

[Andreea Pod]

Attachment: clear_history

This is a test for clear history. I will create a follow up bug for clearing separately the private data for each option from "Clear private data" menu (where is possible), there I will extend this test.

Comment 3

[Andreea Pod]

Comment on attachment 691309 [details] [diff] [review]
clear_history

Joel, can you review this patch please?

Comment 4

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

Comment on attachment 691309 [details] [diff] [review]
clear_history

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

please address the comments in here.  I will run this on try server to see how stable it is with some other patches.

::: mobile/android/base/tests/BaseTest.java.in
@@ +30,5 @@
>      private static final String TARGET_PACKAGE_ID = "org.mozilla.gecko";
>      private static final String LAUNCH_ACTIVITY_FULL_CLASSNAME="@ANDROID_PACKAGE_NAME@.App";
>      private static final int VERIFY_URL_TIMEOUT = 2000;
> +    private static final int WAIT_FOR_CHILD_TIMEOUT = 2000;
> +    private static ListView listview = null;

shouldn't this be a member variable:
private ListView mlistview = null;

::: mobile/android/base/tests/testClearPrivateData.java.in
@@ +11,5 @@
> +import java.util.ArrayList;
> +
> +public class testClearPrivateData extends PixelTest  {
> +    private static final int WAIT_FOR_CHILD_TIMEOUT = 2000;
> +    private static ListView listview = null;

do we need listview and WAIT_FOR_CHILD_TIMEOUT defined in this class?

@@ +23,5 @@
> +        blockForGeckoReady();
> +	clearHistory();
> +    }
> +
> +    private void clearHistory(){

nit: please add a space between (){

@@ +24,5 @@
> +	clearHistory();
> +    }
> +
> +    private void clearHistory(){
> +        //loading a page so we are sure that there is at leats one history entry

nit: please add a space between '//l' and capitalize the l: "// Loading..."

@@ +31,5 @@
> +        verifyPageTitle("Browser Blank Page 01");
> +        //checking that the history list in not empty
> +        ListView hList = getHistoryList();
> +        mAsserter.ok(hList.getChildCount() > 0, "checking history exists", "history exists");
> +        //loading again the page in order to quit the awesomescreen

nit: fix whitespace and capitalization; 
can you just hit menu:back?

@@ +33,5 @@
> +        ListView hList = getHistoryList();
> +        mAsserter.ok(hList.getChildCount() > 0, "checking history exists", "history exists");
> +        //loading again the page in order to quit the awesomescreen
> +        loadAndPaint(url);
> +        //clearing private data

nit: whitspace and capitalization.

@@ +41,5 @@
> +        }
> +        mAsserter.ok(mSolo.searchButton("Clear data"), "checking clear button", "clear button exists");
> +        mSolo.clickOnButton("Clear data");
> +        mAsserter.is(mSolo.waitForText("Private data cleared"), true, "private data cleared successfully");
> +        //checking that history list is empty

nit: whitespace and capitalization.

::: mobile/android/base/tests/testHistoryTab.java.in
@@ +36,1 @@
>      private static final int WAIT_FOR_CHILD_TIMEOUT = 2000;

Do we need listview and WAIT_FOR_CHILD_TIMEOUT defined in this test anymore since they are moved to BaseTest.java.in by this patch?

Comment 5

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

I ran this test locally and I got errors:
0 INFO SimpleTest START
1 INFO TEST-START | testClearPrivateData
2 INFO TEST-PASS | testClearPrivateData | Awesomebar URL typed properly - http://mochi.test:8888/tests/robocop/robocop_blank_01.html should equal http://mochi.test:8888/tests/robocop/robocop_blank_01.html
3 INFO TEST-PASS | testClearPrivateData | Got the awesomebar - org.mozilla.fennec_jmaher.FennecNativeElement@443f4628 should not equal null
4 INFO TEST-PASS | testClearPrivateData | Page title match - Expected /Browser Blank Page 01/, got "Browser Blank Page 01"
5 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | checking history exists - history exists
Exception caught during test!
junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | checking history exists - history exists
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec_jmaher.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec_jmaher.tests.testClearPrivateData.clearHistory(testClearPrivateData.java:34)
	at org.mozilla.fennec_jmaher.tests.testClearPrivateData.testClearPrivateData(testClearPrivateData.java:24)
	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:124)
	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)
6 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | Exception caught - junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | checking history exists - history exists
7 INFO TEST-END | testClearPrivateData | finished in 100698ms
8 INFO TEST-START | Shutdown
9 INFO Passed: 3
10 INFO Failed: 2
11 INFO Todo: 0

Comment 6

[Andreea Pod]

(In reply to Joel Maher (:jmaher) from comment #4)
> Comment on attachment 691309 [details] [diff] [review]
> clear_history
> 
> Review of attachment 691309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please address the comments in here.  I will run this on try server to see
> how stable it is with some other patches.
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +30,5 @@
> >      private static final String TARGET_PACKAGE_ID = "org.mozilla.gecko";
> >      private static final String LAUNCH_ACTIVITY_FULL_CLASSNAME="@ANDROID_PACKAGE_NAME@.App";
> >      private static final int VERIFY_URL_TIMEOUT = 2000;
> > +    private static final int WAIT_FOR_CHILD_TIMEOUT = 2000;
> > +    private static ListView listview = null;
> 
> shouldn't this be a member variable:
> private ListView mlistview = null;
it is used in an inner class in the getHistoryList() method so needed to declare it here 
> 
> ::: mobile/android/base/tests/testClearPrivateData.java.in
> @@ +11,5 @@
> > +import java.util.ArrayList;
> > +
> > +public class testClearPrivateData extends PixelTest  {
> > +    private static final int WAIT_FOR_CHILD_TIMEOUT = 2000;
> > +    private static ListView listview = null;
> 
> do we need listview and WAIT_FOR_CHILD_TIMEOUT defined in this class?
removed them
> 
> @@ +23,5 @@
> > +        blockForGeckoReady();
> > +	clearHistory();
> > +    }
> > +
> > +    private void clearHistory(){
> 
> nit: please add a space between (){
done
> 
> @@ +24,5 @@
> > +	clearHistory();
> > +    }
> > +
> > +    private void clearHistory(){
> > +        //loading a page so we are sure that there is at leats one history entry
> 
> nit: please add a space between '//l' and capitalize the l: "// Loading..."
done
> 
> @@ +31,5 @@
> > +        verifyPageTitle("Browser Blank Page 01");
> > +        //checking that the history list in not empty
> > +        ListView hList = getHistoryList();
> > +        mAsserter.ok(hList.getChildCount() > 0, "checking history exists", "history exists");
> > +        //loading again the page in order to quit the awesomescreen
> 
> nit: fix whitespace and capitalization; 
> can you just hit menu:back?
done
> 
> @@ +33,5 @@
> > +        ListView hList = getHistoryList();
> > +        mAsserter.ok(hList.getChildCount() > 0, "checking history exists", "history exists");
> > +        //loading again the page in order to quit the awesomescreen
> > +        loadAndPaint(url);
> > +        //clearing private data
> 
> nit: whitspace and capitalization.
done
> 
> @@ +41,5 @@
> > +        }
> > +        mAsserter.ok(mSolo.searchButton("Clear data"), "checking clear button", "clear button exists");
> > +        mSolo.clickOnButton("Clear data");
> > +        mAsserter.is(mSolo.waitForText("Private data cleared"), true, "private data cleared successfully");
> > +        //checking that history list is empty
> 
> nit: whitespace and capitalization.
done
> 
> ::: mobile/android/base/tests/testHistoryTab.java.in
> @@ +36,1 @@
> >      private static final int WAIT_FOR_CHILD_TIMEOUT = 2000;
> 
> Do we need listview and WAIT_FOR_CHILD_TIMEOUT defined in this test anymore
> since they are moved to BaseTest.java.in by this patch?
done

I will upload the updated patch.

Comment 7

[Andreea Pod]

Attachment: clear_history_v1

Comment 8

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

Comment on attachment 701767 [details] [diff] [review]
clear_history_v1

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

just a small nit.  This test passes for me locally now, thanks for writing it!

::: mobile/android/base/tests/BaseTest.java.in
@@ +394,5 @@
> +        Activity awesomeBarActivity = clickOnAwesomeBar();
> +        mSolo.clickOnText("History");
> +
> +        final ArrayList<ListView> views = mSolo.getCurrentListViews();
> +        mSolo.waitForText("Today");

In a rare case this could be 'Yesterday'.  Lets make this more robust.

Comment 9

[Andreea Pod]

(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 701767 [details] [diff] [review]
> clear_history_v1
> 
> Review of attachment 701767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just a small nit.  This test passes for me locally now, thanks for writing
> it!
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +394,5 @@
> > +        Activity awesomeBarActivity = clickOnAwesomeBar();
> > +        mSolo.clickOnText("History");
> > +
> > +        final ArrayList<ListView> views = mSolo.getCurrentListViews();
> > +        mSolo.waitForText("Today");
> 
> In a rare case this could be 'Yesterday'.  Lets make this more robust.

"Today" text is found even after I cleared the History, mSolo.waitForText("Today"); does not results into timeout so "Today" will always be found. I can still add "Yesterday" if you think is better.

Comment 10

[Geoff Brown [:gbrown]]

(In reply to Andreea Pod from comment #9)
> "Today" text is found even after I cleared the History,
> mSolo.waitForText("Today"); does not results into timeout so "Today" will
> always be found. I can still add "Yesterday" if you think is better.

We have hit the case where the test starts near midnight and visits a link, creating a history entry for "Today", but then checks for "Today" a minute later, when the link is now filed under "Yesterday" -- see bug 794070; the patch there may be useful.

Comment 11

[Andreea Pod]

(In reply to Geoff Brown [:gbrown] from comment #10)
> We have hit the case where the test starts near midnight and visits a link,
> creating a history entry for "Today", but then checks for "Today" a minute
> later, when the link is now filed under "Yesterday" -- see bug 794070; the
> patch there may be useful.

Ok, done. Thank you for the review :) I'll upload the patch.

Comment 12

[Andreea Pod]

Attachment: clear_history_v1.1

Comment 13

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

Comment on attachment 702378 [details] [diff] [review]
clear_history_v1.1

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

thanks!

Comment 14

[Ed Morley [:emorley]]

Inbound link:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a99974fac17

Comment 15

[Ed Morley [:emorley]]

This is causing bug 830932 - please may someone take a look or back 3a99974fac17 out :-)

Comment 16

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

backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c533aced00c0

Comment 17

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

(In reply to Andreea Pod from comment #6)
> > > +    private static ListView listview = null;
> > 
> > shouldn't this be a member variable:
> > private ListView mlistview = null;
> it is used in an inner class in the getHistoryList() method so needed to
> declare it here 

it can still be a member variable. Please make it so.

>+        listview = null;
>+        boolean success = waitForTest(new BooleanTest() {
>+            public boolean test() {
>+               for (ListView view : views) {
>+                   if (view.getTag() == "history") {

This should be a .equals() instead of "==". The equals operator is not always going to return true for java strings.

Comment 18

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

>+        final ArrayList<ListView> views = mSolo.getCurrentListViews();
>+        mSolo.waitForText("(Today|Yesterday)");
>+
>+        listview = null;
>+        boolean success = waitForTest(new BooleanTest() {

Also, since the "views" arraylist is pulled out before this test is run, there is no point in making this a waitForTest - the test will always return exactly the same thing every time it is run.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3a99974fac17

Comment 20

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

this got landed and backed out, we have a bit more work todo.

Comment 21

[Ed Morley [:emorley]]

(In reply to Joel Maher (:jmaher) from comment #20)
> this got landed and backed out, we have a bit more work todo.

Needed [leave open] since the backout is yet to merge

Comment 22

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #21)
> since the backout is yet to merge

(otherwise m-cMerge can recognise and pair up backouts and their landings)

Comment 23

[Andreea Pod]

Attachment: clear_history_v1.2

Made the changes, review please?

Comment 24

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

Comment on attachment 704795 [details] [diff] [review]
clear_history_v1.2

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

r- due to the confusion I have with the assertions and hLIst == null.

::: mobile/android/base/tests/BaseTest.java.in
@@ +419,5 @@
> +
> +        ListView listview = null;
> +        ArrayList<ListView> views = mSolo.getCurrentListViews();
> +        for (ListView view : views) {
> +            if (view.getTag().equals("History")) {

In testHistoryTab we had 'history', now it is capitalized 'History'.  Is there a reason for this change?

::: mobile/android/base/tests/testClearPrivateData.java.in
@@ +30,5 @@
> +
> +        // Checking that the history list in not empty
> +        ListView hList = getHistoryList();
> +        if (hList == null) {
> +            mAsserter.is((hList == null),true,"hList is null");

if hList == null, we want to fail, right?  this condition should be mAsserter.is((hList != null), true, ...);

@@ +52,5 @@
> +
> +        // Checking that history list is empty
> +        hList = getHistoryList();
> +        if (hList == null) {
> +            mAsserter.is((hList == null),true,"hList is null");

should this trigger an error?

Comment 25

[Andreea Pod]

Attachment: clear_history_v1.3

I moved the getHistoryList() method from testHistoryTab.java.in into BaseTest.java.in and didn't change it like in the last patch version (didn't remove the waitForTest) because that way it wasn't getting the items list from history. So I had to declare historyList global variable because otherwise it is not working just like in the getBookmarksList() method. 

In the testClearPrivateData.java.in we don't need the "if (hList == null) " anymore.

Comment 26

[Andreea Pod]

Attachment: clear_history_v1.4

Cleared up some spaces that I saw only after I uploaded the patch.

Comment 27

[Geoff Brown [:gbrown]]

This passes try: https://tbpl.mozilla.org/?tree=Try&rev=e267ef3af739 and the code looks good to me.

The test fails often for me on my Galaxy S, because it takes a long time before the "Private data cleared" notification is shown: see bug 837274. This case can be accommodated with something like:

-        mAsserter.is(mSolo.waitForText("Private data cleared"),true, "private data cleared successfully");
+        mAsserter.is(mSolo.waitForText("Private data cleared",1,60000),true, "private data cleared successfully");

but I think it is probably better to keep this patch the way it is: a 20+ second delay is really a bug in the product, and should cause a test failure.

Comment 28

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

Comment on attachment 709053 [details] [diff] [review]
clear_history_v1.4

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

looks good.  I will run this on try with a few other patches which are about ready to land.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a1cd615cefb