Robocop: Add tests for doorhanger notifications

RESOLVED FIXED in Firefox 20

Status

()

defect
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Margaret, Assigned: AdrianT)

Tracking

Trunk
Firefox 20
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-fennec1.0 -)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

7 years ago
Posted patch patch (obsolete) — Splinter Review
After looking into bug 731963, I decided I want to clean up the doorhanger notification logic, but I decided we should have test coverage first!

I'm running into bug 716602 on my gingerbread device, and I'm having trouble getting the test urls to load on my ICS device, so I'm having trouble testing this right now, but here's a WIP.
(Reporter)

Comment 1

7 years ago
Posted patch WIP (obsolete) — Splinter Review
Er, uploading old version.
Assignee: nobody → margaret.leibovic
Attachment #602139 - Attachment is obsolete: true
Mind if I tweak the title so QA can look for this if need be?
Summary: Add tests for doorhanger notifications → Robocop: Add tests for doorhanger notifications
(Reporter)

Comment 3

7 years ago
(In reply to Aaron Train [:aaronmt] from comment #2)
> Mind if I tweak the title so QA can look for this if need be?

Not at all :)
(Reporter)

Updated

7 years ago
Blocks: 732336
blocking-fennec1.0: --- → -
Blocks: 744859
(Assignee)

Comment 4

7 years ago
I looked over the test and with a few modifications the test works now on Samsung Galaxy R (Android 2.3.4) and Samsung Galaxy Tab 2 7.0 (Android 4.0.4).

I had to change:
setTestType("mochitest");

to:
@Override
    protected int getTestType() {
        return TEST_MOCHITEST;
    }

Also I had to add "mSolo.clickOnCheckBox(0);" to uncheck the "Don't ask for this site" checkbox.

The last part of the test that opens the new tab needs to be redone because of the new Tabs Menu implementation but moving the addTab method from testNewTab to BaseTest.java.in (this could be useful in further tests also) and using this method to open the new tab works here.

If nobody is working on this test I could clean it up, do all the changes mentioned above and maybe add more doorhanger tests like choosing not to share location, save or not passwords etc.
Adrian - Feel free to work on this if you want to. We can help answer questions and review the patches.
(Reporter)

Comment 6

7 years ago
Un-assigning myself because I'm not actively working on this, but I can give feedback as needed.
Assignee: margaret.leibovic → nobody
(Assignee)

Updated

7 years ago
Assignee: nobody → adrian.tamas
(Assignee)

Comment 7

7 years ago
Posted patch doorhanger test v2 (obsolete) — Splinter Review
The patch was tested by me on Samsung Galaxy R (Android 2.3.4) and Samsung Galaxy Tab (Android 3.1).

I have kept the original patch and added the changes mentioned in Comment 4. Because AddTab() was moved to BaseTest.java.in I had to remove the method from testNewTab.java.in.

In addition to the original tests I added tests for denying access to the location, allowing and not allowing the storing of offline data and remembering and postponing a decision for the password manager doorhanger.

I have tried also for pop-up blocker doorhangers but for some reason the popups are not blocked. I have also tried loading popuptest.com/popuptest1.html thinking it had something to do with my test page but the popups were opened here also. When loading the pages manually on the test build the pop-up blocker works fine. Any idea why? Maybe this could be added as an improvement to the test.
Attachment #602140 - Attachment is obsolete: true
Attachment #669509 - Flags: review?(mark.finkle)
(Assignee)

Comment 8

7 years ago
Posted patch doorhanger test v2.1 (obsolete) — Splinter Review
Cleaned up a few tabs/white spaces in the code seen using the Splinter Review
Attachment #669509 - Attachment is obsolete: true
Attachment #669509 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Attachment #669519 - Flags: review?(mark.finkle)
Comment on attachment 669519 [details] [diff] [review]
doorhanger test v2.1

Brian is a good reviewer for this
Attachment #669519 - Flags: review?(mark.finkle) → review?(bnicholson)
Comment on attachment 669519 [details] [diff] [review]
doorhanger test v2.1

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

> +
> +    public void addTab(String url) {
> +
> +        Element tabs = null;

nit: remove the newline here


> +        boolean clicked = tabs.click();
> +        if (!clicked) {
> +            mAsserter.ok(clicked != false, "checking that tabs clicked", "tabs element clicked");
> +        }

I know you didn't write this original code, but there's no need to have an if statement around this assertion since the assertion will pass if it's true. This could just be simplified to:

mAsserter.ok(tabs.click(), "checking that tabs clicked", "tabs element clicked");


> +        if (!success) {
> +            mAsserter.ok(success != false, "waiting for add tab view", "add tab view available");
> +        }

Same here, you should just be able to write:

mAsserter.ok(success, "waiting for add tab view", "add tab view available");


> +        clicked = addTab.click();
> +        if (!clicked) {
> +            mAsserter.ok(clicked != false, "checking that add_tab clicked", "add_tab element clicked");
> +        }

Same as above.


Nice job with the doorhanger test! Very thorough.

::: mobile/android/base/tests/testDoorHanger.java.in
@@ +10,5 @@
> +   * geolocation doorhangers - sharing and not sharing the location works, the doorhanger is dismissed when a new tab is opened
> +   * offline storage permission doorhangers - allowing and not allowing offline storage dismisses the doorhanger
> +   * Password Manager doorhangers - Remember and Not Now options dismiss the doorhanger
> +*/
> +public class testDoorHanger extends PixelTest {

Is there a reason you're extending PixelTest here instead of BaseTest?
Attachment #669519 - Flags: review?(bnicholson) → review-
Comment on attachment 669519 [details] [diff] [review]
doorhanger test v2.1

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

Just some drive-by additions...

::: mobile/android/base/tests/testDoorHanger.java.in
@@ +34,5 @@
> +
> +        // Test basic "Share" button functionality
> +        mSolo.clickOnCheckBox(0);
> +        mSolo.clickOnText("Share");
> +        mActions.expectGeckoEvent("DOMContentLoaded").blockForEvent();

There's a race here, since the clickOnText could possibly generate the DOMContentLoaded before the event expecter is created. You want to do something like:

expecter = mActions.expectGeckoEvent("DOMContentLoaded");
mSolo.clickOnText("Share");
expecter.blockForEvent();

@@ +44,5 @@
> +
> +        // Test basic "Don't share" button functionality
> +        mSolo.clickOnCheckBox(0);
> +        mSolo.clickOnText("Don't share");
> +        mActions.expectGeckoEvent("DOMContentLoaded").blockForEvent();

Here too...
(Assignee)

Comment 12

7 years ago
Posted patch doorhanger test v3 (obsolete) — Splinter Review
Attachment #669519 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
Comment on attachment 676156 [details] [diff] [review]
doorhanger test v3

Made the suggested modifications in the patch
Attachment #676156 - Flags: review?(bnicholson)
Comment on attachment 676156 [details] [diff] [review]
doorhanger test v3

>+        mAsserter.ok(clicked != false, "checking that tabs clicked", "tabs element clicked");
>+        mAsserter.ok(success != false, "waiting for add tab view", "add tab view available");
>+        mAsserter.ok(clicked != false, "checking that add_tab clicked", "add_tab element clicked");

Using '<boolean> != false' is redundant. I would drop the '!= false' in these cases.
Attachment #676156 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 15

7 years ago
Posted patch doorhanger test v3.1 (obsolete) — Splinter Review
Fixed the asserts highlighted in the review
Attachment #676156 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #676528 - Flags: review?(bnicholson)
Attachment #676528 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 16

7 years ago
Can you please integrate this patch since the review was positive and I don't have the necessary rights to push patches to the repository
Keywords: checkin-needed
(Assignee)

Comment 18

7 years ago
(In reply to Geoff Brown [:gbrown] from comment #17)
> I pushed to try and encountered some failures:
> https://tbpl.mozilla.org/?tree=Try&rev=4deaaec79804

There is actually an issue for some reason with the changes done in the AddTab function that was moved from testNewTab.java.in to BaseTest.java.in but the error you pointed out is because geolocation is disabled on the devices so the app does not receive a geolocation success or failure. Can geolocation be enabled on the devices or the geolocation doorhanger test can't actually run in the labs? 

I will revert the changes to AddTab and retest and post a new patch as soon as possible.
(In reply to adrian tamas from comment #18)
> Can
> geolocation be enabled on the devices or the geolocation doorhanger test
> can't actually run in the labs? 

I am not an expert on this, but there was a similar problem and discussion in bug 757475.
(Assignee)

Comment 20

7 years ago
Adding Joel and Doug. Can we please get a feedback on this issue? Can geolocation permissions be turned on so allowing or refusing geolocation for a website can be tested using Robocop tests?

This is the simple test website for geolocation:
<html>
<title>Geolocation Test Page</title>
<body>
<script>
  function clb(position) {
    document.location = "robocop_geolocation_success.html";
  }
  function err(err) {
    document.location = "robocop_geolocation_error.html";
  }
  navigator.geolocation.getCurrentPosition(clb, err, {timeout: 0});
</script>
</body>
</html>
the geolocation mock provider is built into the browser if --enable-tests is true.  I believe this isn't as straightforward to do for the mobile browser as indicated in bug 757475.
(Assignee)

Comment 22

7 years ago
Posted patch doorhanger test v4 (obsolete) — Splinter Review
Redone the automated test to just test the display/hide of the geolocation doorhanger.

Corrected the error is the AddTab method in BaseTest -> was executing the second click() on tabs instead of the addTab element
Attachment #676528 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #682388 - Flags: review?(bnicholson)
Comment on attachment 682388 [details] [diff] [review]
doorhanger test v4

>+        // Load login page
>+        loadUrl(LOGIN_URL);
>+        mSolo.waitForText("to remeber password for");

>+        mSolo.clickOnText("Remember");
>+        mAsserter.is(mSolo.searchText("to remeber password for"), false, "Doorhanger notification is hidden");
>+

>+        // Reload the page and check that there is no doorhanger displayed
>+        loadUrl(LOGIN_URL);
>+        mAsserter.is(mSolo.searchText("to remeber password for"), false, "Doorhanger notification is hidden");

nit: "remeber"->"remember"
Attachment #682388 - Flags: review?(bnicholson) → review+
Comment on attachment 682388 [details] [diff] [review]
doorhanger test v4

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

::: mobile/android/base/tests/testDoorHanger.java.in
@@ +24,5 @@
> +        String BLANK_URL = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> +        String OFFLINE_STORAGE_URL = getAbsoluteUrl("/robocop/robocop_offline_storage.html");
> +        String LOGIN_URL = getAbsoluteUrl("/robocop/robocop_login.html");
> +
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();

Use blockForGeckoReady() instead, please!
(Assignee)

Comment 25

7 years ago
Posted patch doorhanger test v4.1 (obsolete) — Splinter Review
Added the asked modifications. Please check-in the patch if the review is positive.
Attachment #682388 - Attachment is obsolete: true
(Assignee)

Comment 26

7 years ago
Comment on attachment 685548 [details] [diff] [review]
doorhanger test v4.1

Hi Brian,

Sorry I forgot to add the review request on this. Can you please review the changes and if everything is ok check in the patch? 

Thanks,
Adrian
Attachment #685548 - Flags: review?(bnicholson)
Comment on attachment 685548 [details] [diff] [review]
doorhanger test v4.1

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

::: mobile/android/base/tests/testDoorHanger.java.in
@@ +24,5 @@
> +        String BLANK_URL = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> +        String OFFLINE_STORAGE_URL = getAbsoluteUrl("/robocop/robocop_offline_storage.html");
> +        String LOGIN_URL = getAbsoluteUrl("/robocop/robocop_login.html");
> +
> +        mActions.expectGeckoEvent("Gecko:Ready").blockForEvent();

Can you change this to blockForGeckoReady() as suggested in comment 24?
(Assignee)

Comment 28

7 years ago
Sorry I missed that change ... Fixed now
Attachment #685548 - Attachment is obsolete: true
Attachment #685548 - Flags: review?(bnicholson)
Attachment #691708 - Flags: review?(bnicholson)
Comment on attachment 691708 [details] [diff] [review]
doorhanger test v4.2

Looks good.
Attachment #691708 - Flags: review?(bnicholson) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3f278fad083b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.