Closed Bug 1144879 Opened 9 years ago Closed 8 years ago

Android 4.3 testSessionHistory | Menu is open -

Categories

(Firefox for Android Graveyard :: Testing, defect)

Product:
Firefox for Android Graveyard
Component:
Testing
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox46 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: gbrown, Assigned: Mailkov, Mentored)

References

Details

(Whiteboard: [test disabled on android 4.3])

Attachments

(1 file, 13 obsolete files)

Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella
6.00 KB, patch
Details | Diff | Splinter Review 
On the Android 4.3 emulator, robocop testSessionHistory frequently fails.

http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/gbrown@mozilla.com-e80c0fdd1ef6/try-android-api-9/try_ubuntu64_vm_mobile_test-robocop-4-bm53-tests1-linux64-build10.txt.gz

10:41:50     INFO -  TEST-START | testSessionHistory
10:41:50     INFO -  TEST-PASS | testSessionHistory | Robocop tests need the test device screen to be powered on. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"type":"Gecko:Ready"} - Gecko:Ready should equal Gecko:Ready
10:41:50     INFO -  EventExpecter: no longer listening for Gecko:Ready
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_01.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_01.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for Toolbar to enter editing mode. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for UrlEditText to be input method target. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - http://mochi.test:8888/tests/robocop/robocop_blank_01.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The UrlEditText is the input method target - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | initiatingAction is not null - org.mozilla.gecko.tests.components.ToolbarComponent$2@41a09908 should not equal null
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: stored title, "".
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"type":"DOMTitleChanged","title":"Browser Blank Page 01","tabID":0} - DOMTitleChanged should equal DOMTitleChanged
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"bgColor":"transparent","errorType":"","type":"DOMContentLoaded","metadata":null,"tabID":0} - DOMContentLoaded should equal DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMTitleChanged
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: state changed to title, "mochi.test:8888/tests/robocop/robocop_blank_01.html".
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: was satisfied.
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for Toolbar to exit editing mode. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The url argument is not null - /robocop/robocop_blank_01.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_01.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The Toolbar title is mochi.test:8888/tests/robocop/robocop_blank_01.html - mochi.test:8888/tests/robocop/robocop_blank_01.html should equal mochi.test:8888/tests/robocop/robocop_blank_01.html
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_02.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_02.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for Toolbar to enter editing mode. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for UrlEditText to be input method target. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - http://mochi.test:8888/tests/robocop/robocop_blank_02.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The UrlEditText is the input method target - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | initiatingAction is not null - org.mozilla.gecko.tests.components.ToolbarComponent$2@41987960 should not equal null
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: stored title, "mochi.test:8888/tests/robocop/robocop_blank_01.html".
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"type":"DOMTitleChanged","title":"Browser Blank Page 02","tabID":0} - DOMTitleChanged should equal DOMTitleChanged
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"bgColor":"transparent","errorType":"","type":"DOMContentLoaded","metadata":null,"tabID":0} - DOMContentLoaded should equal DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMTitleChanged
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: state changed to title, "mochi.test:8888/tests/robocop/robocop_blank_02.html".
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: was satisfied.
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for Toolbar to exit editing mode. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The url argument is not null - /robocop/robocop_blank_02.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_02.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The Toolbar title is mochi.test:8888/tests/robocop/robocop_blank_02.html - mochi.test:8888/tests/robocop/robocop_blank_02.html should equal mochi.test:8888/tests/robocop/robocop_blank_02.html
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_03.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_03.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for Toolbar to enter editing mode. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for UrlEditText to be input method target. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - http://mochi.test:8888/tests/robocop/robocop_blank_03.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The UrlEditText is the input method target - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | initiatingAction is not null - org.mozilla.gecko.tests.components.ToolbarComponent$2@41a29140 should not equal null
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: stored title, "mochi.test:8888/tests/robocop/robocop_blank_02.html".
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"type":"DOMTitleChanged","title":"Browser Blank Page 03","tabID":0} - DOMTitleChanged should equal DOMTitleChanged
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"bgColor":"transparent","errorType":"","type":"DOMContentLoaded","metadata":null,"tabID":0} - DOMContentLoaded should equal DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMTitleChanged
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: state changed to title, "mochi.test:8888/tests/robocop/robocop_blank_03.html".
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: was satisfied.
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for Toolbar to exit editing mode. - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The url argument is not null - /robocop/robocop_blank_03.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_03.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The Toolbar title is mochi.test:8888/tests/robocop/robocop_blank_03.html - mochi.test:8888/tests/robocop/robocop_blank_03.html should equal mochi.test:8888/tests/robocop/robocop_blank_03.html
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | initiatingAction is not null - org.mozilla.gecko.tests.helpers.NavigationHelper$1@41c76110 should not equal null
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: stored title, "mochi.test:8888/tests/robocop/robocop_blank_03.html".
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"type":"DOMTitleChanged","title":"Browser Blank Page 02","tabID":0} - DOMTitleChanged should equal DOMTitleChanged
10:41:50     INFO -  EventExpecter: no longer listening for DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMTitleChanged
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: state changed to title, "mochi.test:8888/tests/robocop/robocop_blank_02.html".
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: was satisfied.
10:41:50     INFO -  TEST-PASS | testSessionHistory | The url argument is not null - /robocop/robocop_blank_02.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_02.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The Toolbar title is mochi.test:8888/tests/robocop/robocop_blank_02.html - mochi.test:8888/tests/robocop/robocop_blank_02.html should equal mochi.test:8888/tests/robocop/robocop_blank_02.html
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | initiatingAction is not null - org.mozilla.gecko.tests.helpers.NavigationHelper$1@41a18d00 should not equal null
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: stored title, "mochi.test:8888/tests/robocop/robocop_blank_02.html".
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"type":"DOMTitleChanged","title":"Browser Blank Page 01","tabID":0} - DOMTitleChanged should equal DOMTitleChanged
10:41:50     INFO -  TEST-PASS | testSessionHistory | Given message occurred for registered event: {"bgColor":"transparent","errorType":"","type":"DOMContentLoaded","metadata":null,"tabID":0} - DOMContentLoaded should equal DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMContentLoaded
10:41:50     INFO -  EventExpecter: no longer listening for DOMTitleChanged
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: state changed to title, "mochi.test:8888/tests/robocop/robocop_blank_01.html".
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: was satisfied.
10:41:50     INFO -  TEST-PASS | testSessionHistory | The url argument is not null - /robocop/robocop_blank_01.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | url is not null - /robocop/robocop_blank_01.html should not equal null
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | The Toolbar title is mochi.test:8888/tests/robocop/robocop_blank_01.html - mochi.test:8888/tests/robocop/robocop_blank_01.html should equal mochi.test:8888/tests/robocop/robocop_blank_01.html
10:41:50     INFO -  TEST-PASS | testSessionHistory | The toolbar is not in the editing state - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | initiatingAction is not null - org.mozilla.gecko.tests.helpers.NavigationHelper$2@41c22258 should not equal null
10:41:50     INFO -  ToolbarTitleTextChangeVerifier: stored title, "mochi.test:8888/tests/robocop/robocop_blank_01.html".
10:41:50     INFO -  TEST-PASS | testSessionHistory | Menu is not open - 
10:41:50     INFO -  TEST-PASS | testSessionHistory | Waiting for menu to open. - 
10:41:50  WARNING -  TEST-UNEXPECTED-FAIL | testSessionHistory | Menu is open - 
10:41:50     INFO -  0 ERROR Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testSessionHistory | Menu is open - 
10:41:50     INFO -  	at junit.framework.Assert.fail(Assert.java:50)
10:41:50     INFO -  	at org.mozilla.gecko.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:128)
10:41:50     INFO -  	at org.mozilla.gecko.FennecMochitestAssert.ok(FennecMochitestAssert.java:150)
10:41:50     INFO -  	at org.mozilla.gecko.tests.helpers.AssertionHelper.fAssertTrue(AssertionHelper.java:98)
10:41:50     INFO -  	at org.mozilla.gecko.tests.components.AppMenuComponent.pressMenuItem(AppMenuComponent.java:206)
10:41:50     INFO -  	at org.mozilla.gecko.tests.components.AppMenuComponent.pressMenuItem(AppMenuComponent.java:236)
10:41:50     INFO -  	at org.mozilla.gecko.tests.helpers.NavigationHelper$2.run(NavigationHelper.java:85)
10:41:50     INFO -  	at org.mozilla.gecko.tests.helpers.WaitHelper.waitForPageLoad(WaitHelper.java:95)
10:41:50     INFO -  	at org.mozilla.gecko.tests.helpers.NavigationHelper.goForward(NavigationHelper.java:82)
10:41:50     INFO -  	at org.mozilla.gecko.tests.testSessionHistory.testSessionHistory(testSessionHistory.java:31)
10:41:50     INFO -  	at java.lang.reflect.Method.invokeNative(Native Method)
10:41:50     INFO -  	at java.lang.reflect.Method.invoke(Method.java:525)
10:41:50     INFO -  	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214)
10:41:50     INFO -  	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199)
10:41:50     INFO -  	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192)
10:41:50     INFO -  	at org.mozilla.gecko.tests.UITest.runTest(UITest.java:77)
10:41:50     INFO -  	at junit.framework.TestCase.runBare(TestCase.java:134)
10:41:50     INFO -  	at junit.framework.TestResult$1.protect(TestResult.java:115)
10:41:50     INFO -  	at junit.framework.TestResult.runProtected(TestResult.java:133)
10:41:50     INFO -  	at junit.framework.TestResult.run(TestResult.java:118)
10:41:50     INFO -  	at junit.framework.TestCase.run(TestCase.java:124)
10:41:50     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191)
10:41:50     INFO -  	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176)
10:41:50     INFO -  	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554)
10:41:50     INFO -  	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701)
10:41:50     INFO -  
10:41:50  WARNING -  TEST-UNEXPECTED-FAIL | testSessionHistory | Exception caught - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testSessionHistory | Menu is open - 
10:41:50     INFO -  TEST-OK | testSessionHistory | took 269701ms
10:41:50     INFO -  TEST-START | Shutdown
10:41:50     INFO -  Passed: 72
10:41:50     INFO -  Failed: 2
Whiteboard: [test disabled on android 4.3]
Assignee: nobody → miticomilko
Blocks: 1145821
This is a first patch ... so we can talk on code ...
Flags: needinfo?(michael.l.comella)
Attachment #8641284 - Flags: review?(michael.l.comella)
Comment on attachment 8641284 [details] [diff] [review]
Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella

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

::: mobile/android/tests/browser/robocop/components/AppMenuComponent.java
@@ +307,5 @@
>      private boolean isMenuOpen(String menuItemTitle) {
>          return mSolo.searchText(menuItemTitle, true);
>      }
>  
> +    private boolean isMenuOpen(View menuItemView) {

I don't like this approach because it requires a developer wanting to use this approach to do the heavy lifting of finding a valid menuItemView (and when we call this method, we're not clear that we're using a menu item view). I see why you wanted to use findAppMenuItemView now - if you can verify it doesn't cause weird states as caused on IRC, I think it could be a valid approach.

Sebastian mentioned he extended isMenuOpen with a contentDescription section - did you try that approach at all?
Attachment #8641284 - Flags: review?(michael.l.comella) → feedback+
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #2)

> I don't like this approach because it requires a developer wanting to use
> this approach to do the heavy lifting of finding a valid menuItemView (and
> when we call this method, we're not clear that we're using a menu item
> view). I see why you wanted to use findAppMenuItemView now - if you can
> verify it doesn't cause weird states as caused on IRC, I think it could be a
> valid approach.
> Sebastian mentioned he extended isMenuOpen with a contentDescription section
> - did you try that approach at all?

We talked about it on IRC  2015-07-31 ... approach of sebastian was to create findAppItemView
you can see it on https://bugzilla.mozilla.org/show_bug.cgi?id=944142 

So if I understand I can continue with this approach ... 

>    private boolean isMenuOpen(View menuItemView) {
>        return (menuItemView != null) && (menuItemView.getVisibility() == View.VISIBLE)
>    }

As you said on IRC :  
> "Mailkov: Seems feasible, but we should check whether or not filterViews will return non-visible 
> views, or check that the views are visible before saying returning true from isMenuOpen" 

this function return true only if the view is visible;

and 
> "Mailkov: 2.3 is more concerning though because it just filters on TextViews - you may need to add an > assertion about menu item types in there"

the findAppMenuItemView contains code for this yet as you can see: 

> private View findAppMenuItemView(String text) {
>         mSolo.waitForText(text, 1, MAX_WAITTIME_FOR_MENU_UPDATE_IN_MS);
> 
>        final List<View> views = mSolo.getViews();
> 
>        final List<MenuItemActionBar> menuItemActionBarList = >RobotiumUtils.filterViews(MenuItemActionBar.class, views);
>        for (MenuItemActionBar menuItem : menuItemActionBarList) {
>            if (TextUtils.equals(menuItem.getContentDescription(), text)) {
>                return menuItem;
>            }
>        }
> 
>        final List<MenuItemDefault> menuItemDefaultList = >RobotiumUtils.filterViews(MenuItemDefault.class, views);
>        for (MenuItemDefault menuItem : menuItemDefaultList) {
>            if (TextUtils.equals(menuItem.getText(), text)) {
>                return menuItem;
>            }
>        }
> 
>        // On Android 2.3, menu items may be instances of
>        // com.android.internal.view.menu.ListMenuItemView, each with a child
>        // android.widget.RelativeLayout which in turn has a child
>        // TextView with the appropriate text.
>        final List<TextView> textViewList = RobotiumUtils.filterViews(TextView.class, views);
>        for (TextView textView : textViewList) {
>            if (TextUtils.equals(textView.getText(), text)) {
>                View relativeLayout = (View) textView.getParent();
>                View listMenuItemView = (View)relativeLayout.getParent();
>                return listMenuItemView;
>            }
>        }
>        return null;
>    }

Excuse me, if I did not understand exactly what you said me.
If my phrases seem strange,  can you correct me?
I'm studing to improve my English.

Do you think that the patch is ok? (I tested it and it works)
If it is not ... please can you explain your thinking in detail?
If it is ok ... can you send it on TryServer ... I have not access yet.
Thanks!
Flags: needinfo?(michael.l.comella)
Yes, I would agree to continue using findAppMenuItemView in isMenuOpen. However, regarding type-checking the 2.3 Views, you mention:

> the findAppMenuItemView contains code for this yet as you can see: 

But it actually does not – it checks that the parent views exist but never actually uses `instanceof`, or does the appropriate casting. It's possible there are other TextViews with this number of parents which could be confused. Perhaps this could be fixed as a follow-up.

(if I understand correctly) Regarding whether the current patch is okay, it doesn't use findAppMenuItemView, and still forces the developer to find a View to determine if the menu is open. You should change the existing methods to use this new method to avoid developer confusion (which one should I use?) and ensure consistency of implementation (e.g. if there are bugs in one but not the other, it's hard to keep track). Therefore, I'd expect the API to be:

public boolean isMenuOpen() {
  final String initialMenuItemTitle = ...;
  return isMenuOpen(initialMenuItemTitle);
}

public boolean isMenuOpen(final String menuItemTitle) {
  // findAppMenuItemView in here somewhere...
  ...
}
Flags: needinfo?(michael.l.comella)
I hope it is ok now !
Attachment #8641284 - Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)
Attachment #8643646 - Flags: review?(michael.l.comella)

        Attachment #8643646 -
        Attachment is obsolete: true

        Attachment #8643646 -
        Flags: review?(michael.l.comella)

        Attachment #8643648 -
        Flags: review?(michael.l.comella)

    
    

    
  
(BTW, you only need to specify one flag - if you want a review, only a review flag is necessary.)
Flags: needinfo?(michael.l.comella)

    
    

    
  
Comment on attachment 8643648 [details] [diff] [review]
Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella

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

By the way, I made a push to our try test servers (above this post) to see if this patch works on our emulators.

::: mobile/android/tests/browser/robocop/components/AppMenuComponent.java
@@ +184,5 @@
>          final List<TextView> textViewList = RobotiumUtils.filterViews(TextView.class, views);
>          for (TextView textView : textViewList) {
>              if (TextUtils.equals(textView.getText(), text)) {
>                  View relativeLayout = (View) textView.getParent();
> +                if (relativeLayout instanceof RelativeLayout) {

Better, but this still isn't quite as I'd expect - the comment above mentions ListMenuItemView - can you find out if we can verify if the views are of that type? Those types of views should *only* be used in the menu.

@@ +312,5 @@
> +        final View menuItemView = findAppMenuItemView(menuItemTitle);
> +        return isMenuOpen(menuItemView);
> +    }
> +
> +    private boolean isMenuOpen(View menuItemView) {

Is there any reason to keep this separate from isMenuOpen(String)? I doubt anyone would call it directly so I'm fine moving it into isMenuOpen(String)

        Attachment #8643648 -
        Flags: review?(michael.l.comella) → feedback+

    
    

    
  

      
      
      
      

        Attached file
          
        error building (obsolete)
        — 
      

    


  

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #9)
> Comment on attachment 8643648 [details] [diff] [review]
> Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella
> 
> Review of attachment 8643648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> By the way, I made a push to our try test servers (above this post) to see
> if this patch works on our emulators.
> 
> ::: mobile/android/tests/browser/robocop/components/AppMenuComponent.java
> @@ +184,5 @@
> >          final List<TextView> textViewList = RobotiumUtils.filterViews(TextView.class, views);
> >          for (TextView textView : textViewList) {
> >              if (TextUtils.equals(textView.getText(), text)) {
> >                  View relativeLayout = (View) textView.getParent();
> > +                if (relativeLayout instanceof RelativeLayout) {
> 
> Better, but this still isn't quite as I'd expect - the comment above
> mentions ListMenuItemView - can you find out if we can verify if the views
> are of that type? Those types of views should *only* be used in the menu.
> 

  I tried to use :
> import android.support.v7.internal.view.menu.ListMenuItemView;
...
> if (listMenuItemView instanceof ListMenuItemView) {
>     return listMenuItemView;
> }
 
but it returns an error on building. I attached it!

> @@ +312,5 @@
> > +        final View menuItemView = findAppMenuItemView(menuItemTitle);
> > +        return isMenuOpen(menuItemView);
> > +    }
> > +
> > +    private boolean isMenuOpen(View menuItemView) {
> 
> Is there any reason to keep this separate from isMenuOpen(String)? I doubt
> anyone would call it directly so I'm fine moving it into isMenuOpen(String)

The reason is in pressMenuItem function, as you can see it uses findAppMenuItemView
to find the View for the clickOnView ... in pressMenuItem there is isMenuOpen function also, so I don't want launch findAppMenuItemView two times.

> private void pressMenuItem(final String menuItemTitle) {
>         if (!hasLegacyMenu()) {
>             final View menuItemView = findAppMenuItemView(menuItemTitle);
>             fAssertTrue("Menu is open", isMenuOpen(menuItemView));

    
    

    
  
I'm studying to solve the error on building ...

I'm going to holiday for three weeks, so I cannot work for this times ... 
If you can, you let me assigned the bugs, I'll solve them when I'll return

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=506602b825f2

I'm seeing that it fails on api 9 robocop 4 ...
I'm creating a patch for this

    
    

    
  


  
Can you try it on TryServer ?

My Pc has some problems ... so I cannot test on api 9 ... 
but I think that this patch works.

        Attachment #8643648 -
        Attachment is obsolete: true

        Attachment #8644288 -
        Flags: review?(michael.l.comella)

    
    

    
  
(In reply to Melchiorre Alastra (:Mailkov) from comment #11)
>   I tried to use :
> > import android.support.v7.internal.view.menu.ListMenuItemView;

I found the code via Google [1] and it seems it is in the package `com.android.internal.view.menu` so I'm guessing we don't have access to it. We might be able to use reflection, but that's probably overkill.

Can you add a comment mentioning why we don't verify the type?

[1]: https://github.com/android/platform_frameworks_base/blob/master/core/java/com/android/internal/view/menu/ListMenuItemView.java

    
    

    
  
Comment on attachment 8644288 [details] [diff] [review]
Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella

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

The push in comment 15 fails on 2.3 opt builds. Let me know if you need help debugging that. Note that all of our automation builds run on emulators so they can run slightly differently from devices.

f+ because of the try failure.

::: mobile/android/tests/browser/robocop/components/AppMenuComponent.java
@@ +185,5 @@
>          for (TextView textView : textViewList) {
>              if (TextUtils.equals(textView.getText(), text)) {
>                  View relativeLayout = (View) textView.getParent();
> +                if (relativeLayout instanceof RelativeLayout) {
> +                    View listMenuItemView = (View)relativeLayout.getParent();

nit: (View) relat...

@@ +310,5 @@
> +        final View menuItemView = findAppMenuItemView(menuItemTitle);
> +        return isMenuOpen(menuItemView);
> +    }
> +
> +    private boolean isMenuOpen(View menuItemView) {

I find this method to be confusing (e.g. do I just pass in any View?) but I see your point about calling findAppMenuItemView twice. Since it's test code, I'm tempting to say call it twice but the method is private so I'll accept this.

Can you add a comment explaining the parameter?

        Attachment #8644288 -
        Flags: review?(michael.l.comella) → feedback+

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #17)
 
> The push in comment 15 fails on 2.3 opt builds. Let me know if you need help
> debugging that.

I'm on vacation and will return in September, 
I think of knowing why it fails on 2.3.
In this moment I have problem on debugging 2.3, 
but I want solve it when I back.

    
    

    
  
(In reply to Melchiorre Alastra (:Mailkov) from comment #18)
> I'm on vacation and will return in September, 

Sure - enjoy your vacation!
 
> but I want solve it when I back.

As long as this doesn't need to land before you get back, it'll be here when you return! :)

    
    

    
  
Mike this looks like it has stalled. Can you finish it up? We only run this test on 2.3, which seems bad.
Flags: needinfo?(michael.l.comella)

    
    

    
  
Mailkov, if you're interested in finishing this, let me know! In the mean time, I'll assign myself and work on it if I have time.
Assignee: miticomilko → michael.l.comella
Flags: needinfo?(michael.l.comella)

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #21)
> Mailkov, if you're interested in finishing this, let me know! In the mean
> time, I'll assign myself and work on it if I have time.

Michael, I`m interested in finishing this. I had some personal problems, I think that I can solve this bug next week.
Flags: needinfo?(michael.l.comella)

    
    

    
  
Good to hear back from you! :) I've assigned you again.
Assignee: michael.l.comella → miticomilko
Flags: needinfo?(michael.l.comella)

    
    

    
  


  
I'm back ... I tested it and works on 2.3 and 4.3 ... 

Can you try it on TryServer ?

        Attachment #8644279 -
        Attachment is obsolete: true

        Attachment #8644288 -
        Attachment is obsolete: true

        Attachment #8686762 -
        Flags: review?(michael.l.comella)

    
    

    
  
The patch does not cleanly apply – can you rebase it for me please?
Flags: needinfo?(miticomilko)

    
    

    
  
You'll notice that the robocop test sources were actually moved to better support IDEs. The components/ dir is now in:
  mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug1144879.patch (obsolete)
        — 
          — Splinter Review
      

    


  
I hope it's now ok for you ;)

        Attachment #8686762 -
        Attachment is obsolete: true

        Attachment #8686762 -
        Flags: review?(michael.l.comella)
Flags: needinfo?(miticomilko)

        Attachment #8687952 -
        Flags: review?(michael.l.comella)

    
    

    
  
Comment on attachment 8687952 [details] [diff] [review]
bug1144879.patch

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

Since it's (more-or-less) the same patch as before, r+ w/ nits assuming green try.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java
@@ +185,5 @@
>          for (TextView textView : textViewList) {
>              if (TextUtils.equals(textView.getText(), text)) {
>                  View relativeLayout = (View) textView.getParent();
> +                if (relativeLayout instanceof RelativeLayout) {
> +                    View listMenuItemView = (View)relativeLayout.getParent();

nit: (View) relativeLayout

@@ +312,5 @@
> +        final View menuItemView = findAppMenuItemView(menuItemTitle);
> +        return isMenuOpen(menuItemView) ? true : mSolo.searchText(menuItemTitle, true);
> +    }
> +
> +    private boolean isMenuOpen(View menuItemView) {

Can you add a comment explaining this parameter?

        Attachment #8687952 -
        Flags: review?(michael.l.comella) → review+

    
    

    
  


  
I added the comment as you asked.

Can I add the checkin-needed keyword to the bug?

        Attachment #8687952 -
        Attachment is obsolete: true
Flags: needinfo?(michael.l.comella)

        Attachment #8689518 -
        Flags: review+

    
    

    
  
(In reply to Melchiorre Alastra (:Mailkov) from comment #30)
> Can I add the checkin-needed keyword to the bug?

Yep! If you get a r+, you can just make the changes, upload a new patch, and add checkin-needed without getting a re-review.
Flags: needinfo?(michael.l.comella)

    
    

    
  
(In reply to Michael Comella (:mcomella) from comment #32)
> Yep! If you get a r+, you can just make the changes, upload a new patch, and
> add checkin-needed without getting a re-review.

But actually I see some test failures so those will need to be fixed first - see comment 28. Let me know if you'll need help reading the results.

    
    

    
  
I need help to read the results.
Flags: needinfo?(michael.l.comella)

    
    

    
  
(In reply to Melchiorre Alastra (:Mailkov) from comment #34)
> I need help to read the results.

We spoke on IRC and I explained how to read the results. Then:

09:33 <Mailkov> mcomella: I think there is nothing related to my work.
09:42 <mcomella> Mailkov: I think the 2.3 failure in testAppMenuPathways could be related
09:43 <mcomella> Mailkov: This view might be null because we return null from the method you modified: https://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java#136
09:43 <mcomella> Mailkov: Why do you feel this error is unrelated?
09:43 <mcomella> (I haven't dug in deep enough to know it's definitely related)

Melchiorre, what do you think?
Flags: needinfo?(michael.l.comella) → needinfo?(miticomilko)

    
    

    
  


  
Excuse me! I had not seen testAppMenuPathways.
I corrected it and I think this patch corrects the bug 1158005 also.

Can try it on TryServer?

        Attachment #8689519 -
        Attachment is obsolete: true
Flags: needinfo?(miticomilko)

        Attachment #8690384 -
        Flags: review?(michael.l.comella)

    
    

    
  
I'm seeing that testAppMenuPathways does not work for api11+ ...
On my PC it works ... however I can reinsert block on robocop.ini.
What do you think?
Flags: needinfo?(michael.l.comella)

    
    

    
  
I tried adding a sleep() to pressSubMenuItem in https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfb30972f811 and it seemed to fix testAppMenuPathways. That is definitely not a real fix, but I hope it provides a clue about the underlying problem.

    
    

    
  
Mailkov, can you use comment 39 to investigate further?
Flags: needinfo?(michael.l.comella) → needinfo?(miticomilko)

    
    

    
  


  
Can you try it on TryServer?

Thanks

        Attachment #8690384 -
        Attachment is obsolete: true

        Attachment #8690384 -
        Flags: review?(michael.l.comella)
Flags: needinfo?(miticomilko)

        Attachment #8692049 -
        Flags: review?(michael.l.comella)

    
    

    
  
Comment on attachment 8692049 [details] [diff] [review]
Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella

Melchiorre, can you flag me for a review when the push goes green? You can use needinfo to ask me further questions or ask me to make more pushes.

If you'd like to do your own pushes to our try server, I'd be happy to vouch for you though! :) See [1] and bug 1175275 for an example access request bug.

[1]: https://www.mozilla.org/en-US/about/governance/policies/commit/

        Attachment #8692049 -
        Flags: review?(michael.l.comella)

    
    

    
  
I'll happy if you vouch for me:  https://mozillians.org/en-US/u/Mailkov/
Flags: needinfo?(michael.l.comella)

    
    

    
  
(In reply to Melchiorre Alastra (:Mailkov) from comment #44)
> I'll happy if you vouch for me:  https://mozillians.org/en-US/u/Mailkov/

Done. That being said, this profile won't get you try access – you have to follow the process in the link from comment 43.
Flags: needinfo?(michael.l.comella)

    
    

    
  
Comment on attachment 8692049 [details] [diff] [review]
Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella

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

I'm seeing that last push is green.

This patch corrects the bug 1158005 also.

I'm happy!

        Attachment #8692049 -
        Flags: review?(michael.l.comella)

    
    

    
  
I reran the test suite a few times and it appears the test is still intermittent with the new patch – do you want to look into that further?
Flags: needinfo?(miticomilko)

    
    

    
  
Comment on attachment 8692049 [details] [diff] [review]
Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella

Clearing review as it doesn't appear to fully fix the issue.

        Attachment #8692049 -
        Flags: review?(michael.l.comella)

    
    

    
  
fwiw, working in the test framework can be frustrating sometimes due to these intermittent issues. :\ Usually, these could be related to timing (e.g. we're running the tests on Android emulators so they run *slowly*), and screen sizes (e.g. 2.3 devices are not very large). It could be helpful to get try access here – again, I'd vouch for you if you're interested.

Looking at the failing screenshot [1] (which can be found by searching in the output log file for "robocop-screenshot" and copy-pasting the related link), it's in a weird state – I'm actually not sure what's going on here. Do you? Maybe you have more context from looking at more of the code.

If the overhead of working on this bug is too much, I would happy to find you another bug to tackle too! :)

[1]: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/b0635df8f5551fa58850640b5b9dc95c47c8a734dbdbebe2583d97d550942333e4a17f5476d8835f80887b77f30c7bf352c8b9f7d0bf0b73f58c8041144a34bb

    
    

    
  
Michael I would know if intermittent test is testAppMenuPathways on Api 11+ ...
If so I'll change simply the file robocop.ini and I'll reinsert the block for bug 1158005

> [src/org/mozilla/gecko/tests/testAppMenuPathways.java]
> # disabled on 4.3, bug 1158005		
> skip-if = android_version == "18"

The bug on testAppMenuPathways on Api 11+ is not created by this patch!
Flags: needinfo?(miticomilko) → needinfo?(michael.l.comella)

    
    

    
  
But this bug is to fix the issue on API 11+ – 4.3 is the only device that runs these tests. If there's another bug that also is causing a similar intermittent, it would have to be fixed too.

The end result must be this test should not have too many intermittent failures. Does that make sense?
Flags: needinfo?(michael.l.comella)

    
    

    
  
Oh! I misread the title as testAppMenuPathways, not testSessionHistory. Sure, if you block out testAppMenuPathways, I think this will work. Thanks Mailkov! :)

    
    

    
  


  
I blocked out testAppMenuPathways ...
Can you try it?

I opened Bug 1230140 for commit access level 1
Flags: needinfo?(michael.l.comella)

    
  

        Attachment #8692049 -
        Attachment is obsolete: true

    
    

    
  
Mailkov, this fails to apply on the latest fx-team – I fixed it and will push that fix to try, but do you mind rebasing this patch?
Flags: needinfo?(michael.l.comella) → needinfo?(miticomilko)

    
    

    
  


  
I rebased the patch, I think the TryServer is ok ...

next step?

        Attachment #8695374 -
        Attachment is obsolete: true
Flags: needinfo?(miticomilko) → needinfo?(michael.l.comella)

    
    

    
  
Sorry for the delay, Mailkov. The results look good – let me just take a look at the patch.
Flags: needinfo?(michael.l.comella)

    
    

    
  
Mailkov, I'll get to this review on Monday – my apologies for not having it done sooner.

    
    

    
  
Comment on attachment 8695835 [details] [diff] [review]
Bug 1144879 - Android 4.3 testSessionHistory | Menu is open. r=mcomella

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

(In reply to Michael Comella (:mcomella) from comment #59)
> Mailkov, I'll get to this review on Monday – my apologies for not having it
> done sooner.

Sorry. x_x I'll do better.

I retriggered the test suite w/ testSessionHistory x9 – if this error doesn't occur again, we should be good and you can add the checkin-needed keyword. Thanks for your help!

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java
@@ +319,5 @@
> +     *
> +     * @return true if app menu is open.
> +     */
> +    private boolean isMenuOpen(View menuItemView) {
> +        return (menuItemView != null) && (menuItemView.getVisibility() == View.VISIBLE);

I wonder if we shouldn't just switch to using this everywhere – I assume if the menuItemView has visibility VISIBLE, it should be visible after some scrolling.

Why did you choose to keep `Solo.searchText`?

        Attachment #8695835 -
        Flags: review?(michael.l.comella) → review+

    
    

    
  
Can I add the checkin-needed keyword?
Flags: needinfo?(michael.l.comella)

    
    

    
  
(In reply to Melchiorre Alastra (:Mailkov) from comment #61)
> Can I add the checkin-needed keyword?

Yep! Your push looks good. Afterwards, do you mind answering my question from comment 60?

Thanks Mailkov!
Flags: needinfo?(michael.l.comella)

    
  
Keywords: checkin-needed

    
    

    
  
Michael, Apologies ... I choose to keep `Solo.searchText` because it works well on 2.3 ...
At start ,deleting `Solo.searchText`, I had some problems with this patch on 2.3, so a I reused it!

Thanks for your patience!

    
    

    
  
Hi, this failed to apply:

adding 1144879 to series file
renamed 1144879 -> issue1144879.patch
applying issue1144879.patch
patching file mobile/android/tests/browser/robocop/robocop.ini
Hunk #1 FAILED at 132
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/tests/browser/robocop/robocop.ini.rej
patching file mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java
Hunk #1 FAILED at 16
1 out of 3 hunks FAILED -- saving rejects to file mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh issue1144879.patch
Flags: needinfo?(miticomilko)
Keywords: checkin-needed

    
    

    
  


  
I rebase it ... I hope it's now ok

        Attachment #8695835 -
        Attachment is obsolete: true
Flags: needinfo?(miticomilko)

    
    

    
  
It's now ok?
Flags: needinfo?(cbook)

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3b83bc04babe
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox46:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Flags: needinfo?(cbook)

    
    

    
  
Hey Mailkov. If you're looking for a follow-up, perhaps bug 1163963?

    
  
Product: Firefox for Android → Firefox for Android Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: