896557 - [fig] Fix and re-enable testWebContentContextMenu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Comment 1

[Adrian Tamas (:AdrianT)]

I can look into this. Assigning this to myself.

Comment 2

[Adrian Tamas (:AdrianT)]

Attachment: testWebContentContextMenu fix

This patch also contains the new clickOnScreen method in FennecNativeActions and the changes to focusURL from Bug 896574. I can remove those if the changes are integrated with bug 896574 but they are needed to make this work reliably.

This works for me locally every time on the Asus Transformer TF101 (Android 4.1)

Comment 3

[:Margaret Leibovic]

Comment on attachment 784961 [details] [diff] [review]
testWebContentContextMenu fix

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

Looks like this is on the right track, but I want us to sort out the patches from these various bugs to make sure there isn't duplicate logic in them.

::: mobile/android/base/tests/BaseTest.java.in
@@ +636,5 @@
> +    /**
> +     * The method takes as parameter either "bookmark" or "history"
> +     * @return - it returns an ArrayList of the urls in the Firefox for Android Bookmarks or History databases
> +     */
> +    public ArrayList<String> getFirefoxData(String data) {

My comments from bug 896566 also apply here.

In general, it's best to just introduce code in one patch, then set a bug dependency and build another patch on top of that.

Or, if you're really just making some utility-type changes that are to be used in multiple different bugs, you could file a new bug for that and finish that first.

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +70,5 @@
>                  tabEventExpecter.unregisterListener();
>                  contentEventExpecter.unregisterListener();
>  
>                  // See tab count
> +                Element tabCount = mDriver.findElement(getActivity(), "tabs_counter");

Was this just broken before? That doesn't inspire confidence :/

@@ +82,5 @@
>                  if (opt.equals("Open Link in Private Tab")) {
>                      accessSection(2, opt, urls);
>                      mAsserter.ok(mSolo.waitForText("New private tab opened"), "Waiting for private tab to open", "The private tab is opened");
>  
>                      // Verifying if the private data is not listed in Awesomescreen

While you're here, please remove any references to the "Awesomescreen", since it doesn't exist anymore.

@@ +86,5 @@
>                      // Verifying if the private data is not listed in Awesomescreen
>                      focusUrlBar();
> +                    int width = mDriver.getGeckoWidth() / 2;
> +                    int height = mDriver.getGeckoHeight() / 2;
> +                    mActions.drag(0, width, height, height);

Add a comment explaining that you're dragging to the history page. This, once again, seems like something that should be in a helper method.

@@ +116,5 @@
>                                  accessSection(0, opt, urls);
>                                  mAsserter.is(mSolo.waitForText("Bookmark added"), true, "Bookmark added verified");
>  
> +                                String bookmarkedLink = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> +                                mAsserter.ok(isBookmark(bookmarkedLink), "Checking to see if the link was bookmarked","The link was bookmarked");

Nice, I like how this removed a dependency on the UI for checking the bookmark state.

@@ +183,5 @@
>                          mAsserter.ok(mSolo.waitForText(opt), "Waiting for  " + opt + "  option", "The " + opt + "  option is present");
>                      }
>                      else {
>                          if (opt.equals("Save Image")) {
> +                            // Checking if the option exits. We do not download it since at one point all the saved files may take up unwanted space

This comment says we don't download the image, but the logic down below makes it sound like we do. Is that correct?

@@ +202,5 @@
> +        Actions.EventExpecter clearData = mActions.expectGeckoEvent("Sanitize:Finished");
> +        mSolo.clickOnText("Clear data");
> +        clearData.blockForEvent();
> +        clearData.unregisterListener();        
> +    }

I don't like seeing this logic duplicated between here and your patch in bug 896566. Perhaps this would also be a good candidate for a utility class (or at least moved into BaseTest for now).

Comment 4

[Adrian Tamas (:AdrianT)]

Attachment: testWebContentContextMenu fix v2

Cleaned up the duplicate code and added it as a separate patch on Bug 901432. Cleaned up all the neats.

The tab_counter issue is there because at one point the id was changed but since this test was disabled on m-c for time now it never created any issues. This was disabled because of Bug 854043 and Bug 862493.

Comment 5

[:Margaret Leibovic]

Comment on attachment 785734 [details] [diff] [review]
testWebContentContextMenu fix v2

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

Thanks for updating the patch! It looks like bug 901432 is already making things nicer.

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +87,4 @@
>                      focusUrlBar();
> +                    int width = mDriver.getGeckoWidth() / 2;
> +                    int height = mDriver.getGeckoHeight() / 2;
> +                    mActions.drag(0, width, height, height);

I wonder if this kind of movement logic would be better in a method in AboutHomeTest, since it seems like something other test would want to do. That could be done in a separate bug, or here if you want.

@@ +93,1 @@
>                      mAsserter.ok(mSolo.waitForText("Browser Blank Page 01"), "Looking in History for the page loaded in the normal tab", "Fount it");

Just noticed a typo here you could fix while you're making changes in here - "Found it".

@@ +113,5 @@
>                              mSolo.waitForText("Big Link");
>                          }
>                          else {
>                              if (opt.equals("Bookmark Link")) {
> +                                

Nit: whitespace.

Comment 6

[Adrian Tamas (:AdrianT)]

Attachment: testWebContentContextMenu fix v3

Addressed the neats and also changed the move to the history tab to use the new openAboutHomeTab method. I had to add a delay since it seems that sometimes the page was not painted when the long click was done which resulted in the test failing now that we use loadUrl and not loadAndPaint

Comment 7

[:Margaret Leibovic]

Comment on attachment 786243 [details] [diff] [review]
testWebContentContextMenu fix v3

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

Looking good, but I just have two comments to address before landing.

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +35,4 @@
>      }
>  
>      public void openContextMenu(int i, String urls []) {
> +        String urls_titles [] = { "Big Link", "Big Mailto", "Picture Link"};

Let's declare this up above in testWebContentContextMenu below where we declare the urls array, then pass it into this method as another parameter. I also think we should name it urlTitles to be consistent with the other array variable names.

@@ +53,5 @@
>      }
>  
>      public void accessSection(int i, String opt, String urls []) {
>          openContextMenu(i, urls);
> +        mAsserter.ok(mSolo.waitForText(opt), "Waiting for  " + opt + "  option", "The " + opt + " option is present");

I see you're just turning two spaces into one space in the third parameter here. You could do that in the second parameter as well.

@@ +117,4 @@
>                                  accessSection(0, opt, urls);
>                                  mAsserter.is(mSolo.waitForText("Bookmark added"), true, "Bookmark added verified");
> +                                String bookmarkedLink = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> +                                mAsserter.ok(isBookmark(bookmarkedLink), "Checking to see if the link was bookmarked","The link was bookmarked");

Nice, this is a way better way to check if a page is bookmarked.

@@ +183,5 @@
>                          mAsserter.ok(mSolo.waitForText(opt), "Waiting for  " + opt + "  option", "The " + opt + "  option is present");
>                      }
>                      else {
>                          if (opt.equals("Save Image")) {
> +                            mAsserter.ok(mSolo.searchText(opt), "Verify that the download option exits", "The Save Image option exits");

s/download/Save Image

Comment 8

[Adrian Tamas (:AdrianT)]

Attachment: testWebContentContextMenu fix v4

Changed the declaration of the arrays to constants so to remove the need to send the params in consecutive calls of the methods. If I would have declared urlTitles as suggested and sent it as a parameter I would have had to add it as a parameter to every method used. This seems simpler and more strait forward.

Comment 9

[:Margaret Leibovic]

Comment on attachment 789597 [details] [diff] [review]
testWebContentContextMenu fix v4

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

::: mobile/android/base/tests/testWebContentContextMenu.java.in
@@ +13,5 @@
> +    private final String URLS [] = {"/robocop/robocop_big_link.html", "/robocop/robocop_big_mailto.html", "/robocop/robocop_picture_link.html"};
> +    private final String LINK_MENU_ITEMS [] = {"Open Link in New Tab", "Open Link in Private Tab", "Copy Link", "Share Link", "Bookmark Link"};
> +    private final String MAILTO_MENU_ITEMS [] = {"Open With an App", "Copy Email Address", "Share Email Address"};
> +    private final String PHOTO_MENU_ITEMS [] = {"Copy Image Location", "Share Image", "Set Image As", "Save Image"};
> +    private final String URL_TITLES [] = {"Big Link", "Big Mailto", "Picture Link"};

Move this up to be declared right below URLS, so that it's more obvious that these urls and titles are supposed to match. Maybe even add a comment explaining that.

@@ +45,4 @@
>          getInstrumentation().waitForIdleSync();
> +        loadUrl(url);
> +        // Wait for a complete load since we can't use loadAndPaint() and DOMContentLoaded returns to fast from loadUrl()
> +        waitForText(URL_TITLES [i]);

In another patch I was reviewing today I saw lucasr use BaseTest's verifyPageTitle method. Maybe that would be better here.

@@ +51,3 @@
>      }
>  
> +    public void accessSection(int i, String opt) {

I must not have looked at this entire test very closely last time because it's confusing that this accessSection method takes an integer as a parameter, which it then passes to openContextMenu, which then is only used to decide which URL to open. I feel like it would make more sense to just have these methods take a url and a title directly. The name accessSection also feels unintuitive (what is a section in this context?).

However, I don't want to scope creep this bug, so let's just file a follow-up bug to make the logic in this test easier to follow.

@@ +116,2 @@
>                                  mAsserter.is(mSolo.waitForText("Bookmark added"), true, "Bookmark added verified");
> +                                String bookmarkedLink = getAbsoluteUrl("/robocop/robocop_blank_01.html");

You should use URLS[0] here, since you're passing in 0 to accessSection above (this is one example of that method being confusing ;).

Comment 10

[Adrian Tamas (:AdrianT)]

Attachment: testWebContentContextMenu fix v5

(In reply to :Margaret Leibovic from comment #9)
> Comment on attachment 789597 [details] [diff] [review]
> testWebContentContextMenu fix v4
> 
> Review of attachment 789597 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +45,4 @@
> >          getInstrumentation().waitForIdleSync();
> > +        loadUrl(url);
> > +        // Wait for a complete load since we can't use loadAndPaint() and DOMContentLoaded returns to fast from loadUrl()
> > +        waitForText(URL_TITLES [i]);
> 
> In another patch I was reviewing today I saw lucasr use BaseTest's
> verifyPageTitle method. Maybe that would be better here.
verifyPageTitle timeout is just 3000 ms which is way to small to work here unfortunately. 

> @@ +51,3 @@
> >      }
> >  
> > +    public void accessSection(int i, String opt) {
> 
> I must not have looked at this entire test very closely last time because
> it's confusing that this accessSection method takes an integer as a
> parameter, which it then passes to openContextMenu, which then is only used
> to decide which URL to open. I feel like it would make more sense to just
> have these methods take a url and a title directly. The name accessSection
> also feels unintuitive (what is a section in this context?).
> 
> However, I don't want to scope creep this bug, so let's just file a
> follow-up bug to make the logic in this test easier to follow.
Changed accessSection to accessContextMenuOption( String pageLink, Sting option). I hope this helps a bit with making the code more easy to understand. Paul Feher wrote extended this test not me but from what I know he was asked to make the changes and catch all the common code in this general methods.

Since this is such a big and complex test (60 assertions) we might want to look into making 3 different tests here: testLinkContextMenu, testMailToContextMenu and testPictureLinkContextMenu. In the future we could also add a 4th html5 context menu test.

> @@ +116,2 @@
> >                                  mAsserter.is(mSolo.waitForText("Bookmark added"), true, "Bookmark added verified");
> > +                                String bookmarkedLink = getAbsoluteUrl("/robocop/robocop_blank_01.html");
> 
> You should use URLS[0] here, since you're passing in 0 to accessSection
> above (this is one example of that method being confusing ;).

Here /robocop/robocop_blank_01.html is actually the page opened from the link not the URL sent to the context menu. I decided to declare the the url locally in verfyLinkContextMenu since it's used in both the bookmark link test and in the copy link test

Comment 11

[:Margaret Leibovic]

Comment on attachment 790048 [details] [diff] [review]
testWebContentContextMenu fix v5

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

Let's land this for now, but I think a follow-up to break this test up sounds like a good idea.

Comment 12

[:Margaret Leibovic]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=cfb36e074557

If this is green I can push this to fig.

Comment 13

[:Margaret Leibovic]

Try push was green.

https://hg.mozilla.org/projects/fig/rev/59e82ad7b744

Comment 14

[Lucas Rocha (:lucasr)]

Backed out, test still failing: http://hg.mozilla.org/projects/fig/rev/cb129e23e514

Comment 15

[Adrian Tamas (:AdrianT)]

Attachment: testWebContentContextMenu fix v6

I removed the download test. I think it's the simplest solution to remove the fail which is most likely caused by the app being closed to early since there is no screenshot taken for the fail. Until we split this up into multiple more simple tests as proposed in Comment 10 I think removing the download part, which proved to be problematic also in the past - bug 854043, is the simplest solution.

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

Comment 16

[Adrian Tamas (:AdrianT)]

Unfortunately I am still seeing fails in the tryrun. A full rewrite and split may be our best solution here to make the tests reliable.

Comment 17

[Adrian Tamas (:AdrianT)]

Attachment: webcontext.patch

I split the test in 2 separate, more easy to understand and follow tests. It works locally but I am still waiting for the tryserver results for the run.

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

Comment 18

[Adrian Tamas (:AdrianT)]

Comment on attachment 794023 [details] [diff] [review]
webcontext.patch

The split in 3 different tests seems to work. There is one fail on the armv6 build but I am seeing issues with the Clipboard:
08-23 03:13:50.390 I/dalvikvm( 3442): Could not find method android.content.ClipData.newPlainText, referenced from method org.mozilla.gecko.util.Clipboard$2.run
08-23 03:13:50.390 W/dalvikvm( 3442): VFY: unable to resolve static method 189: Landroid/content/ClipData;.newPlainText (Ljava/lang/CharSequence;Ljava/lang/CharSequence;)Landroid/content/ClipData;
08-23 03:13:50.401 W/InputManagerService( 1020): Window already focused, ignoring focus gain of: com.android.internal.view.IInputMethodClient$Stub$Proxy@484ee7c8
08-23 03:13:51.030 E/Profiler( 3442): BPUnw: [8 total] thread_register_for_profiling(me=0x3db458, stacktop=0x5ceffe27)
08-23 03:13:51.040 E/Profiler( 3442): BPUnw: [9 total] thread_register_for_profiling(me=0x3db498, stacktop=0x5d2ffe27)
08-23 03:13:51.170 E/Profiler( 3442): BPUnw: [10 total] thread_register_for_profiling(me=0x3db418, stacktop=0x5cdffd6b)
08-23 03:13:51.190 E/Profiler( 3442): BPUnw: [11 total] thread_register_for_profiling(me=0x3db518, stacktop=0x5d8ffd6b)
08-23 03:13:51.190 E/Profiler( 3442): BPUnw: [12 total] thread_register_for_profiling(me=0x3db4d8, stacktop=0x5d6ffd6b)
08-23 03:13:51.530 D/GeckoFavicons( 3442): Could not get URI for favicon
08-23 03:13:53.061 D/GeckoFavicons( 3442): Could not get URI for favicon
08-23 03:13:55.570 E/Profiler( 3442): BPUnw: [11 total] thread_unregister_for_profiling(me=0x3db418) 
08-23 03:13:55.581 E/Profiler( 3442): BPUnw: [10 total] thread_unregister_for_profiling(me=0x3db4d8) 
08-23 03:13:55.590 E/Profiler( 3442): BPUnw: [9 total] thread_unregister_for_profiling(me=0x3db518) 
08-23 03:14:11.440 E/GeckoConsole( 3442): [JavaScript Warning: "WARN addons.updates: Request failed: http://10.250.49.165:30173/extensions-dummy/updateBackgroundURL - 404: Not Found" {file: "resource://gre/modules/AddonUpdateChecker.jsm" line: 469}]
08-23 03:14:11.450 E/GeckoConsole( 3442): [JavaScript Warning: "WARN addons.updates: Request failed: http://10.250.49.165:30173/extensions-dummy/updateBackgroundURL - 404: Not Found" {file: "resource://gre/modules/AddonUpdateChecker.jsm" line: 469}]
08-23 03:14:13.640 I/Robocop ( 3442): waitForText timeout on Paste
08-23 03:14:13.640 I/Robocop ( 3442): 14 INFO TEST-UNEXPECTED-FAIL | testLinkContextMenu | Checking that the Paste option exists - The option exits
08-23 03:14:13.661 I/Robocop ( 3442): Exception caught during test!

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

Comment 19

[:Margaret Leibovic]

Comment on attachment 794023 [details] [diff] [review]
webcontext.patch

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

I really like where this is going! I mainly just have some comments about trying to reduce some copy/paste.

::: mobile/android/base/tests/BaseTest.java.in
@@ +545,5 @@
> +    public void openWebContentContextMenu() {
> +        DisplayMetrics dm = new DisplayMetrics();
> +        getActivity().getWindowManager().getDefaultDisplay().getMetrics(dm);
> +
> +        // The link has a 60px height, so let's try to hit the middle

This method makes an assumption about the page that's currently loaded, so I think it would be better to include loading that page as part of this method, or at the very least make a comment about when it's safe to use this method.

@@ +551,5 @@
> +        float left = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() / 2;
> +
> +        mAsserter.dumpLog("long-clicking at "+left+", "+top);
> +        mActions.clickLongOnScreen(left, top);
> +        waitForText("Share");

This is also an assumption that there will always be a "Share" item. If this method is only intended to long tap on links, I think it should be renamed openLinkContextMenu.

@@ +552,5 @@
> +
> +        mAsserter.dumpLog("long-clicking at "+left+", "+top);
> +        mActions.clickLongOnScreen(left, top);
> +        waitForText("Share");
> +    }

To avoid putting even more things in BaseTest, should we consider making a ContextMenuTest subclass, or something like that?

::: mobile/android/base/tests/testLinkContextMenu.java.in
@@ +27,5 @@
> +        // Test that the menu items are displayed
> +        openWebContentContextMenu();
> +        for (String option:linkMenuItems) {
> +            mAsserter.ok(mSolo.searchText(option), "Checking that the option: " + option + " is available", "The option is available");
> +        }

This same type of check appears in each of these tests. Perhaps you could pass this array as a parameter to openWebContentContextMenu (or openLinkContextMenu if you rename it), and then that method can do the check. You could then also use one of the items in this array for the waitForText check, instead of just hard-coding "Share". Or you could pass a string to check for to openWebContentContextMenu, and then make a separate method for verifying that all the expected items are shown.

@@ +34,5 @@
> +        Actions.EventExpecter tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
> +        mSolo.clickOnText(linkMenuItems[0]);
> +        tabEventExpecter.blockForEvent();
> +        tabEventExpecter.unregisterListener();
> +        verifyTabCount(2); // Verify tab count has increased

This is also re-used in other tests (as well as some of the other menuitem checks), so they can also be pulled out into helper methods.

::: mobile/android/base/tests/testPictureLinkContextMenu.java.in
@@ +53,5 @@
> +        Actions.EventExpecter tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
> +        mSolo.clickOnText(photoMenuItems[4]);
> +        tabEventExpecter.blockForEvent();
> +        tabEventExpecter.unregisterListener();
> +        verifyTabCount(3); // Verify tab count has increased

This depends on all these tests being run in succession, which isn't very robust. At the end of each test, we should make sure to close any new tabs we've opened, or just close them after opening them and doing a check to make sure they exist.

Comment 20

[Adrian Tamas (:AdrianT)]

(In reply to :Margaret Leibovic from comment #19)
> Comment on attachment 794023 [details] [diff] [review]
> webcontext.patch
> 
> Review of attachment 794023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I really like where this is going! I mainly just have some comments about
> trying to reduce some copy/paste.
> 
> ::: mobile/android/base/tests/BaseTest.java.in
> @@ +545,5 @@
> > +    public void openWebContentContextMenu() {
> > +        DisplayMetrics dm = new DisplayMetrics();
> > +        getActivity().getWindowManager().getDefaultDisplay().getMetrics(dm);
> > +
> > +        // The link has a 60px height, so let's try to hit the middle
> 
> This method makes an assumption about the page that's currently loaded, so I
> think it would be better to include loading that page as part of this
> method, or at the very least make a comment about when it's safe to use this
> method.
I will add a comment here. I know there were a lot of issues with loading pages. After a few page loads it would start to fail. I don't know in what amount that is fixed now but I would like not to go back to loading a lot of pages or loading the same page over and over again
> @@ +551,5 @@
> > +        float left = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() / 2;
> > +
> > +        mAsserter.dumpLog("long-clicking at "+left+", "+top);
> > +        mActions.clickLongOnScreen(left, top);
> > +        waitForText("Share");
> 
> This is also an assumption that there will always be a "Share" item. If this
> method is only intended to long tap on links, I think it should be renamed
> openLinkContextMenu.
There is a "Share" option in the context menu for links, pictures, picture links, mailto links and html5 videos. I think we can assume there will be a "Share" option in any web content context menus we test and be safe with this or add a different string if it will be needed at some point. 
> @@ +552,5 @@
> > +
> > +        mAsserter.dumpLog("long-clicking at "+left+", "+top);
> > +        mActions.clickLongOnScreen(left, top);
> > +        waitForText("Share");
> > +    }
> 
> To avoid putting even more things in BaseTest, should we consider making a
> ContextMenuTest subclass, or something like that?
Maybe we should consult with Geoff or Joel on this but everything that uses robotium or the Robocop api (FennecNativeDriver, FennecNativeElement) needs to be used in a class that extends ActivityInstrumentationTestCase2<Activity>. I would not create another subclass to extend BaseTest because we will have problems accessing methods form PixelTest or AboutHomeTest. The only way I can think of making these subclasses work would be to extend AboutHomeTest which already extends BaseTest in order to access all methods from both BaseTest and AboutHomeTest.

> ::: mobile/android/base/tests/testPictureLinkContextMenu.java.in
> @@ +53,5 @@
> > +        Actions.EventExpecter tabEventExpecter = mActions.expectGeckoEvent("Tab:Added");
> > +        mSolo.clickOnText(photoMenuItems[4]);
> > +        tabEventExpecter.blockForEvent();
> > +        tabEventExpecter.unregisterListener();
> > +        verifyTabCount(3); // Verify tab count has increased
> 
> This depends on all these tests being run in succession, which isn't very
> robust. At the end of each test, we should make sure to close any new tabs
> we've opened, or just close them after opening them and doing a check to
> make sure they exist.
The count here is 3 not because of the other tests but because we have the test page loaded in the first tab, the picture link in a second where we verify the link is copied correctly and the 3rd with the newly opened tab using "Open in New Tab". Each test starts a new instance of Firefox when it begins and closes it when it ends. There is no need to close the tabs at the end and the check is specific for only this test.

Comment 21

[:Margaret Leibovic]

Comment on attachment 794023 [details] [diff] [review]
webcontext.patch

Thanks for responding to my comments. After thinking about this more, we can just go ahead and land this, since it's clearly an improvement over what we have right now, and we can think about how to make these tests even better (with more generic helper methods) in future bugs.

Comment 22

[Adrian Tamas (:AdrianT)]

Attachment: web context

Made a WebContentContextTest class to host all the common methods but I still have to investigate a bit the copy test because it seems that the test sometimes pastes the content of the last test. Probably a timing issue I need to investigate further

tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=276245402f2e

Comment 23

[Adrian Tamas (:AdrianT)]

Attachment: web context

Covered all the changes to limit duplicate code. Also changed the copy test to check for the text in the Clipboard in order to create a waitForTest for cases where it takes a few seconds for the text to be saved and fix the fails in the last tryrun.

This works for me locally but here is a new tryrun for the changes: https://tbpl.mozilla.org/?tree=Try&rev=f0e4a3c81eef

Comment 24

[:Margaret Leibovic]

Comment on attachment 797211 [details] [diff] [review]
web context

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

Looking good, I just have a few comments and questions.

::: mobile/android/base/tests/WebContentTest.java.in
@@ +9,5 @@
> +
> +/**
> + * This class covers interactions with the context menu opened from web content
> + */
> +abstract class WebContentTest extends AboutHomeTest {

Nit: I think a better name for this would be ContentContextMenuTest, to be more explicit about the fact that this is about context menus.

It's also a little odd that we need to extend AboutHomeTest to create a test that's only about web content, but I understand that's just a result of where our helper methods are located right now, so let's not worry about that until some of the new re-organization work starts landing.

@@ +23,5 @@
> +        float left = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() / 2;
> +
> +        mAsserter.dumpLog("long-clicking at "+left+", "+top);
> +        mActions.clickLongOnScreen(left, top);
> +        waitForText("Share");

This assumes that there's always a "Share" item in whatever context menu is opened through this test. I would prefer if we at least pass this string as a parameter, so that this won't fail if we ever make a new test for a context menu that doesn't have a "Share" item.

@@ +26,5 @@
> +        mActions.clickLongOnScreen(left, top);
> +        waitForText("Share");
> +    }
> +
> +    protected void verifyConextMenuItems(String[] items) {

Typo: ... verifyContextMenuItems( ...

@@ +85,5 @@
> +    }
> +
> +    protected void verifyBookmarkLinkOption(String link) {
> +        openWebContentContextMenu();
> +        mSolo.clickOnText("Bookmark Link");

It would also be nice to pass this string in as a paramter to be consistent with the other helper methods you have in here.

::: mobile/android/base/tests/testLinkContextMenu.java.in
@@ +24,5 @@
> +        inputAndLoadUrl(LINK_PAGE_URL);
> +        waitForText(LINK_PAGE_TITLE);
> +
> +        verifyConextMenuItems(linkMenuItems); // Verify context menu items are correct
> +        openTabFromContextMenu(linkMenuItems[0],2); // Test the "Open in New Tab" option

I agree it's handy to pass the array to verifyContextMenuItems, but for these individual label checks it would be easier to read if we just passed in the strings directly. However, duplicating the strings also duplicates the number of places we need to update strings, so this is okay for now, but let's file a follow-up to use your new StringHelper class to factor out these strings into constants we can use.

@@ +29,5 @@
> +        openTabFromContextMenu(linkMenuItems[1],2); // Test the "Open in Private Tab" option
> +        verifyCopyOption(linkMenuItems[2], BLANK_PAGE_URL); // Test the "Copy Link" option
> +
> +        // Open a new tab to continue the test
> +        addTab(LINK_PAGE_URL);

Why do we need to add a new tab here? (Same question applies to the other tests as well)

::: mobile/android/base/tests/testPictureLinkContextMenu.java.in
@@ +29,5 @@
> +
> +        // Open a new tab to continue the test
> +        addTab(PICTURE_PAGE_URL);
> +        verifyShareOption(photoMenuItems[1], PICTURE_PAGE_TITLE); // Test the "Share Image" option
> +        openTabFromContextMenu(photoMenuItems[4],3); // Test the "Open in New Tab" option

Can you add a comment about why we expect to have 3 tabs open here? It would help future people who come along and are confused like I was :)

Comment 25

[Adrian Tamas (:AdrianT)]

(In reply to :Margaret Leibovic from comment #24)
> Comment on attachment 797211 [details] [diff] [review]
> web context
> 
> Review of attachment 797211 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good, I just have a few comments and questions.
> 
...
> @@ +23,5 @@
> > +        float left = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() / 2;
> > +
> > +        mAsserter.dumpLog("long-clicking at "+left+", "+top);
> > +        mActions.clickLongOnScreen(left, top);
> > +        waitForText("Share");
> 
> This assumes that there's always a "Share" item in whatever context menu is
> opened through this test. I would prefer if we at least pass this string as
> a parameter, so that this won't fail if we ever make a new test for a
> context menu that doesn't have a "Share" item.

As explained in Comment 20 there will always be a Share item in any context menu. There is no point in waiting for anything else here.

> ::: mobile/android/base/tests/testLinkContextMenu.java.in
> @@ +24,5 @@
> > +        inputAndLoadUrl(LINK_PAGE_URL);
> > +        waitForText(LINK_PAGE_TITLE);
> > +
> > +        verifyConextMenuItems(linkMenuItems); // Verify context menu items are correct
> > +        openTabFromContextMenu(linkMenuItems[0],2); // Test the "Open in New Tab" option
> 
> I agree it's handy to pass the array to verifyContextMenuItems, but for
> these individual label checks it would be easier to read if we just passed
> in the strings directly. However, duplicating the strings also duplicates
> the number of places we need to update strings, so this is okay for now, but
> let's file a follow-up to use your new StringHelper class to factor out
> these strings into constants we can use.

I'll look into this as soon as the StringHelper patch lands

> @@ +29,5 @@
> > +        openTabFromContextMenu(linkMenuItems[1],2); // Test the "Open in Private Tab" option
> > +        verifyCopyOption(linkMenuItems[2], BLANK_PAGE_URL); // Test the "Copy Link" option
> > +
> > +        // Open a new tab to continue the test
> > +        addTab(LINK_PAGE_URL);
> 
> Why do we need to add a new tab here? (Same question applies to the other
> tests as well)

I am adding a new tab because at this point we are in about:home in edit mode with a text pasted in the url bar. Loading the page in a new tab is more reliable here then trying to go back to the original page

Comment 26

[:Margaret Leibovic]

(In reply to Adrian Tamas (:AdrianT) from comment #25)
> (In reply to :Margaret Leibovic from comment #24)
> > Comment on attachment 797211 [details] [diff] [review]
> > web context
> > 
> > Review of attachment 797211 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looking good, I just have a few comments and questions.
> > 
> ...
> > @@ +23,5 @@
> > > +        float left = mDriver.getGeckoLeft() + mDriver.getGeckoWidth() / 2;
> > > +
> > > +        mAsserter.dumpLog("long-clicking at "+left+", "+top);
> > > +        mActions.clickLongOnScreen(left, top);
> > > +        waitForText("Share");
> > 
> > This assumes that there's always a "Share" item in whatever context menu is
> > opened through this test. I would prefer if we at least pass this string as
> > a parameter, so that this won't fail if we ever make a new test for a
> > context menu that doesn't have a "Share" item.
> 
> As explained in Comment 20 there will always be a Share item in any context
> menu. There is no point in waiting for anything else here.

This is an assumption about our current context menus. Maybe I'm being too pedantic here, but I'm just trying to avoid running into unexpected test failures in the case that we ever change our context menus. However, we can just leave it for now if you think it's too annoying to change.

> > ::: mobile/android/base/tests/testLinkContextMenu.java.in
> > @@ +24,5 @@
> > > +        inputAndLoadUrl(LINK_PAGE_URL);
> > > +        waitForText(LINK_PAGE_TITLE);
> > > +
> > > +        verifyConextMenuItems(linkMenuItems); // Verify context menu items are correct
> > > +        openTabFromContextMenu(linkMenuItems[0],2); // Test the "Open in New Tab" option
> > 
> > I agree it's handy to pass the array to verifyContextMenuItems, but for
> > these individual label checks it would be easier to read if we just passed
> > in the strings directly. However, duplicating the strings also duplicates
> > the number of places we need to update strings, so this is okay for now, but
> > let's file a follow-up to use your new StringHelper class to factor out
> > these strings into constants we can use.
> 
> I'll look into this as soon as the StringHelper patch lands

Okay, thanks.

> > @@ +29,5 @@
> > > +        openTabFromContextMenu(linkMenuItems[1],2); // Test the "Open in Private Tab" option
> > > +        verifyCopyOption(linkMenuItems[2], BLANK_PAGE_URL); // Test the "Copy Link" option
> > > +
> > > +        // Open a new tab to continue the test
> > > +        addTab(LINK_PAGE_URL);
> > 
> > Why do we need to add a new tab here? (Same question applies to the other
> > tests as well)
> 
> I am adding a new tab because at this point we are in about:home in edit
> mode with a text pasted in the url bar. Loading the page in a new tab is
> more reliable here then trying to go back to the original page

Sounds reasonable. Can you add a comment to the test explaining that?

Comment 27

[Adrian Tamas (:AdrianT)]

Attachment: split testWebContentContextMenu to make it more reliable

Made the requested changes.

Changed the openWebContentContextMenu method to use a wait test and I'm waiting in each method that uses this for the specific option the method tests.

Removed the addTab from the tests since I didn't realize that because of test reliability problems I changed the test of the copy options to check the clipboard test directly.

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

Comment 28

[:Margaret Leibovic]

Comment on attachment 802319 [details] [diff] [review]
split testWebContentContextMenu to make it more reliable

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

Looks good! Thanks for addressing my comments. If the try push looks green, we can go ahead and land this.

Comment 29

[Lucas Rocha (:lucasr)]

Pushed: https://hg.mozilla.org/integration/fx-team/rev/9df00be7eb58

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9df00be7eb58