996227 - Add "Save as PDF" button test

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Discovered we didn't have one in 994989 comment 2.

This would test the UI pathway, not the functionality, to ensure we actually have a consistent "Save as PDF" button location on all devices (rather than accidentally missing them when changing code - see the bug above).

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #0)
lazy link: bug 994989 comment 2.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

gbrown, if we save a PDF, it will be cleaned up by the harness, right?

Implementation notes:
  * The above comment
  * The extension to AppMenuComponent should be generalized so we can enter any two-three-etc. level menu (it's currently in the Page top-level menu); error handling should be added if these options are disabled (or a followup filed)
  * Don't forget we have to navigate to a page in order to enable "Save as PDF"

Comment 3

[Geoff Brown [:gbrown]]

That should be okay. 

We delete /mnt/sdcard/Download between test jobs: http://hg.mozilla.org/build/tools/annotate/27073c214410/sut_tools/cleanup.py#l111

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I spoke with vivek on IRC.

Comment 5

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

Save as pdf UI path tested in this patch.

Comment 6

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

Patch updated with minor corrections.

Patch tested in try and logs can be found here

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

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8420632 [details] [diff] [review]
996227.patch

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

Pretty close! It looks like it should work - I'm just really picky. :P

I think you should also test that "SaveAsPDF" is disabled when on about:home.

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +74,5 @@
>      private void assertMenuIsNotOpen() {
>          fAssertFalse("Menu is not open", isMenuOpen());
>      }
>  
> +    private void assertSubMenuIsOpen(String text) {

nit: It's pretty pedantic (enough so that you don't have to add it), but I really like using `final` everywhere it's applicable (here and elsewhere).

@@ +135,5 @@
>              mSolo.clickOnMenuItem(text, true);
>          }
>      }
>  
> +    public void pressMenuItem(SubMenuItem childMenuItem, MenuItem parentMenuItem) {

A few things:
  * I would prefer the argument order swapped: `parentMenuItem, childMenuItem`. When you're clicking into the menu, this is the order a user (and thus dev writing the test) is thinking of it.
  * SubMenuItem seems too general - why not PageMenuItem? This has the added benefit that you already know what the parentMenuItem is (which can be stored as a constant in the Enum).
  * Have you tried using parts of the existing `pressMenuItem()` (or refactoring it) to do the work here? I'd rather not duplicate this much code.

@@ +190,5 @@
>          // the menu is open or not.
> +        return isMenuOpen(MenuItem.NEW_TAB.getString(mSolo));
> +    }
> +
> +    private boolean isMenuOpen(String text) {

I'm not sure I like this helper method - the callee might think, "How is this used? Can I just pass in any String?" This confusion, tied together with the fact that this helper method is only called once, makes it seem unnecessary. 

Thank you for being proactive and cleaning up the code though (the assertion is a good one!). :)

::: mobile/android/base/tests/robocop.ini
@@ +86,5 @@
>  # disabled on x86 only; bug 957185
>  skip-if = processor == "x86"
>  # [testReaderMode] # see bug 913254, 936224
>  [testReadingListProvider]
> +[testSaveAsPdf]

This should move below the "# Using UITest" comment.

::: mobile/android/base/tests/testSaveAsPdf.java
@@ +1,5 @@
> +package org.mozilla.gecko.tests;
> +
> +import org.mozilla.gecko.tests.helpers.GeckoHelper;
> +import org.mozilla.gecko.tests.helpers.NavigationHelper;
> +import org.mozilla.gecko.tests.components.AppMenuComponent;

nit: Alphabetize.

@@ +6,5 @@
> +
> +/**
> + * Tests the UI pathway to save a page as pdf.
> + */
> +public class testSaveAsPdf extends UITest {

We try to avoid adding too many individual tests as each test we add has a lot of set up time (e.g. delete the profile, install a profile, start the app, wait for gecko to load, etc.) so I think I would like it if this test was more generic - e.g. testAppMenuPathways. You can add a "_testSaveAsPDFPathway" helper method for your code. See TestNativeCrypto [1] for an example.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testNativeCrypto.java

@@ +11,5 @@
> +    public void testSaveAsPdf() {
> +        GeckoHelper.blockForReady();
> +
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> +        mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE);

Add a comment explaining why we're loading the page.

@@ +13,5 @@
> +
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> +        mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE);
> +
> +        mAppMenu.pressMenuItem(AppMenuComponent.SubMenuItem.SAVE_AS_PDF,

nit: Add a comment saying we're just trying to ensure that pressing the button does not throw.

@@ +14,5 @@
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> +        mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE);
> +
> +        mAppMenu.pressMenuItem(AppMenuComponent.SubMenuItem.SAVE_AS_PDF,
> +        		AppMenuComponent.MenuItem.PAGE);

nit: I think this is indented incorrectly. It should either be a double-indent (8 spaces), or aligned with the first argument to the function above - use your judgment on which looks most readable (I tend to prefer the former).

Comment 8

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

Patch updated with review comments

Comment 9

[Vivek Balakrishnan[:vivek]]

Try Server logs :
https://tbpl.mozilla.org/?tree=Try&rev=a470acc4c729

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8425085 [details] [diff] [review]
996227.patch

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

This code may work, but it feels confusing - work on making the method functionality consistent, with well-named parameters and method names.

Let me know if you have questions.

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +30,5 @@
>      public enum MenuItem {
>          FORWARD(R.string.forward),
>          NEW_TAB(R.string.new_tab),
> +        RELOAD(R.string.reload),
> +        PAGE(R.string.page);

nit: Alphabetize.

@@ +53,5 @@
> +        SAVE_AS_PDF(R.string.save_as_pdf);
> +
> +        private final int resourceID;
> +        private static final MenuItem PARENT_MENU = MenuItem.PAGE;
> +        private String stringResource;

nit: Constants separate, at the top.

private static final MenuItem PARENT_MENU = MenuItem.PAGE;

private final int resourceID;
private String stringResource;

@@ +81,5 @@
> +        fAssertTrue("Page Menu is open", isPageMenuOpen());
> +    }
> +
> +    private void assertMenuItemIsEnabledAndVisible(final View menuItemView) {
> +        fAssertTrue("Menu item is enabled", menuItemView.isEnabled());

You should take in a descriptor String here - it'd be hard to debug when all we get out of this is "Menu item is enabled", rather than before where we had "Overflow menu button is enabled".

I'm now on the fence about how necessary this method is with the added complication - e.g. what's the difference between this and having an assertMenuItemIsAAndBAndC? Or A-D, etc. It adds a lot of overhead to finding relevant assertion methods for not much value.

@@ +85,5 @@
> +        fAssertTrue("Menu item is enabled", menuItemView.isEnabled());
> +        fAssertEquals("The menu item is visible", View.VISIBLE, menuItemView.getVisibility());
> +    }
> +
> +    public void assertMenuItemIsVisibleNotEnabled(final MenuItem menuItem) {

The code in this path does not follow the code in its sibling method, assertMenuItemIsEnabledAndVisible, which is unintuitive. Also, why do they have different parameter types? Can they share the same code?

nit: Follow the same naming convention as `assertMenuItemIsEnabledAndVisible` - i.e. assertMenuItemIsDisabledAndVisible.

@@ +93,5 @@
> +
> +        if (menuItemView != null) {
> +        	// Page Menu is available only in non legacy devices.
> +            fAssertFalse("Menu item is not enabled", menuItemView.isEnabled());
> +        	fAssertEquals("The menu item is visible", View.VISIBLE, menuItemView.getVisibility());

nit: indentation.

@@ +138,5 @@
> +     * Will return true when the Android legacy menu is in use.
> +     *
> +     * This method is dependent on not having two views with equivalent contentDescription / text.
> +     */
> +    private boolean pressMenuItem(final MenuItem menuItem, final String text) {

What is text? Perhaps these parameters should have better names, or there should be a javadoc description (with parameter descriptions) of what is going on here - it's hard to follow how the methods calling this work.

@@ +171,5 @@
> +
> +        boolean isLegacyDevice = pressMenuItem(pageMenuItem.PARENT_MENU, childText);
> +
> +        if (isLegacyDevice != true) {
> +        	// Sub Menu Item is not open. Click on the child menu item.

nit: Indentation.

::: mobile/android/base/tests/testAppMenuPathways.java
@@ +4,5 @@
> +import org.mozilla.gecko.tests.helpers.GeckoHelper;
> +import org.mozilla.gecko.tests.helpers.NavigationHelper;
> +
> +/**
> + * Set of tests to test UI App menu paths.

nit: I think you should elaborate on what a "menu path" is.

@@ +12,5 @@
> +    /**
> +     * Robocop supports only a single test function per test class. Therefore, we
> +     * have a single top-level test function that dispatches to sub-tests.
> +     */
> +    public void test() {

nit: testAppMenuPathways - the test name should be the same as the class.

@@ +19,5 @@
> +        _testSaveAsPdfPathway();
> +    }
> +
> +    /**
> +     * Test save as Pdf pathway.

nit: Comment is redundant.

@@ +21,5 @@
> +
> +    /**
> +     * Test save as Pdf pathway.
> +     */
> +    public void _testSaveAsPdfPathway() {

nit: _testSaveAsPDFPathway

@@ +29,5 @@
> +        // Navigate to a page to test save as pdf functionality.
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> +        mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE);
> +
> +        mAppMenu.pressPageMenuItem(AppMenuComponent.PageMenuItem.SAVE_AS_PDF);

Add a comment saying we're not waiting for the results of this - we just want to make sure it doesn't throw an exception.

Comment 11

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

Changes related to review comments

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8443121 [details] [diff] [review]
996227.patch

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

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +81,5 @@
> +    private void assertPageMenuIsOpen() {
> +        fAssertTrue("Page Menu is open", isPageMenuOpen());
> +    }
> +
> +    public void assertParentMenuItemIsDisabledAndVisible(MenuItem parentMenuItem) {

Why "ParentMenu"? Just because we're using it this way now doesn't mean it can't be used with other, non-parent menu views.

If we want to use this only on parent menu items, we should assert that the input is one of the expected values, otherwise change the name. A javadoc comment can help describe what a "ParentMenuItem" is anyway.

@@ +87,5 @@
> +
> +        View parentMenuItemView = findAppMenuItemView(parentMenuItem.getString(mSolo));
> +
> +        if (parentMenuItemView != null) {
> +            // Page Menu is available only in non legacy devices.

Update this comment in accordance with the decision above. Also, if we apply this to non-parent views, make sure you handle the case that parentMenuItemView == null.

@@ +129,5 @@
>      }
>  
> +    /**
> +     * Opens the menu item represented by menuItem in newer devices. In Legacy android device menu item
> +     * having content description menuItemContentDescription is opened. This method is dependent on 

nit: Excess whitespace at the end of this line.

nit: -> In legacy Android devices, the menu item having a content description of menuItemContentDescription is opened.

@@ +134,5 @@
> +     * not having two views with equivalent contentDescription.
> +     *
> +     * @param menuItem The menuItem instance.
> +     * @param menuItemContentDescription The string content description of menuItem.
> +     * @return true for legacy android devices.

We should do a separate check for this - this is an unintuitive return value.

@@ +136,5 @@
> +     * @param menuItem The menuItem instance.
> +     * @param menuItemContentDescription The string content description of menuItem.
> +     * @return true for legacy android devices.
> +     */
> +    private boolean pressMenuItem(final MenuItem menuItem, final String menuItemContentDescription) {

Is this a content description as described by "android:contentDescription"? I think it's actually "android:title" [1] so I would updated the name. 

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/menu/browser_app_menu.xml?rev=480860e8a7f2#19

@@ +164,5 @@
> +        final String text = menuItem.getString(mSolo);
> +        pressMenuItem(menuItem, text);
> +    }
> +
> +    public void pressPageMenuItem(final PageMenuItem pageMenuItem) {

I think pressPageMenuItem should call out to a generic pressSubMenuItem method, if possible.

@@ +169,5 @@
> +        final String pageMenuItemText = pageMenuItem.getString(mSolo);
> +
> +        boolean isLegacyDevice = pressMenuItem(pageMenuItem.PARENT_MENU, pageMenuItemText);
> +
> +        if (isLegacyDevice != true) {

nit: if (!isLegacyDevice)

We should fail an assertion if we're on a legacy device because then this method should be unused.

Comment 13

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

Changes made to include review comments.
Refactored code for generic pressSubMenu.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8447419 [details] [diff] [review]
996227.patch

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

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +84,5 @@
> +        fAssertTrue(String.format("Sub Menu %s is open", childMenuItemTitle), isSubMenuOpen(childMenuItemTitle));
> +    }
> +
> +    /**
> +     * Legacy Android devices doesn't have hierarchical menus. "Page" menu item is missing in these devices.

nit: -> "Legacy Android devices don't have hierarchical menus. Sub-menus, such as "Page", are missing on these devices."

Mention that this method expects the app menu to be open, and assert that it is open in the code.

@@ +87,5 @@
> +    /**
> +     * Legacy Android devices doesn't have hierarchical menus. "Page" menu item is missing in these devices.
> +     * Try to determine if the menu item "Page" is present.
> +     *
> +     * TODO : This fragile way to determine legacy menus must be replaced with a check for 6-panel menu item.

nit: "must" -> "should"

We technically don't have to do this.

@@ +89,5 @@
> +     * Try to determine if the menu item "Page" is present.
> +     *
> +     * TODO : This fragile way to determine legacy menus must be replaced with a check for 6-panel menu item.
> +     *
> +     * @return true, if displayed menu doesn't have "Page" menu item.

nit: true if there is a legacy menu - we don't need to know the implementation details in the javadoc.

@@ +91,5 @@
> +     * TODO : This fragile way to determine legacy menus must be replaced with a check for 6-panel menu item.
> +     *
> +     * @return true, if displayed menu doesn't have "Page" menu item.
> +     */
> +    private boolean checkForLegacyMenu() {

nit: checkForLegacyMenu -> hasLegacyMenu

The convention is to use "is*" or "has*" for boolean-returning functions.

@@ +93,5 @@
> +     * @return true, if displayed menu doesn't have "Page" menu item.
> +     */
> +    private boolean checkForLegacyMenu() {
> +        if (hasLegacyMenu == null) {
> +            fAssertTrue("Menu is not open", isMenuOpen());

nit: "Menu is open" - we say what we *are* asserting.

@@ +94,5 @@
> +     */
> +    private boolean checkForLegacyMenu() {
> +        if (hasLegacyMenu == null) {
> +            fAssertTrue("Menu is not open", isMenuOpen());
> +            hasLegacyMenu = (findAppMenuItemView(MenuItem.PAGE.getString(mSolo)) == null);

nit: Surrounding parens are unnecessary.

@@ +107,5 @@
> +        if (!checkForLegacyMenu()) {
> +            // Non-legacy devices have hierarchical menu, check for parent menu item "page".
> +            View parentMenuItemView = findAppMenuItemView(MenuItem.PAGE.getString(mSolo));
> +            fAssertFalse("The parent 'page' menu item is not enabled", parentMenuItemView.isEnabled());
> +            fAssertEquals("The parent 'page' menu item is visible", View.VISIBLE, parentMenuItemView.getVisibility());

If SAVE_AS_PDF is disabled, the Page menu doesn't have to be disabled. Can you take this into account?

Note that you can take these assertions out of the if statement if you find the appropriate View in the if statements.

@@ +176,4 @@
>          }
>      }
>  
> +    private void pressSubMenuItem(final String parentMenuItemTitle, final String childMenuItemTitle) {

assert the menu is open, or open the menu in this function (I kind of prefer the latter).

@@ +199,5 @@
> +        openAppMenu();
> +        pressMenuItem(menuItem.getString(mSolo));
> +    }
> +
> +    public void pressPageMenuItem(final PageMenuItem pageMenuItem) {

nit: I think I'd prefer it if this was `pressMenuItem(PageMenuItem)`.

Comment 15

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

Review comment incorporated.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8450513 [details] [diff] [review]
996227.patch

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

Sorry for the delay.

The patch's description is incorrect - it's a try selection. Can you update that to the "Bug ####### - <patch description>".

Looking pretty good! Just a few pedantic issues left! Thanks for hanging in there on this patch.

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +95,5 @@
> +     * @return true if there is a legacy menu.
> +     */
> +    private boolean hasLegacyMenu() {
> +        if (hasLegacyMenu == null) {
> +            fAssertTrue("Menu is open", isMenuOpen());

Oh, my mistake - the menu doesn't actually need to be open, does it? You can remove the assertion, the associated comment, and the places where you needed to open the menu before this was called.

By the way, if you think I'm incorrect in my reviews, feel free to say so. The word of the reviewer isn't law.

@@ +110,5 @@
> +            // Non-legacy devices have hierarchical menu, check for parent menu item "page".
> +            final View parentMenuItemView = findAppMenuItemView(MenuItem.PAGE.getString(mSolo));
> +            if (parentMenuItemView.isEnabled()) {
> +                fAssertTrue("The parent 'page' menu item is enabled", parentMenuItemView.isEnabled());
> +                fAssertEquals("The parent 'page' menu item is visible", View.VISIBLE, 

nit: ws.

@@ +199,5 @@
> +        if (!hasLegacyMenu()) {
> +            pressMenuItem(parentMenuItemTitle);
> +
> +            // Verify child menu is open.
> +            assertSubMenuIsOpen(childMenuItemTitle);

I think this is accomplished by childMenuItemView.getVisibility() == View.VISIBLE, and probably unnecessary. Do you agree?

@@ +207,5 @@
> +            fAssertTrue(String.format("The child menu item %s is enabled", childMenuItemTitle),
> +                    childMenuItemView.isEnabled());
> +            fAssertEquals(String.format("The child menu item %s is visible", childMenuItemTitle), View.VISIBLE,
> +                    childMenuItemView.getVisibility());
> +            mSolo.clickOnView(childMenuItemView);

We can use `pressMenuItem(String)` for the child view here, right?

@@ +252,5 @@
>  
> +    private boolean isSubMenuOpen(final String childMenuItemTitle) {
> +        // The presence of the menu item with title "childMenuItemTitle" is our best guess about whether
> +        // the sub menu is open or not.
> +        return mSolo.searchText(childMenuItemTitle);

This is fragile because it could fail if we need to scroll the menu. However, given my other comment, I don't think it's necessary.

::: mobile/android/base/tests/testAppMenuPathways.java
@@ +27,5 @@
> +        NavigationHelper.enterAndLoadUrl(StringHelper.ROBOCOP_BLANK_PAGE_01_URL);
> +        mToolbar.assertTitle(StringHelper.ROBOCOP_BLANK_PAGE_01_TITLE);
> +
> +        // Test save as pdf functionality.
> +        // The following call doesn't wait for result but checks if no exception are thrown.

nit: result -> the resulting pdf

checks if no exception -> checks that no exceptions

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Here's a try run to ensure this runs as well as it looks: https://tbpl.mozilla.org/?tree=Try&rev=8b61575d8d42

Comment 18

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

New patch based on review feedback. Also, corrected the errors during try server run.

The try server run logs for this new patch : https://tbpl.mozilla.org/?tree=Try&rev=406c594dcdb9

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8459238 [details] [diff] [review]
996227.patch

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

Sorry again for the delay - almost there! Thanks for your hard work!

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +198,5 @@
> +            fAssertTrue(String.format("The child menu item %s is enabled", childMenuItemTitle),
> +                    childMenuItemView.isEnabled());
> +            fAssertEquals(String.format("The child menu item %s is visible", childMenuItemTitle), View.VISIBLE,
> +                    childMenuItemView.getVisibility());
> +            pressMenuItem(childMenuItemTitle);

If we use `pressMenuItem(child)` here, we don't need to do all the other code (the two assertions, the `findAppMenuItemView`, and `Solo.clickOnView`), right?

You might have to make isMenuOpen() include submenus too.

@@ +218,4 @@
>      private void openAppMenu() {
>          assertMenuIsNotOpen();
>  
> +        if (HardwareUtils.hasMenuButton() || DeviceHelper.isTablet()) {

Sorry if I was unclear, but using `isTablet` here is a hack rather than a final solution - let me know if you want to know why. You should add a comment saying it's a hack, and file a followup bug to figure out why this is broken on tablet.

Comment 20

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

Feedback comment related changes

Comment 21

[Vivek Balakrishnan[:vivek]]

Try server logs:
https://tbpl.mozilla.org/?tree=Try&rev=3ddd3a3f1381

Comment 22

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8461300 [details] [diff] [review]
996227.patch

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

Looking good! Only nits left! Thanks for your hard work!

::: mobile/android/base/tests/components/AppMenuComponent.java
@@ +212,4 @@
>      private void openAppMenu() {
>          assertMenuIsNotOpen();
>  
> +        // This is the hack needed for tablets where OverflowMenuButton are in GONE state.

nit: You should be more specific about what the hack is, e.g. "This is a hack needed for tablets where the OverflowMenuButton is always in the GONE state, so we press the menu key instead."

@@ +231,5 @@
>          mSolo.clickOnView(overflowMenuButton, true);
>      }
>  
> +    /**
> +    * Determines whether the app menu is open by searching for text "New tab".

nit: -> for the text "New tab".

@@ +240,5 @@
> +        return isMenuOpen(MenuItem.NEW_TAB.getString(mSolo));
> +    }
> +
> +    /**
> +     * Determines whether the app menu is open by searching for text menuItemTitle.

nit: -> for the text in menuItemTitle.

Comment 23

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

nits corrected.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8462532 [details] [diff] [review]
996227.patch

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

Try run in comment 21 should be sufficient.

Thanks, Vivek!

Comment 25

[Carsten Book [:Tomcat]]

seems the patch couldn't apply cleanly:

renamed 996227 -> 996227.patch
applying 996227.patch
patching file mobile/android/base/tests/robocop.ini
Hunk #1 FAILED at 117
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/tests/robocop.ini.rej


could you maybe look into this and rebase it against a current tree? thanks!

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Carsten Book [:Tomcat] from comment #25)
> patching file mobile/android/base/tests/robocop.ini
> Hunk #1 FAILED at 117

Looks like the list of tests got updated underneath you - this should be a pretty easy fix (which unfortunately happens way too often :( ).

Comment 27

[Vivek Balakrishnan[:vivek]]

Attachment: 996227.patch

new patch after rebase

Comment 28

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8463532 [details] [diff] [review]
996227.patch

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

Thanks for the quick turn around.

Comment 29

[Carsten Book [:Tomcat]]

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

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71ef1c18665f

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

re IRC, if you think a `assertNotNull` can give more information, then it can be worth putting in. File a followup if you think it's worthwhile.

Unrelated to this bug, the feedback? flag is on the attachment - click on "details".

needinfo'ing to ensure you see this.