Fix PasswordsSettingsTestCase.testCopyPasswordToast

Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small devices. Tapping the button results in showing a snackbar, which covers the button on small devices. testCopyPasswordToast in particular needs to tap the copy button twice. The second tap was impossible on small devices because the snackbar was still around, blocking the button.

This CL adds two actions to prevent this issue:

(1) While checking for the snackbar, the test also taps it, causing it to close. The test waits for the fade-out animation to finish.

(2) The test also uses usingSearchAction:grey_scrollInDirection to ask EarlGrey to scroll until the control is clickable.

Even though either single option from the above would fix the current issue, the CL contains both: (2) because it will also be useful on smaller devices, e.g., should we start testing in landscape mode (see https://crbug.com/717548 and also https://crbug.com/717163 for another blocker for landscape mode), where the buttons will be off the screen instead of just being covered. It also includes (1), because cleaning up the snackbar explicitly seems like a reasonable thing to do. Speaking of cleaning up, the CL also:

(3) Forcibly closes all passwords-related snackbars on teardown. This will be useful when the test fails in the middle, leaving a snackbar up. If not foce-closed, such snackbar would remain visible for (currently) 4 seconds, which might be enough to interfere with the next test.

The CL also applied (1) and (2) to two other tests in the same suite as well. Those deal with similar buttons, which are currently high enough not to experience the problem with being blocked. This secures them in case that, e.g., the layout changes in the future.

Note that while (2) is clearly beneficial, it also has a drawback: the search action takes a considerable amount of time (about 2 seconds per one scrolling step on my computer). With it, the tests are more likely to time out. If there is an issue with timing out in the future, we should increase kScrollAmount to speed up the scrolling, or split long tests (testCopyPasswordToast is a particular candidate).

BUG=718043
Review-Url: https://codereview.chromium.org/2860453004
Cr-Commit-Position: refs/heads/master@{#469361}
Committed: https://chromium.googlesource.com/chromium/src/+/6d0700c264fabe9d1b6857b2f860759544c4e640

[vabr (Chromium), 2017-05-03 17:37:35]

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

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

Dry run: CQ is trying da patch.

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

[vabr (Chromium), 2017-05-03 17:44:51]

Description was changed from

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The test failed to click on some controls on devices with small screen.

The fix uses usingSearchAction:grey_scrollInDirection to ensure that the
control is clickable.

BUG=718043
==========

to

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

My first reaction was to use usingSearchAction:grey_scrollInDirection to ask
EarlGrey to scroll until the control is clickable. This failed, because when the
snackbar was up, there seemed nothing to scroll on. In particular, while without
the snackbar I was able to scroll on the
PasswordDetailsCollectionViewController, with it it was no longer possible.

The alternatives which worked were: (A) scrolling in advance before showing the
snackbar, or (B) closing the snackbar before resuming.

(A) seemed slightly more hacky than (B), so I went with (B).

usingSearchAction:grey_scrollInDirection will still be needed if we decide to
test in landscape mode (see https://crbug.com/717548), but at this moment it
does not seem necessary yet.

BUG=718043
==========

[vabr (Chromium), 2017-05-03 17:44:52]

vabr@chromium.org changed reviewers:
+ baxley@chromium.org, lpromero@chromium.org

[vabr (Chromium), 2017-05-03 17:46:47]

Hi baxley@,
Please check whether my fix makes sense to you. In particular, let me know if
you want me to throw in the scrolling as well, for the sake of the tests ran in
landscape in the future.

Hi lpromer@,
Please do an OWNERS review.

Thanks!
Vaclav

[commit-bot: I haz the power, 2017-05-03 17:48:14]

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

[commit-bot: I haz the power, 2017-05-03 17:48:15]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-05-03 17:49:19]

Description was changed from

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

My first reaction was to use usingSearchAction:grey_scrollInDirection to ask
EarlGrey to scroll until the control is clickable. This failed, because when the
snackbar was up, there seemed nothing to scroll on. In particular, while without
the snackbar I was able to scroll on the
PasswordDetailsCollectionViewController, with it it was no longer possible.

The alternatives which worked were: (A) scrolling in advance before showing the
snackbar, or (B) closing the snackbar before resuming.

(A) seemed slightly more hacky than (B), so I went with (B).

usingSearchAction:grey_scrollInDirection will still be needed if we decide to
test in landscape mode (see https://crbug.com/717548), but at this moment it
does not seem necessary yet.

BUG=718043
==========

to

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

My first reaction was to use usingSearchAction:grey_scrollInDirection to ask
EarlGrey to scroll until the control is clickable. This failed, because when the
snackbar was up, there seemed nothing to scroll on. In particular, while without
the snackbar I was able to scroll on the
PasswordDetailsCollectionViewController, with it it was no longer possible.

The alternatives which worked were: (A) scrolling in advance before showing the
snackbar, or (B) closing the snackbar before resuming.

(A) seemed slightly more hacky than (B), so I went with (B).

usingSearchAction:grey_scrollInDirection will still be needed if we decide to
test in landscape mode (see https://crbug.com/717548), but at this moment it
does not seem necessary yet. Currently, the test in landscape mode fails much
earlier -- it cannot open the settings at all (see https://crbug.com/717163).

BUG=718043
==========

[baxley, 2017-05-03 18:07:11]

I agree with your approach, thanks for the good description/comment. I just have
one question about if the drainUntilIdle is guaranteed to have the UI complete
and remove the snackbar, or if it acts as a sleep statement.

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

https://codereview.chromium.org/2860453004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:381:
[[GREYUIThreadExecutor sharedInstance] drainUntilIdle];
Does this guarantee that the snackbar is gone when the drain is complete? Or
does this act as more of a sleep?

If it's the former, then I think it's fine. If it's the latter, this can become
a source of flake and would prefer waiting for some state that we know we would
reach when the snackbar is gone.

[vabr (Chromium), 2017-05-03 20:32:26]

Thanks for the review. Please see my response below.

Cheers,
Vaclav

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

https://codereview.chromium.org/2860453004/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:381:
[[GREYUIThreadExecutor sharedInstance] drainUntilIdle];
On 2017/05/03 18:07:11, baxley wrote:
> Does this guarantee that the snackbar is gone when the drain is complete? Or
> does this act as more of a sleep?
> 
> If it's the former, then I think it's fine. If it's the latter, this can
become
> a source of flake and would prefer waiting for some state that we know we
would
> reach when the snackbar is gone.

I have good news. When I caused the fade-out animation to take 5 seconds instead
of the usual 0.5 (by adjusting MDCSnackbarTransitionDuration in
ios/third_party/material_components_ios/src/components/Snackbar/src/private/MDCSnackbarOverlayView.m
, the test still patiently waited for the animation to complete before
continuing (and it passed). So it seems that drainUntilIdle indeed waits until
the snackbar is gone.

[baxley, 2017-05-03 20:49:28]

On 2017/05/03 20:32:26, vabr (hobby only until 5 June) wrote:
> Thanks for the review. Please see my response below.
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/2860453004/diff/20001/ios/chrome/browser/ui/s...
> File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):
> 
>
https://codereview.chromium.org/2860453004/diff/20001/ios/chrome/browser/ui/s...
> ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:381:
> [[GREYUIThreadExecutor sharedInstance] drainUntilIdle];
> On 2017/05/03 18:07:11, baxley wrote:
> > Does this guarantee that the snackbar is gone when the drain is complete? Or
> > does this act as more of a sleep?
> > 
> > If it's the former, then I think it's fine. If it's the latter, this can
> become
> > a source of flake and would prefer waiting for some state that we know we
> would
> > reach when the snackbar is gone.
> 
> I have good news. When I caused the fade-out animation to take 5 seconds
instead
> of the usual 0.5 (by adjusting MDCSnackbarTransitionDuration in
>
ios/third_party/material_components_ios/src/components/Snackbar/src/private/MDCSnackbarOverlayView.m
> , the test still patiently waited for the animation to complete before
> continuing (and it passed). So it seems that drainUntilIdle indeed waits until
> the snackbar is gone.

Thanks for verifying!
LGTM

[vabr (Chromium), 2017-05-03 20:51:03]

One more update: I realised that keeping the snackbars open after the end of the
test might also lead to flakes -- the snackbars stays for 4 seconds, which might
be enough for the next test case to start (although unlikely). If the snackbar
happens to block the UI for the next testcase, that would lead to flakes.

So I made sure to always close the snackbar in all the tests here, and added
appropriate comments. Please see them in patch set 3. I will also update the CL
description.

Cheers,
Vaclav

[vabr (Chromium), 2017-05-03 20:52:56]

Description was changed from

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

My first reaction was to use usingSearchAction:grey_scrollInDirection to ask
EarlGrey to scroll until the control is clickable. This failed, because when the
snackbar was up, there seemed nothing to scroll on. In particular, while without
the snackbar I was able to scroll on the
PasswordDetailsCollectionViewController, with it it was no longer possible.

The alternatives which worked were: (A) scrolling in advance before showing the
snackbar, or (B) closing the snackbar before resuming.

(A) seemed slightly more hacky than (B), so I went with (B).

usingSearchAction:grey_scrollInDirection will still be needed if we decide to
test in landscape mode (see https://crbug.com/717548), but at this moment it
does not seem necessary yet. Currently, the test in landscape mode fails much
earlier -- it cannot open the settings at all (see https://crbug.com/717163).

BUG=718043
==========

to

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

My first reaction was to use usingSearchAction:grey_scrollInDirection to ask
EarlGrey to scroll until the control is clickable. This failed, because when the
snackbar was up, there seemed nothing to scroll on. In particular, while without
the snackbar I was able to scroll on the
PasswordDetailsCollectionViewController, with it it was no longer possible.

The alternatives which worked were: (A) scrolling in advance before showing the
snackbar, or (B) closing the snackbar before resuming.

(A) seemed slightly more hacky than (B), so I went with (B).

usingSearchAction:grey_scrollInDirection will still be needed if we decide to
test in landscape mode (see https://crbug.com/717548), but at this moment it
does not seem necessary yet. Currently, the test in landscape mode fails much
earlier -- it cannot open the settings at all (see https://crbug.com/717163).

While at it, this CL also ensures that all the snackbars are closed, even at the
test end. The snackbar stays up for 4 seconds by default, and in theory it could
affect the next test run. To avoid flakes based on that, the tests close their
snackbars and wait for the fade-out to complete.

BUG=718043
==========

[vabr (Chromium), 2017-05-03 20:54:21]

Thanks for the review, baxley@!

We appeared to have posted our latest updates at the same time, so I'd like to
highlight #12, where I announce one more change (the diff of patch set 3 against
p.s. 2). Just in case you found that controversial and would like to retract
your approval. :)

Cheers,
Vaclav

[baxley, 2017-05-03 21:10:58]

On 2017/05/03 20:54:21, vabr (hobby only until 5 June) wrote:
> Thanks for the review, baxley@!
> 
> We appeared to have posted our latest updates at the same time, so I'd like to
> highlight #12, where I announce one more change (the diff of patch set 3
against
> p.s. 2). Just in case you found that controversial and would like to retract
> your approval. :)
> 
> Cheers,
> Vaclav

Is it possible for the test to fail, leaving the popup displayed? If so, is
there a way to clean it up after the test (either in tearDown or
setTearDownHandler)? I don't think it's that controversial, especially since in
the failure case it will disappear in seconds.

[lpromero, 2017-05-04 09:21:08]

lgtm

https://codereview.chromium.org/2860453004/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2860453004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:378: // button won't
work when the snackbar is up.
Is this an action item we could solve? Scrolling on visible pieces of UI ought
to work, no? Or is the snackbar modal in some way that it prevents any
interaction with any other place on the screen?

[vabr (Chromium), 2017-05-04 11:00:04]

Thanks both for the review!

lpromero@ -- I don't have a good answer yet (see below). The test seems to do a
reasonable thing for now, so one way forward would be to remove the "scrolling
to reach the button won't work when the snackbar is up" from the code comment
and land it as it is now, with me having a look at how to do the scrolling
properly as a follow-up. Or we can just keep the test disabled and I'll
investigate next week.

baxley@:
> Is it possible for the test to fail, leaving the popup displayed?
It sounds possible, good catch.

> If so, is there a way to clean it up after the test (either in tearDown or
setTearDownHandler)?
Indeed, I can introduce a category for the snackbar message and then call
[MDCSnackbarManager dismissAndCallCompletionBlocksWithCategory]. This seems a
better idea than tapping it at the end. I will check the details and upload a
new patch set soon, or I can do it as a follow-up if we want to have the test
enabled now.

I'll post an update once the new patchset is up. I'm offline this Friday and
over weekend, so might come back first next week.

Cheers,
Vaclav

https://codereview.chromium.org/2860453004/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2860453004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:378: // button won't
work when the snackbar is up.
On 2017/05/04 09:21:08, lpromero wrote:
> Is this an action item we could solve? Scrolling on visible pieces of UI ought
> to work, no? Or is the snackbar modal in some way that it prevents any
> interaction with any other place on the screen?

When I tried manually with Chrome, I was able to scroll the view under the
snackbar just fine. When I was trying with the EG test yesterday, I tried
scrolling on the PasswordDetailsCollectionViewController (which I assigned an
accessibilityID for easy matching). Scrolling on the controller worked as long
as the snackbar was not up, but with the snackbar it was broken (can't remember
now whether the controller was not interactable, or whether the scrolling just
failed).

I will have a deeper look, will probably need to find better objects to scroll
on (as trivial as that seems, it took me hours yesterday and I failed).

[lpromero, 2017-05-04 11:24:54]

https://codereview.chromium.org/2860453004/diff/40001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2860453004/diff/40001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:378: // button won't
work when the snackbar is up.
On 2017/05/04 11:00:04, vabr (hobby only until 5 June) wrote:
> On 2017/05/04 09:21:08, lpromero wrote:
> > Is this an action item we could solve? Scrolling on visible pieces of UI
ought
> > to work, no? Or is the snackbar modal in some way that it prevents any
> > interaction with any other place on the screen?
> 
> When I tried manually with Chrome, I was able to scroll the view under the
> snackbar just fine. When I was trying with the EG test yesterday, I tried
> scrolling on the PasswordDetailsCollectionViewController (which I assigned an
> accessibilityID for easy matching). Scrolling on the controller worked as long
> as the snackbar was not up, but with the snackbar it was broken (can't
remember
> now whether the controller was not interactable, or whether the scrolling just
> failed).
> 
> I will have a deeper look, will probably need to find better objects to scroll
> on (as trivial as that seems, it took me hours yesterday and I failed).

To be clear, I am fine as is, I was just wondering if we should track it with a
bug or report to MDC or EarlGrey. Accessibility and VoiceOver have a concept of
isAccessibilityModal, that makes some parts of the UI unreachable with VoiceOver
when a modal is up (but still interactive without VoiceOver). I wondered if
EarlGrey was relying on VoiceOver there too and would be trapped in this case.

Please don't be blocked on this discussion that goes beyond the scope of your
CL.

[vabr (Chromium), 2017-05-04 14:39:02]

Description was changed from

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

My first reaction was to use usingSearchAction:grey_scrollInDirection to ask
EarlGrey to scroll until the control is clickable. This failed, because when the
snackbar was up, there seemed nothing to scroll on. In particular, while without
the snackbar I was able to scroll on the
PasswordDetailsCollectionViewController, with it it was no longer possible.

The alternatives which worked were: (A) scrolling in advance before showing the
snackbar, or (B) closing the snackbar before resuming.

(A) seemed slightly more hacky than (B), so I went with (B).

usingSearchAction:grey_scrollInDirection will still be needed if we decide to
test in landscape mode (see https://crbug.com/717548), but at this moment it
does not seem necessary yet. Currently, the test in landscape mode fails much
earlier -- it cannot open the settings at all (see https://crbug.com/717163).

While at it, this CL also ensures that all the snackbars are closed, even at the
test end. The snackbar stays up for 4 seconds by default, and in theory it could
affect the next test run. To avoid flakes based on that, the tests close their
snackbars and wait for the fade-out to complete.

BUG=718043
==========

to

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

This CL adds two actions to prevent this issue:

(1) While checking for the snackbar, the test also taps it, causing it to close.
The test waits for the fade-out animation to finish.

(2) The test also uses usingSearchAction:grey_scrollInDirection to ask EarlGrey
to scroll until the control is clickable.

Even though either single option from the above would fix the current issue, the
CL contains both: (2) because it will also be useful on smaller devices, e.g.,
should we start testing in landscape mode (see https://crbug.com/717548 and also
https://crbug.com/717163 for another blocker for landscape mode), where the
buttons will be off the screen instead of just being covered. It also includes
(1), because cleaning up the snackbar explicitly seems like a reasonable thing
to do. Speaking of cleaning up, the CL also:

(3) Forcibly closes all passwords-related snackbars on teardown. This will be
useful when the test fails in the middle, leaving a snackbar up. If not
foce-closed, such snackbar would remain visible for (currently) 4 seconds, which
might be enough to interfere with the next test.

The CL also applied (1) and (2) to two other tests in the same suite as well.
Those deal with similar buttons, which are currently high enough not to
experience the problem with being blocked. This secures them in case that, e.g.,
the layout changes in the future.

Note that while (2) is clearly beneficial, it also has a drawback: the search
action takes a considerable amount of time (about 2 seconds per one scrolling
step on my computer). With it, the tests are more likely to time out. If there
is an issue with timing out in the future, we should increase kScrollAmount to
speed up the scrolling, or split long tests (testCopyPasswordToast is a
particular candidate).

BUG=718043
==========

[vabr (Chromium), 2017-05-04 14:46:52]

Thanks again, both of you, for asking the right questions!

I added the teardown for closing snackbars in patch set 4.

I also added the scrolling in patch set 5. Please see the updated CL description
for more details.

Let me share the mistake I made with scrolling, in case you hit it in the future
as well: I paid too little attention to checking interactability
(grey_interactable()). My matchers for the buttons did not check
interactability, so the search-by-scrolling did always exit early (the elements
were there, just not interactable, because off the screen or covered). But then
the grey_tap() action failed, because it implicitly asserts interactability
(tapping is, after all, an interaction). The fix was to add grey_interactable()
as part of the button matchers; this kept the search going until the element was
indeed interactable.

Finally, let me note that I tested this even in landscape mode, and it seems
fine (involved me rotating the simulator during the test run due to
https://crbug.com/717163).

I'd like to wait until lpromero@ re-approves, because my changes in the
direction of his comments were significant. I think that I addressed baxley@'s
concerns in a straightforward way, so will not block landing on that. However,
there is no rush, the tests only check feature which is not launched yet and
should be also fine to be fixed next week.

Cheers,
Vaclav

[lpromero, 2017-05-04 15:55:41]

Latest patch LGTM.
Thanks for going to the bottom of it!

https://codereview.chromium.org/2860453004/diff/70001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2860453004/diff/70001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:55: constexpr int
kScrollAmount = 150;
No action needed: I didn't know about constexpr.

[baxley, 2017-05-04 16:25:28]

On 2017/05/04 14:46:52, vabr (hobby-only until 5 June) wrote:
> Thanks again, both of you, for asking the right questions!
> 
> I added the teardown for closing snackbars in patch set 4.
> 
> I also added the scrolling in patch set 5. Please see the updated CL
description
> for more details.
> 
> Let me share the mistake I made with scrolling, in case you hit it in the
future
> as well: I paid too little attention to checking interactability
> (grey_interactable()). My matchers for the buttons did not check
> interactability, so the search-by-scrolling did always exit early (the
elements
> were there, just not interactable, because off the screen or covered). But
then
> the grey_tap() action failed, because it implicitly asserts interactability
> (tapping is, after all, an interaction). The fix was to add
grey_interactable()
> as part of the button matchers; this kept the search going until the element
was
> indeed interactable.
> 
> Finally, let me note that I tested this even in landscape mode, and it seems
> fine (involved me rotating the simulator during the test run due to
> https://crbug.com/717163).
> 
> I'd like to wait until lpromero@ re-approves, because my changes in the
> direction of his comments were significant. I think that I addressed baxley@'s
> concerns in a straightforward way, so will not block landing on that. However,
> there is no rush, the tests only check feature which is not launched yet and
> should be also fine to be fixed next week.
still LGTM! Thanks for all the investigation and putting up with all the
back-and-forth. I'm encountering some of these same issues as I try to make our
test interaction with menus more robust.

[vabr (Chromium), 2017-05-04 16:38:20]

Thanks for the swift replies, both of you!

I'm landing now and will check on the bots later tonight.

Cheers,
Vaclav

https://codereview.chromium.org/2860453004/diff/70001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/passwords_settings_egtest.mm (right):

https://codereview.chromium.org/2860453004/diff/70001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/passwords_settings_egtest.mm:55: constexpr int
kScrollAmount = 150;
On 2017/05/04 15:55:41, lpromero wrote:
> No action needed: I didn't know about constexpr.

I'm also getting used to it. It is part of C++11, but clear and low-level enough
IMO to be useful also in Objective-C++.

[vabr (Chromium), 2017-05-04 16:38:25]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-05-04 16:38:58]

CQ is trying da patch.

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

[baxley, 2017-05-04 16:44:21]

On 2017/05/04 16:38:20, vabr (hobby-only until 5 June) wrote:
> Thanks for the swift replies, both of you!
> 
> I'm landing now and will check on the bots later tonight.
You're in luck, I'm iOS sheriff the next few days and know what to look for, so
don't worry about checking in outside of work hours.

[commit-bot: I haz the power, 2017-05-04 16:53:17]

CQ is committing da patch.

Bot data: {"patchset_id": 70001, "attempt_start_ts": 1493915905942360,
"parent_rev": "69b6c44ab3317745f442a91b24b89e4e2f6ca20b", "commit_rev":
"6d0700c264fabe9d1b6857b2f860759544c4e640"}

[commit-bot: I haz the power, 2017-05-04 16:54:26]

Description was changed from

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

This CL adds two actions to prevent this issue:

(1) While checking for the snackbar, the test also taps it, causing it to close.
The test waits for the fade-out animation to finish.

(2) The test also uses usingSearchAction:grey_scrollInDirection to ask EarlGrey
to scroll until the control is clickable.

Even though either single option from the above would fix the current issue, the
CL contains both: (2) because it will also be useful on smaller devices, e.g.,
should we start testing in landscape mode (see https://crbug.com/717548 and also
https://crbug.com/717163 for another blocker for landscape mode), where the
buttons will be off the screen instead of just being covered. It also includes
(1), because cleaning up the snackbar explicitly seems like a reasonable thing
to do. Speaking of cleaning up, the CL also:

(3) Forcibly closes all passwords-related snackbars on teardown. This will be
useful when the test fails in the middle, leaving a snackbar up. If not
foce-closed, such snackbar would remain visible for (currently) 4 seconds, which
might be enough to interfere with the next test.

The CL also applied (1) and (2) to two other tests in the same suite as well.
Those deal with similar buttons, which are currently high enough not to
experience the problem with being blocked. This secures them in case that, e.g.,
the layout changes in the future.

Note that while (2) is clearly beneficial, it also has a drawback: the search
action takes a considerable amount of time (about 2 seconds per one scrolling
step on my computer). With it, the tests are more likely to time out. If there
is an issue with timing out in the future, we should increase kScrollAmount to
speed up the scrolling, or split long tests (testCopyPasswordToast is a
particular candidate).

BUG=718043
==========

to

==========
Fix PasswordsSettingsTestCase.testCopyPasswordToast

The tested settings have a button just at the bottom end of the screen on small
devices. Tapping the button results in showing a snackbar, which covers the
button on small devices. testCopyPasswordToast in particular needs to tap the
copy button twice. The second tap was impossible on small devices because the
snackbar was still around, blocking the button.

This CL adds two actions to prevent this issue:

(1) While checking for the snackbar, the test also taps it, causing it to close.
The test waits for the fade-out animation to finish.

(2) The test also uses usingSearchAction:grey_scrollInDirection to ask EarlGrey
to scroll until the control is clickable.

Even though either single option from the above would fix the current issue, the
CL contains both: (2) because it will also be useful on smaller devices, e.g.,
should we start testing in landscape mode (see https://crbug.com/717548 and also
https://crbug.com/717163 for another blocker for landscape mode), where the
buttons will be off the screen instead of just being covered. It also includes
(1), because cleaning up the snackbar explicitly seems like a reasonable thing
to do. Speaking of cleaning up, the CL also:

(3) Forcibly closes all passwords-related snackbars on teardown. This will be
useful when the test fails in the middle, leaving a snackbar up. If not
foce-closed, such snackbar would remain visible for (currently) 4 seconds, which
might be enough to interfere with the next test.

The CL also applied (1) and (2) to two other tests in the same suite as well.
Those deal with similar buttons, which are currently high enough not to
experience the problem with being blocked. This secures them in case that, e.g.,
the layout changes in the future.

Note that while (2) is clearly beneficial, it also has a drawback: the search
action takes a considerable amount of time (about 2 seconds per one scrolling
step on my computer). With it, the tests are more likely to time out. If there
is an issue with timing out in the future, we should increase kScrollAmount to
speed up the scrolling, or split long tests (testCopyPasswordToast is a
particular candidate).

BUG=718043

Review-Url: https://codereview.chromium.org/2860453004
Cr-Commit-Position: refs/heads/master@{#469361}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d0700c264fabe9d1b6857b2f860...
==========

[commit-bot: I haz the power, 2017-05-04 16:54:27]

Committed patchset #5 (id:70001) as
https://chromium.googlesource.com/chromium/src/+/6d0700c264fabe9d1b6857b2f860...

[vabr (Chromium), 2017-05-04 17:26:05]

On 2017/05/04 16:44:21, baxley wrote:
> On 2017/05/04 16:38:20, vabr (hobby-only until 5 June) wrote:
> > Thanks for the swift replies, both of you!
> > 
> > I'm landing now and will check on the bots later tonight.
> You're in luck, I'm iOS sheriff the next few days and know what to look for,
so
> don't worry about checking in outside of work hours.

That's great, thanks a lot!
Vaclav