Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1968: Fix NavigationDrawerTestActivityTest for Robolectric #2204

Conversation

anandwana001
Copy link
Contributor

@anandwana001 anandwana001 commented Dec 1, 2020

Explanation

Fix #1968

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@BenHenning
Copy link
Member

So I've been investigating this. When digging into why click() is failing (e.g. due to the 'Admin' text not being at least 90% visible), I found something interesting. The internal check to Espresso uses something called view.getGlobalVisibleRect() to get the visible rect of the child. That function, internally, relies heavily on its parent to determine if it's visible (e.g. by calling ViewGroup.getChildVisibleRect()). If you follow this up to the DrawerLayout starting at the admin text, all views underneath that layout are visible until you reach that point of the hierarchy. The child in question is NavigationView and it has an mLeft value of -768 (meaning the menu is completely offscreen when Robolectric is trying to interact with it).

Not sure why Robolectric isn't waiting for the drawer to open--trying to figure that part out now.

@BenHenning
Copy link
Member

I have a decent idea why this is happening now, but don't yet have a workaround. Clicking to open the navigation drawer is piping through ActionBarDrawerToggle & into DrawerLayout.openDrawer() as expected. However, it starts going downhill from there: openDrawer() uses a utility class called ViewDragHelper + an override to computeScroll() to scroll the drawer open after being told to open. The initiation of this process is working, but unsurprisingly Robolectric doesn't call into computeScroll() correctly (we've run into this problem in past tests, too). Looking for a way to force the scrolling.

@BenHenning
Copy link
Member

BenHenning commented Dec 3, 2020

Aha, it seems that computeScroll() is only called during the View's drawing step, and Robolectric skips view rendering. It's a bit bad that Android has a rendering process with side effects imo, but that's something that we need to live with for certain animations it seems.

@BenHenning
Copy link
Member

BenHenning commented Dec 3, 2020

Hmm. Apparently just calling computeScroll() opens the drawer enough to fix the test. I'm not sure why it works with just one call (real Android calls it 3-4 times with an emulator & debugger attached)--maybe it has something to do with the fake time. We may need to tweak this if we see flakes in the future, but it's easily isolatable. Will post fixed version of the test suite soon.

@BenHenning
Copy link
Member

Here's my partially working implementation: https://gist.github.com/BenHenning/e0c3c6d729409d74bcb2e213a6557db6/revisions. Some tests are passing, and I think the mechanism being introduced here is correct. Some additional changes that I think will be needed:

  1. I don't think we can use open() for the drawer--we should probably use the mechanism I introduced here (which may need to be generalized to support different screens besides home)
  2. A lot of these tests are performing multi-activity tests--they need to split apart since Robolectric doesn't support actually changing activities

All of the tests are passing on Espresso, and a bit less than half are passing on Robolectric. The other failures seem mainly tied to testing multiple activities, so I'm deferring to you @anandwana001 to finish up those pieces.

@BenHenning
Copy link
Member

(Reassigning since I don't think I have anything left to do here since I found a workaround for the core issue).

@BenHenning BenHenning removed their assignment Dec 8, 2020
@anandwana001 anandwana001 changed the title [DO NOT MERGE] Sample PR to investigate NavigationDrawerTest for Robolectric Fix #1968: Fix NavigationDrawerTestActivityTest for Robolectric Dec 8, 2020
@BenHenning
Copy link
Member

Ack thanks @anandwana001. Will do a review pass later today my time.

Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anandwana001! Overall, this LGTM (and I'm really happy to see all of the disables removed here--these were quite tricky!). Just a few comments before I'll approve.

@BenHenning BenHenning assigned anandwana001 and unassigned BenHenning Dec 9, 2020
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anandwana001! Just a few more comments.

@BenHenning BenHenning removed their assignment Dec 10, 2020
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anandwana001! Besides the comment for filing an issue, just had one other comment. Feel free to merge once both comments are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix NavigationDrawerTestActivityTest for Robolectric
2 participants