Make settings_egtest.mm work in landscape on small phones.

Make settings_egtest.mm work in landscape on small phones.

Create ChromeEarlGreyUI methods to tap menu buttons, where it will
scroll and make sure the button is visible.

BUG=717614
Review-Url: https://codereview.chromium.org/2867553002
Cr-Commit-Position: refs/heads/master@{#470626}
Committed: https://chromium.googlesource.com/chromium/src/+/959c01304869349d98de2df8775f7209a240dad8

[baxley, 2017-05-08 23:01:12]

baxley@chromium.org changed reviewers:
+ eugenebut@chromium.org

[baxley, 2017-05-08 23:01:13]

you can look at the changes to chrome_earl_grey_ui, and if you don't like it
then there's no need to review settings_egtest.mm

Here is the reason for this change...

We've had a few test failures when tests tap menus, and the menu item is not on
screen and a scroll is needed. I moved some of the simple cases to tap a menu
item into helpers, since it is easy to do it the wrong way in EarlGrey.
If you scroll until content edge, it doesn't look until the end is reached,
which isn't good for something in the middle of the menu.
If scroll displacement is too large, you can skip things.
If scroll displacement is too small, it is slow.
A lot of tests just don't use a search action, since it works most of time.

Basically, tapping a button in a collection view has lots of ways for the user
to do it wrong, so I wanted helpers.
We thought about having methods that take the collection view as a parameter,
but doing it the way I have it allows the test to not have to depend upon the
view controller (or hardcode the accessibility ID).

I ran this locally on an iPhone 5s simulator in landscape and it worked for
settings_egtest.mm. There are other cases that need to be fixed, but I'll do
that separately.

Thanks!

[Eugene But (OOO till 7-30), 2017-05-09 00:15:04]

lgtm

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/settings_egtest.mm (right):

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/settings_egtest.mm:277: id<GREYMatcher>
clearBrowsingDataViewMatcher =
nit: Do you want to create a function for this matcher?

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
File ios/chrome/test/earl_grey/chrome_earl_grey_ui.h (right):

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.h:25: // Scrolls to find the
button in the Settings menu with the corresponding
Is this API used anywhere except settings tests? If no, would it be better to
move this code to test file?

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.h:28: +
(void)tapButtonInSettingsMenu:(id<GREYMatcher>)buttonMatcher;
Per Cocoa Guidelines for Coding, make world(s) before the argument describe the
argument. e.g. tapSettingsMenuButton:

Same comment for other names

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
File ios/chrome/test/earl_grey/chrome_earl_grey_ui.mm (right):

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.mm:67: id<GREYMatcher>
interactableSettingsButton = grey_allOf(
nit: Do you want to make a function for this matcher?

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.mm:85: id<GREYMatcher>
clearBrowsingDataViewMatcher =
ditto

[baxley, 2017-05-09 18:39:19]

Comments addressed, with one question before I move some of the methods. Thanks!

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/settings_egtest.mm (right):

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/settings_egtest.mm:277: id<GREYMatcher>
clearBrowsingDataViewMatcher =
On 2017/05/09 00:15:03, Eugene But wrote:
> nit: Do you want to create a function for this matcher?

Good idea. It's used in a few files so I put it in chrome_matchers and updated
other callsites

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
File ios/chrome/test/earl_grey/chrome_earl_grey_ui.h (right):

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.h:25: // Scrolls to find the
button in the Settings menu with the corresponding
On 2017/05/09 00:15:03, Eugene But wrote:
> Is this API used anywhere except settings tests? If no, would it be better to
> move this code to test file?

Yes, it is used in some other tests. However, the other two aren't...

My biggest concern of moving them is that scrolling menus can have subtle bugs
if done incorrectly, and there are about 5 different ways to do it, where only
one works. We have had multiple test bugs with this. However, I also don't like
cluttering this file with APIs only used in one test class. :)

What do you think if I expose ScrollDown() as ScrollMenuDown() in
chrome_actions, then call-sites could use that as their search action? Then
tapSettingsMenuButton would live here, and I'd move the other two tap APIs into
settings_egtest.mm? Even if we have the helpers here, it won't prevent tests
from doing it themselves. Having a custom scroll action might help?

I wanted to check before moving anything, or to see if you had other ideas.

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.h:28: +
(void)tapButtonInSettingsMenu:(id<GREYMatcher>)buttonMatcher;
On 2017/05/09 00:15:03, Eugene But wrote:
> Per Cocoa Guidelines for Coding, make world(s) before the argument describe
the
> argument. e.g. tapSettingsMenuButton:
> 
> Same comment for other names

Done.

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
File ios/chrome/test/earl_grey/chrome_earl_grey_ui.mm (right):

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.mm:67: id<GREYMatcher>
interactableSettingsButton = grey_allOf(
On 2017/05/09 00:15:04, Eugene But wrote:
> nit: Do you want to make a function for this matcher?

I created a function for SettingsButton, but not the interactable one. When
doing a search action the matcher must check for interactable, while other uses
don't care.

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.mm:85: id<GREYMatcher>
clearBrowsingDataViewMatcher =
On 2017/05/09 00:15:03, Eugene But wrote:
> ditto

Done.

[baxley, 2017-05-09 20:16:39]

All comments resolved. I'll land after the ios-simulator-eg tryjob completes.

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
File ios/chrome/test/earl_grey/chrome_earl_grey_ui.h (right):

https://codereview.chromium.org/2867553002/diff/20001/ios/chrome/test/earl_gr...
ios/chrome/test/earl_grey/chrome_earl_grey_ui.h:25: // Scrolls to find the
button in the Settings menu with the corresponding
On 2017/05/09 18:39:19, baxley wrote:
> On 2017/05/09 00:15:03, Eugene But wrote:
> > Is this API used anywhere except settings tests? If no, would it be better
to
> > move this code to test file?
> 
> Yes, it is used in some other tests. However, the other two aren't...
> 
> My biggest concern of moving them is that scrolling menus can have subtle bugs
> if done incorrectly, and there are about 5 different ways to do it, where only
> one works. We have had multiple test bugs with this. However, I also don't
like
> cluttering this file with APIs only used in one test class. :)
> 
> What do you think if I expose ScrollDown() as ScrollMenuDown() in
> chrome_actions, then call-sites could use that as their search action? Then
> tapSettingsMenuButton would live here, and I'd move the other two tap APIs
into
> settings_egtest.mm? Even if we have the helpers here, it won't prevent tests
> from doing it themselves. Having a custom scroll action might help?
> 
> I wanted to check before moving anything, or to see if you had other ideas.

After offline discussion with eugenebut@ it was determined that it can stay as
is. It's not clear how big this file will grow, and if/where would be the good
places to start splitting things up.

[baxley, 2017-05-09 21:20:20]

The CQ bit was checked by baxley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 21:21:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[baxley, 2017-05-09 21:22:43]

The CQ bit was unchecked by baxley@chromium.org

[baxley, 2017-05-09 21:22:48]

The CQ bit was checked by baxley@chromium.org

[baxley, 2017-05-09 21:22:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2867553002/#ps60001
(title: "revert extra history_ui_egtest change")

[commit-bot: I haz the power, 2017-05-09 21:23:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 00:47:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 00:47:23]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))

[baxley, 2017-05-10 02:19:34]

The CQ bit was checked by baxley@chromium.org

[commit-bot: I haz the power, 2017-05-10 02:20:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 04:22:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 04:22:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))

[baxley, 2017-05-10 16:15:29]

The CQ bit was checked by baxley@chromium.org

[commit-bot: I haz the power, 2017-05-10 16:16:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 17:17:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494432929235710,
"parent_rev": "694e49d71adce6c37b63d8c8efb708f1b6114e05", "commit_rev":
"959c01304869349d98de2df8775f7209a240dad8"}

[commit-bot: I haz the power, 2017-05-10 17:17:30]

Description was changed from

==========
Make settings_egtest.mm work in landscape on small phones.

Create ChromeEarlGreyUI methods to tap menu buttons, where it will
scroll and make sure the button is visible.

BUG=717614
==========

to

==========
Make settings_egtest.mm work in landscape on small phones.

Create ChromeEarlGreyUI methods to tap menu buttons, where it will
scroll and make sure the button is visible.

BUG=717614

Review-Url: https://codereview.chromium.org/2867553002
Cr-Commit-Position: refs/heads/master@{#470626}
Committed:
https://chromium.googlesource.com/chromium/src/+/959c01304869349d98de2df8775f...
==========

[commit-bot: I haz the power, 2017-05-10 17:17:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/959c01304869349d98de2df8775f...