Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906313004:
Making the gradient click-through in ChromeSigninViewController (Closed)
|
Created:
3 years, 6 months ago by jlebel Modified:
3 years, 6 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMaking the gradient click-through in ChromeSigninViewController
"Settings" button is partially below the gradient while using small screen
like iPhone SE. This is probably the reason why testSignInOpenSettings
and testSignInAndTapSettingsLink can be flaky.
Also, updating those 2 tests to make it more robust to animation by
adding calls to WaitForMatcher(), to wait for "Settings" to be displayed.
BUG=718023
Review-Url: https://codereview.chromium.org/2906313004
Cr-Commit-Position: refs/heads/master@{#476691}
Committed: https://chromium.googlesource.com/chromium/src/+/b87d80f6485276511b3c543e90cde926eed2838d
Patch Set 1 #
Total comments: 5
Patch Set 2 : Adding WaitForMatcher() #
Total comments: 2
Patch Set 3 : Adding FLAKY prefix #
Total comments: 12
Patch Set 4 : Cleanup #Patch Set 5 : Fixing the test for iPhone SE #Patch Set 6 : Enabling the tests #Patch Set 7 : Fixing dependencies ( Merge) #
Messages
Total messages: 46 (26 generated)
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Removing animations in flaky tests BUG=718023 ========== to ========== Adding delay in testSignInOpenSettings and testSignInAndTapSettingsLink Adding a delay before performing a tap on "Settings" link in the confirmation sign-in view. BUG=718023 ==========
Description was changed from ========== Adding delay in testSignInOpenSettings and testSignInAndTapSettingsLink Adding a delay before performing a tap on "Settings" link in the confirmation sign-in view. BUG=718023 ========== to ========== Adding delay in testSignInOpenSettings and testSignInAndTapSettingsLink Adding a delay before performing a tap on "Settings" link in the confirmation sign-in view. It looks like the confirmation sign-in view animation is not done when EarlGrey taps on the link. BUG=718023 ==========
Patchset #1 (id:1) has been deleted
jlebel@chromium.org changed reviewers: + sdefresne@chromium.org
Helo Sylvain, Can you review this patch? Thanks,
sdefresne@chromium.org changed reviewers: + baxley@chromium.org
I'm not a fan of delay in tests. Adding Mike so that he can comment on whether there is a way to fix this flakyness without introducing a delay.
On 2017/05/31 14:53:01, sdefresne wrote: > I'm not a fan of delay in tests. Adding Mike so that he can comment on whether > there is a way to fix this flakyness without introducing a delay. My other solution is to remove animation all together. My biggest issue is I can't reproduce it on my computer, so I'm not totally sure why EarlGrey didn't fail to tap on the link but the next screen didn't appear.
On 2017/05/31 14:56:52, jlebel wrote: > On 2017/05/31 14:53:01, sdefresne wrote: > > I'm not a fan of delay in tests. Adding Mike so that he can comment on whether > > there is a way to fix this flakyness without introducing a delay. > > My other solution is to remove animation all together. My biggest issue is I > can't > reproduce it on my computer, so I'm not totally sure why EarlGrey didn't fail to > tap on the link but the next screen didn't appear. Since it is possible to remove the animation, can you try to debug by increasing the duration of the animations locally?
I agree with Sylvain's concern with a delay. We can typically avoid it by waiting for the condition we expect to be true, which is faster and less flaky. Let me know if I'm missing something and my suggestions don't work. Thanks! https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:374: base::test::ios::SpinRunLoopWithMaxDelay(base::TimeDelta::FromSecondsD(0.5)); delays are bad to use in tests. In the best case, the test is slowed down for 0.5 every run. In the worst case, there are times where this delay is not long enough and it is still flaky. I understand there are cases where we cannot synchronize for some actions. Ideally, we'd make it synchronized, but if not it is better to poll for a condition, with timeout. Could you useWaitForMatcher on line 96 in this file? Instead of a delay, it polls for the matcher to succeed. If it doesn't find it in 4 seconds, then it fails. https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/f... File ios/chrome/browser/ui/first_run/first_run_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/f... ios/chrome/browser/ui/first_run/first_run_egtest.mm:236: base::test::ios::SpinRunLoopWithMaxDelay(base::TimeDelta::FromSecondsD(0.5)); Similar to the other comment, could you add in the logic to wait for the matcher. I'm okay if you just put the code inline. I don't know if we want it in a shared method, since it may make it to easy for people to use. It should only be used as a last resort. We have had cases where there were animations that were running when they shouldn't have, so the tests actually found a bug.
Thanks for your input https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:374: base::test::ios::SpinRunLoopWithMaxDelay(base::TimeDelta::FromSecondsD(0.5)); On 2017/05/31 15:51:29, baxley wrote: > delays are bad to use in tests. In the best case, the test is slowed down for > 0.5 every run. In the worst case, there are times where this delay is not long > enough and it is still flaky. > > I understand there are cases where we cannot synchronize for some actions. > Ideally, we'd make it synchronized, but if not it is better to poll for a > condition, with timeout. > > Could you useWaitForMatcher on line 96 in this file? Instead of a delay, it > polls for the matcher to succeed. If it doesn't find it in 4 seconds, then it > fails. This test doesn't fail to find "Settings", neither to tap on it. But it fails find the "Done" button in the next screen. According to the crash: https://paste.googleplex.com/5483254505275392 we believe link is found while the screen is still moving. So either the button is disabled while moving or the NSEvent is missing the "Settings" button since it is still moving. I guess another way to fix it (if this is really the problem), is to create NSNotification from my view controller once the animation is finished. But I don't think it is a good idea to create code just for testing. If I turn on the slow animation, the test fails to find "Settings" button (for this test, for the other one, it fails at the beginning). I don't know if there is any better solution. I'm very new to EarlGrey.
https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:374: base::test::ios::SpinRunLoopWithMaxDelay(base::TimeDelta::FromSecondsD(0.5)); On 2017/05/31 16:16:40, jlebel wrote: > On 2017/05/31 15:51:29, baxley wrote: > > delays are bad to use in tests. In the best case, the test is slowed down for > > 0.5 every run. In the worst case, there are times where this delay is not long > > enough and it is still flaky. > > > > I understand there are cases where we cannot synchronize for some actions. > > Ideally, we'd make it synchronized, but if not it is better to poll for a > > condition, with timeout. > > > > Could you useWaitForMatcher on line 96 in this file? Instead of a delay, it > > polls for the matcher to succeed. If it doesn't find it in 4 seconds, then it > > fails. > > This test doesn't fail to find "Settings", neither to tap on it. But it fails > find the "Done" button in the next screen. According to the crash: > https://paste.googleplex.com/5483254505275392 > we believe link is found while the screen is still moving. So either the > button is disabled while moving or the NSEvent is missing the "Settings" > button since it is still moving. What about if you add more matcher constraints by using grey_allOf(matcher1, matcher2, ... , nil); Perhaps matching "Done", with "grey_interactable()"? If you did this, then maybe no delay would be necessary? This was a problem we had with scrolling and tapping menu items, that they are sometimes present but not tappable since it's not interactable. > > I guess another way to fix it (if this is really the problem), is to create > NSNotification from my view controller once the animation is finished. > But I don't think it is a good idea to create code just for testing. Yeah, that may be overkill. The examples I alluded to were cases where a background animation was running, that should not have been.
https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/20001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:374: base::test::ios::SpinRunLoopWithMaxDelay(base::TimeDelta::FromSecondsD(0.5)); On 2017/05/31 16:29:39, baxley wrote: > On 2017/05/31 16:16:40, jlebel wrote: > > On 2017/05/31 15:51:29, baxley wrote: > > > delays are bad to use in tests. In the best case, the test is slowed down > for > > > 0.5 every run. In the worst case, there are times where this delay is not > long > > > enough and it is still flaky. > > > > > > I understand there are cases where we cannot synchronize for some actions. > > > Ideally, we'd make it synchronized, but if not it is better to poll for a > > > condition, with timeout. > > > > > > Could you useWaitForMatcher on line 96 in this file? Instead of a delay, it > > > polls for the matcher to succeed. If it doesn't find it in 4 seconds, then > it > > > fails. > > > > This test doesn't fail to find "Settings", neither to tap on it. But it fails > > find the "Done" button in the next screen. According to the crash: > > https://paste.googleplex.com/5483254505275392 > > we believe link is found while the screen is still moving. So either the > > button is disabled while moving or the NSEvent is missing the "Settings" > > button since it is still moving. > What about if you add more matcher constraints by using grey_allOf(matcher1, > matcher2, ... , nil); > > Perhaps matching "Done", with "grey_interactable()"? If you did this, then maybe > no delay would be necessary? This was a problem we had with scrolling and > tapping menu items, that they are sometimes present but not tappable since it's > not interactable. > > > > I guess another way to fix it (if this is really the problem), is to create > > NSNotification from my view controller once the animation is finished. > > But I don't think it is a good idea to create code just for testing. > Yeah, that may be overkill. The examples I alluded to were cases where a > background animation was running, that should not have been. > Since according to the log, the UI is still on the previous view with the "Settings" button. So one possibility is that the tap failed. This could happen if EarlGrey tries to tap it while its view appears with animation. That would explain why the element is found but why the next screen is not displayed. Unfortunately, I can't reproduce it on my computer. When I activate slow animation, "Settings" is not even found. But I followed your advice by adding WaitForMatcher() (and I also added more matcher). Now I can make this test working with slow animation. I would be interested to try if it solve the flakiness.
https://codereview.chromium.org/2906313004/diff/40001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (left): https://codereview.chromium.org/2906313004/diff/40001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:366: - (void)DISABLED_testSignInOpenSettings { If you prefix it with FLAKY_ (i.e. s/DISABLED_/FLAKY_), then it will run on our downstream flaky bots.
Thanks for your patience. A few more questions. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (left): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:364: // Settings shown. The previous 3 lines are test comments, could you leave them in? https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:362: - (void)FLAKY_testSignInOpenSettings { Could you keep the TODO comment? https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:375: grey_enabled(), grey_interactable(), nil); are these extra constraints needed? Or with these constraints do we not need the WaitForMatcher call? https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... File ios/chrome/browser/ui/first_run/first_run_egtest.mm (left): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... ios/chrome/browser/ui/first_run/first_run_egtest.mm:218: // Signs in to an account and then taps the Advanced link to go to settings. This is the test comment, so it should stay. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... File ios/chrome/browser/ui/first_run/first_run_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... ios/chrome/browser/ui/first_run/first_run_egtest.mm:233: - (void)FLAKY_testSignInAndTapSettingsLink { Could you keep the TODO? https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... ios/chrome/browser/ui/first_run/first_run_egtest.mm:252: WaitForMatcher(settings_link_matcher); Similar to in the other file, do we need the extra matchers and to wait for the matcher? Ideally, we wouldn't WaitForMatcher. But if that's required we shouldn't use more matchers than we need. If we need all of it, then it's fine as is.
Patchset #4 (id:80001) has been deleted
Thanks for your help https://codereview.chromium.org/2906313004/diff/40001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (left): https://codereview.chromium.org/2906313004/diff/40001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:366: - (void)DISABLED_testSignInOpenSettings { On 2017/06/01 15:36:21, baxley wrote: > If you prefix it with FLAKY_ (i.e. s/DISABLED_/FLAKY_), then it will run on our > downstream flaky bots. Done. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (left): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:364: // Settings shown. On 2017/06/01 17:16:25, baxley wrote: > The previous 3 lines are test comments, could you leave them in? Done. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... File ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:362: - (void)FLAKY_testSignInOpenSettings { On 2017/06/01 17:16:25, baxley wrote: > Could you keep the TODO comment? Done. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/a... ios/chrome/browser/ui/authentication/signin_interaction_controller_egtest.mm:375: grey_enabled(), grey_interactable(), nil); On 2017/06/01 17:16:25, baxley wrote: > are these extra constraints needed? > Or with these constraints do we not need the WaitForMatcher call? Yes, both of them are not needed. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... File ios/chrome/browser/ui/first_run/first_run_egtest.mm (left): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... ios/chrome/browser/ui/first_run/first_run_egtest.mm:218: // Signs in to an account and then taps the Advanced link to go to settings. On 2017/06/01 17:16:25, baxley wrote: > This is the test comment, so it should stay. Done. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... File ios/chrome/browser/ui/first_run/first_run_egtest.mm (right): https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... ios/chrome/browser/ui/first_run/first_run_egtest.mm:233: - (void)FLAKY_testSignInAndTapSettingsLink { On 2017/06/01 17:16:25, baxley wrote: > Could you keep the TODO? Done. https://codereview.chromium.org/2906313004/diff/60001/ios/chrome/browser/ui/f... ios/chrome/browser/ui/first_run/first_run_egtest.mm:252: WaitForMatcher(settings_link_matcher); On 2017/06/01 17:16:25, baxley wrote: > Similar to in the other file, do we need the extra matchers and to wait for the > matcher? > > Ideally, we wouldn't WaitForMatcher. But if that's required we shouldn't use > more matchers than we need. If we need all of it, then it's fine as is. Done.
Description was changed from ========== Adding delay in testSignInOpenSettings and testSignInAndTapSettingsLink Adding a delay before performing a tap on "Settings" link in the confirmation sign-in view. It looks like the confirmation sign-in view animation is not done when EarlGrey taps on the link. BUG=718023 ========== to ========== Making the gradient click-through in ChromeSigninViewController "Settings" button is partially below the gradient while using small screen like iPhone SE. This is probably the reason why testSignInOpenSettings and testSignInAndTapSettingsLink can be flaky. Also, updating those 2 tests to make it more robust to animation by adding calls to WaitForMatcher(), to wait for "Settings" to be displayed. BUG=718023 ==========
jlebel@chromium.org changed reviewers: + msarda@chromium.org
Hello Mihai, Can you review this patch about fixing the flaky tests? Thanks,
Hello Mihai, Can you review this patch about fixing the flaky tests? Thanks,
lgtm
The CQ bit was checked by msarda@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by msarda@chromium.org
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by jlebel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm
The CQ bit was unchecked by jlebel@chromium.org
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2906313004/#ps180001 (title: "Fixing dependencies ( Merge)")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1496418036248040,
"parent_rev": "ea6510d0290d12d09ca9bb29e4b332718d06e19d", "commit_rev":
"b87d80f6485276511b3c543e90cde926eed2838d"}
Message was sent while issue was closed.
Description was changed from ========== Making the gradient click-through in ChromeSigninViewController "Settings" button is partially below the gradient while using small screen like iPhone SE. This is probably the reason why testSignInOpenSettings and testSignInAndTapSettingsLink can be flaky. Also, updating those 2 tests to make it more robust to animation by adding calls to WaitForMatcher(), to wait for "Settings" to be displayed. BUG=718023 ========== to ========== Making the gradient click-through in ChromeSigninViewController "Settings" button is partially below the gradient while using small screen like iPhone SE. This is probably the reason why testSignInOpenSettings and testSignInAndTapSettingsLink can be flaky. Also, updating those 2 tests to make it more robust to animation by adding calls to WaitForMatcher(), to wait for "Settings" to be displayed. BUG=718023 Review-Url: https://codereview.chromium.org/2906313004 Cr-Commit-Position: refs/heads/master@{#476691} Committed: https://chromium.googlesource.com/chromium/src/+/b87d80f6485276511b3c543e90cd... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/b87d80f6485276511b3c543e90cd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
