846340 - Robocop: Add test for 'Clear Site settings', 'Clear Saved passwords' and 'Clear history' options

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Catalin Suciu [:csuciu]]

This bug will track the creation of on automated test for clearing the site settings.
MozTrap testcase: https://moztrap.mozilla.org/manage/caseversion/35827/

Comment 1

[Catalin Suciu [:csuciu]]

Attachment: Patch ClearSiteSettings v1

Comment 2

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

Comment on attachment 719947 [details] [diff] [review]
Patch ClearSiteSettings v1

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

this fails for me locally and on tbpl:
https://tbpl.mozilla.org/?tree=Try&rev=d7e4964d6a3d&noignore=1

specifically on tegra.

Comment 3

[Catalin Suciu [:csuciu]]

Attachment: Patch ClearSiteSettings v2

Comment 4

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

Comment on attachment 720731 [details] [diff] [review]
Patch ClearSiteSettings v2

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

this is failing on my local tegra:

01-19 13:31:39.266 I/Robocop (23782): 4 INFO TEST-PASS | testClearPrivateData | checking clear button - clear button exists
01-19 13:31:40.356 W/InputManagerService( 1022): Window already focused, ignoring focus gain of: com.android.internal.view.IInputMethodClient$Stub$Proxy@4454aa38
01-19 13:32:01.286 I/Robocop (23782): 5 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | private data cleared successfully - got false, expected true
01-19 13:32:01.306 I/Robocop (23782): Exception caught during test!
01-19 13:32:01.306 I/Robocop (23782): junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | private data cleared successfully - got false, expected true
01-19 13:32:01.306 I/Robocop (23782): 	at junit.framework.Assert.fail(Assert.java:47)
01-19 13:32:01.306 I/Robocop (23782): 	at org.mozilla.fennec_jmaher.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
01-19 13:32:01.306 I/Robocop (23782): 	at org.mozilla.fennec_jmaher.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
01-19 13:32:01.306 I/Robocop (23782): 	at org.mozilla.fennec_jmaher.FennecMochitestAssert.is(FennecMochitestAssert.java:142)
01-19 13:32:01.306 I/Robocop (23782): 	at org.mozilla.fennec_jmaher.tests.testClearPrivateData.clearHistory(testClearPrivateData.java:47)
01-19 13:32:01.306 I/Robocop (23782): 	at org.mozilla.fennec_jmaher.tests.testClearPrivateData.testClearPrivateData(testClearPrivateData.java:23)
01-19 13:32:01.306 I/Robocop (23782): 	at java.lang.reflect.Method.invokeNative(Native Method)
01-19 13:32:01.306 I/Robocop (23782): 	at java.lang.reflect.Method.invoke(Method.java:521)
01-19 13:32:01.306 I/Robocop (23782): 	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
01-19 13:32:01.306 I/Robocop (23782): 	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
01-19 13:32:01.306 I/Robocop (23782): 	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
01-19 13:32:01.306 I/Robocop (23782): 	at org.mozilla.fennec_jmaher.tests.BaseTest.runTest(BaseTest.java:125)
01-19 13:32:01.306 I/Robocop (23782): 	at junit.framework.TestCase.runBare(TestCase.java:127)
01-19 13:32:01.306 I/Robocop (23782): 	at junit.framework.TestResult$1.protect(TestResult.java:106)
01-19 13:32:01.306 I/Robocop (23782): 	at junit.framework.TestResult.runProtected(TestResult.java:124)
01-19 13:32:01.306 I/Robocop (23782): 	at junit.framework.TestResult.run(TestResult.java:109)
01-19 13:32:01.306 I/Robocop (23782): 	at junit.framework.TestCase.run(TestCase.java:118)
01-19 13:32:01.306 I/Robocop (23782): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
01-19 13:32:01.306 I/Robocop (23782): 	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
01-19 13:32:01.306 I/Robocop (23782): 	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
01-19 13:32:01.306 I/Robocop (23782): 	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
01-19 13:32:01.306 I/Robocop (23782): 6 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | Exception caught - junit.framework.AssertionFailedError: 5 INFO TEST-UNEXPECTED-FAIL | testClearPrivateData | private data cleared successfully - got false, expected true
01-19 13:32:01.326 I/Robocop (23782): 7 INFO TEST-END | testClearPrivateData | finished in 122825ms
01-19 13:32:01.326 I/Robocop (23782): 8 INFO TEST-START | Shutdown
01-19 13:32:01.326 I/Robocop (23782): 9 INFO Passed: 3
01-19 13:32:01.336 I/Robocop (23782): 10 INFO Failed: 2
01-19 13:32:01.386 I/Robocop (23782): 11 INFO Todo: 0
01-19 13:32:01.396 I/Robocop (23782): 12 INFO SimpleTest FINISHED

Comment 5

[Catalin Suciu [:csuciu]]

Attachment: Patch ClearSiteSettings v3

Comment 6

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

Comment on attachment 723899 [details] [diff] [review]
Patch ClearSiteSettings v3

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

This is failing on try server on both tegra (android 2.3) and panda (android 4.0.4).

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

Comment 7

[Catalin Suciu [:csuciu]]

Attachment: Patch ClearSiteSettings v4

Try server run: https://tbpl.mozilla.org/?tree=Try&rev=f260110918f9

Comment 8

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

Comment on attachment 762614 [details] [diff] [review]
Patch ClearSiteSettings v4

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

a few nits, r- to ensure we cancel out the dialog at the end.  The Try server run looks pretty good.

::: mobile/android/base/tests/testClearPrivateData.java.in
@@ +13,5 @@
>  
>      public void testClearPrivateData() {
>          blockForGeckoReady();
>          clearHistory();
> +        clearSiteSettigs();

s/Settigs/Settings/

@@ +36,4 @@
>          mAsserter.ok(mSolo.searchButton("Clear data"),"checking clear button","clear button exists");
>          mSolo.clickOnButton("Clear data");
> +        ClearDataEventExpecter.blockForEvent();
> +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);

we are removing the confirmation check here?  is this intentional, accidental?

@@ +69,5 @@
> +
> +        // Re-trigger geolocation notification in new tab
> +        addTab(url); // Using addTab(url) because loadAndPaint(url) seems to fail at this step
> +        mSolo.waitForText("Share your location");
> +        mAsserter.is(mSolo.searchText("Share your location"), true, "Geolocation doorhanger has been displayed");

I would like to cancel this dialog out to make sure we don't leave things in a pending state.

Comment 9

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Be aware that the Settings are being modified in bug 872329 -- that might affect this test.

Comment 10

[Catalin Suciu [:csuciu]]

Attachment: Patch ClearSiteSettings v4.1

(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 762614 [details] [diff] [review]
> Patch ClearSiteSettings v4
> 
> Review of attachment 762614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a few nits, r- to ensure we cancel out the dialog at the end.  The Try
> server run looks pretty good.
> 
> ::: mobile/android/base/tests/testClearPrivateData.java.in
> @@ +13,5 @@
> >  
> >      public void testClearPrivateData() {
> >          blockForGeckoReady();
> >          clearHistory();
> > +        clearSiteSettigs();
> 
> s/Settigs/Settings/

Corrected.

> 
> @@ +36,4 @@
> >          mAsserter.ok(mSolo.searchButton("Clear data"),"checking clear button","clear button exists");
> >          mSolo.clickOnButton("Clear data");
> > +        ClearDataEventExpecter.blockForEvent();
> > +        mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> 
> we are removing the confirmation check here?  is this intentional,
> accidental?

This was intentional.

> 
> @@ +69,5 @@
> > +
> > +        // Re-trigger geolocation notification in new tab
> > +        addTab(url); // Using addTab(url) because loadAndPaint(url) seems to fail at this step
> > +        mSolo.waitForText("Share your location");
> > +        mAsserter.is(mSolo.searchText("Share your location"), true, "Geolocation doorhanger has been displayed");
> 
> I would like to cancel this dialog out to make sure we don't leave things in
> a pending state.

Added mActions.sendSpecialKey(Actions.SpecialKey.BACK); to dismiss the geolocation doorhanger

Comment 11

[Catalin Suciu [:csuciu]]

(In reply to Geoff Brown [:gbrown] from comment #9)
> Be aware that the Settings are being modified in bug 872329 -- that might
> affect this test.

I will update the test when this changes will land

Comment 12

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

Comment on attachment 763443 [details] [diff] [review]
Patch ClearSiteSettings v4.1

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

thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec7bccd33fc0

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bcccaf54ff1

https://tbpl.mozilla.org/php/getParsedLog.php?id=24250715&tree=Mozilla-Inbound

Comment 15

[Teodora Vermesan (:TeoVermesan)]

Attachment: Clear: Site settings, saved password, history

I have updated this test for clearSiteSettings option, which was backed out for frequent failures and I have integrated test for clearSavedPassword  option.
I have run the tests locally on multiple devices and on our Panda board and there were no failures.

Comment 16

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

Comment on attachment 832224 [details] [diff] [review]
Clear: Site settings, saved password, history

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

a lot of style changes needed.  I would also like to see this on try server with 20 retriggers and verify that this test case and subsequent test cases in the test run are stable.  Paul is in the process of obtaining try server access.

::: mobile/android/base/tests/testClearPrivateData.java
@@ +62,5 @@
> +
> +    public void clearSiteSettings() {
> +        String SHARE_STRINGS [] = {"Share your location with", "Share", "Don't share", "There are no settings to clear"};
> +        String TITLE = StringHelper.ROBOCOP_GEOLOCATION_TITLE;
> +        String URL = getAbsoluteUrl(StringHelper.ROBOCOP_GEOLOCATION_URL);

please make these variables lower case, i.e. shareStrings, title, url;

@@ +64,5 @@
> +        String SHARE_STRINGS [] = {"Share your location with", "Share", "Don't share", "There are no settings to clear"};
> +        String TITLE = StringHelper.ROBOCOP_GEOLOCATION_TITLE;
> +        String URL = getAbsoluteUrl(StringHelper.ROBOCOP_GEOLOCATION_URL);
> +        loadCheckDismiss(SHARE_STRINGS [1], URL, SHARE_STRINGS [0]);
> +        checkOption(SHARE_STRINGS [1], "Clear");

nit: why is there a space between 'share_strings' and '[1]'?  The existing style is to have SHARE_STRINGS[1], etc.

@@ +75,5 @@
> +
> +    public void clearPassword(){
> +        String PASSWORD_STRINGS [] = {"Save password", "Save", "Don't save"};
> +        String TITLE = StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE;
> +        String LOGIN_URL = getAbsoluteUrl(StringHelper.ROBOCOP_LOGIN_URL);

please make these local variables lowercase.

Comment 17

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 832224 [details] [diff] [review]
Clear: Site settings, saved password, history

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

Hope you don't mind a little more feedback!

::: mobile/android/base/tests/testClearPrivateData.java
@@ +33,5 @@
>          String blank1 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
>          String blank2 = getAbsoluteUrl(StringHelper.ROBOCOP_BLANK_PAGE_02_URL);
> +        String title = StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE;
> +        inputAndLoadUrl(blank1);
> +        waitForText(title);

Consider using verifyPageTitle here instead of waitForText.

@@ +41,5 @@
>          verifyHistoryCount(1);
>          clearPrivateData();
>  
> +        //checking for device
> +        checkDevice(title);

I would combine checkDevice with clearPrivateData -- whenever you go into Settings and clear data, you probably want to exit Settings afterwards.

@@ +76,5 @@
> +    public void clearPassword(){
> +        String PASSWORD_STRINGS [] = {"Save password", "Save", "Don't save"};
> +        String TITLE = StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE;
> +        String LOGIN_URL = getAbsoluteUrl(StringHelper.ROBOCOP_LOGIN_URL);
> +        blockForGeckoReady();

Why blockForGeckoReady now? 2 sub-tests have already been run -- surely the browser is up?

@@ +90,5 @@
> +        if (mDevice.type.equals("phone")) {
> +            mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +            mAsserter.ok(waitForText(StringHelper.PRIVACY_SECTION_LABEL), "waiting to perform one back", "one back");
> +            mActions.sendSpecialKey(Actions.SpecialKey.BACK);
> +            waitForText(title);

verifyPageTitle instead of waitForText here?

@@ +102,5 @@
> +    // Load a URL, verify that the doorhanger appears and dismiss it
> +    public void loadCheckDismiss(String option, String url, String message) {
> +        inputAndLoadUrl(url);
> +        waitForText(message);
> +        mAsserter.is(mSolo.searchText(message), true, "Doorhanger has been displayed");

If you use something like:
   ..., true, "Doorhanger "+message+" has been displayed");
it will be easier to follow the test progress from the output.

@@ +114,5 @@
> +        mSolo.clickLongOnView(toolbarView);
> +        mAsserter.ok(waitForText(StringHelper.CONTEXT_MENU_ITEMS_IN_URL_BAR [2]), "Waiting for the pop-up to open", "Pop up was openend");
> +        mSolo.clickOnText(StringHelper.CONTEXT_MENU_ITEMS_IN_URL_BAR [2]);
> +        mAsserter.ok(waitForText(option), "Verify that the option: " + option + " is in the list", "The option is in the list. There are settings to clear");
> +        mSolo.clickOnText(button);

Can you use mSolo.clickOnButton here?

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Attachment: Clear: Site settings, saved password, history v1.0

Made the necessary changes.
The try-server is all green: https://tbpl.mozilla.org/?tree=Try&rev=586252ae1d19

Comment 19

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

Comment on attachment 8336675 [details] [diff] [review]
Clear: Site settings, saved password, history v1.0

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

gbrown is more up to speed on robocop than I am, my review would be basic syntax and a few random questions.

Comment 20

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 8336675 [details] [diff] [review]
Clear: Site settings, saved password, history v1.0

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

::: mobile/android/base/tests/testClearPrivateData.java
@@ +62,5 @@
> +    public void clearSiteSettings() {
> +        String shareStrings[] = {"Share your location with", "Share", "Don't share", "There are no settings to clear"};
> +        String titleGeolocation = StringHelper.ROBOCOP_GEOLOCATION_TITLE;
> +        String url = getAbsoluteUrl(StringHelper.ROBOCOP_GEOLOCATION_URL);
> +        loadCheckDismiss(shareStrings[1], url, shareStrings[0]);

nit - I find the array of strings less readable than alternatives:
  loadCheckDismiss("Share", url, "Share your location with")
or
  String shareButton = "Share";
  String doorhangerTitle = "Share your location with";
  loadCheckDismiss(shareButton, url, doorhangerTitle);

@@ +74,5 @@
> +    public void clearPassword(){
> +        String passwordStrings[] = {"Save password", "Save", "Don't save"};
> +        String title = StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE;
> +        String loginUrl = getAbsoluteUrl(StringHelper.ROBOCOP_LOGIN_URL);
> +        loadCheckDismiss(passwordStrings[1], loginUrl, passwordStrings[0]);

Same here...but keep it as it is if you prefer this style.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/0aeb846dc2e7

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0aeb846dc2e7

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing frequent testMasterPassword failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e38b4476027
https://hg.mozilla.org/releases/mozilla-aurora/rev/f9ae278dead7

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Merge of the backout:
https://hg.mozilla.org/mozilla-central/rev/9e38b4476027

That said, the failure persists (I think the robocop chunking changed, which obscured the regression range), so I'll probably end up relanding this.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f28c346633a6