Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748873002:
Make testContextMenuOpenInNewTabFromTallPage less flaky. (Closed)
|
Created:
3 years, 9 months ago by baxley Modified:
3 years, 6 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake testContextMenuOpenInNewTabFromTallPage less flaky.
When this test fails, it appears that it didn't scroll all the way
to the bottom, like the toolbar disappeared after scrolling. This
adds an extra scroll to remove the toolbar.
BUG=701104
Patch Set 1 #Patch Set 2 : fix blank lines #
Total comments: 4
Messages
Total messages: 11 (1 generated)
baxley@chromium.org changed reviewers: + eugenebut@chromium.org
I was able to repro reliably locally in <100 runs. With this fix, I was able to run the test 200+ times with no failures.
https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/context_menu/context_menu_egtest.mm:215: performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; Instead of scrolling twice should we keep scrolling until hit the bottom?
https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/context_menu/context_menu_egtest.mm:215: performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; On 2017/03/13 22:35:01, Eugene But wrote: > Instead of scrolling twice should we keep scrolling until hit the bottom? This API does (or should) scroll to the bottom. I'll debug a little more. When it failed, it looked like there was about one toolbar's worth of content to scroll more to go. I am wondering if the timing of when the toolbar goes away is messing with the calculation EG is using to find the bottom.
https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/context_menu/context_menu_egtest.mm:215: performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; On 2017/03/13 22:43:41, baxley wrote: > On 2017/03/13 22:35:01, Eugene But wrote: > > Instead of scrolling twice should we keep scrolling until hit the bottom? > > This API does (or should) scroll to the bottom. I'll debug a little more. When > it failed, it looked like there was about one toolbar's worth of content to > scroll more to go. I am wondering if the timing of when the toolbar goes away is > messing with the calculation EG is using to find the bottom. Maybe this is because WebView's height has changed after the scrolling has started?
https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... ios/chrome/browser/context_menu/context_menu_egtest.mm:215: performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; On 2017/03/13 22:54:28, Eugene But wrote: > On 2017/03/13 22:43:41, baxley wrote: > > On 2017/03/13 22:35:01, Eugene But wrote: > > > Instead of scrolling twice should we keep scrolling until hit the bottom? > > > > This API does (or should) scroll to the bottom. I'll debug a little more. When > > it failed, it looked like there was about one toolbar's worth of content to > > scroll more to go. I am wondering if the timing of when the toolbar goes away > is > > messing with the calculation EG is using to find the bottom. > Maybe this is because WebView's height has changed after the scrolling has > started? Yes, that's my guess. This method is used in fullscreen_egtest (and maybe other places) to hide the toolbar, maybe I'll make a helper for it. It seems relatively reliable. What do you think if I make the following changes? - add a call to [ChromeEarlGreyUI waitForToolbarVisible:NO] after the first swipe. - Create a helper (swipeWebViewToHideToolbar), and then call that in place of the first swipe. Even if it's calling the same EG method this will make it more readable, and it will also allow us to come up with a better solution or change implementation of how we hide the toolbar.
On 2017/03/14 23:32:10, baxley wrote: > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > ios/chrome/browser/context_menu/context_menu_egtest.mm:215: > performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; > On 2017/03/13 22:54:28, Eugene But wrote: > > On 2017/03/13 22:43:41, baxley wrote: > > > On 2017/03/13 22:35:01, Eugene But wrote: > > > > Instead of scrolling twice should we keep scrolling until hit the bottom? > > > > > > This API does (or should) scroll to the bottom. I'll debug a little more. > When > > > it failed, it looked like there was about one toolbar's worth of content to > > > scroll more to go. I am wondering if the timing of when the toolbar goes > away > > is > > > messing with the calculation EG is using to find the bottom. > > Maybe this is because WebView's height has changed after the scrolling has > > started? > > Yes, that's my guess. This method is used in fullscreen_egtest (and maybe other > places) to hide the toolbar, maybe I'll make a helper for it. It seems > relatively reliable. > > What do you think if I make the following changes? > - add a call to [ChromeEarlGreyUI waitForToolbarVisible:NO] after the first > swipe. > - Create a helper (swipeWebViewToHideToolbar), and then call that in place of > the first swipe. Even if it's calling the same EG method this will make it more > readable, and it will also allow us to come up with a better solution or change > implementation of how we hide the toolbar. a helper like this... https://codereview.chromium.org/2752583004
On 2017/03/14 23:32:10, baxley wrote: > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > ios/chrome/browser/context_menu/context_menu_egtest.mm:215: > performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; > On 2017/03/13 22:54:28, Eugene But wrote: > > On 2017/03/13 22:43:41, baxley wrote: > > > On 2017/03/13 22:35:01, Eugene But wrote: > > > > Instead of scrolling twice should we keep scrolling until hit the bottom? > > > > > > This API does (or should) scroll to the bottom. I'll debug a little more. > When > > > it failed, it looked like there was about one toolbar's worth of content to > > > scroll more to go. I am wondering if the timing of when the toolbar goes > away > > is > > > messing with the calculation EG is using to find the bottom. > > Maybe this is because WebView's height has changed after the scrolling has > > started? > > Yes, that's my guess. This method is used in fullscreen_egtest (and maybe other > places) to hide the toolbar, maybe I'll make a helper for it. It seems > relatively reliable. > > What do you think if I make the following changes? > - add a call to [ChromeEarlGreyUI waitForToolbarVisible:NO] after the first > swipe. > - Create a helper (swipeWebViewToHideToolbar), and then call that in place of > the first swipe. Even if it's calling the same EG method this will make it more > readable, and it will also allow us to come up with a better solution or change > implementation of how we hide the toolbar. I don't like swiping twice because it's just happen to be enough in this specific case. EG API contract does not guarantee that |grey_swipeFastInDirection| scrolls all the way to the top or bottom, so we probably should not make any assumptions about it's implementation. Do you want to add a helper which guarantees to scroll to the bottom as a part of its API contract (and will swipe as many times as needed)? According to cl/2752583004, |swipeWebViewToHideToolbar| does not provide any guarantees.
On 2017/03/15 15:01:24, Eugene But wrote: > On 2017/03/14 23:32:10, baxley wrote: > > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > > File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): > > > > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > > ios/chrome/browser/context_menu/context_menu_egtest.mm:215: > > performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; > > On 2017/03/13 22:54:28, Eugene But wrote: > > > On 2017/03/13 22:43:41, baxley wrote: > > > > On 2017/03/13 22:35:01, Eugene But wrote: > > > > > Instead of scrolling twice should we keep scrolling until hit the > bottom? > > > > > > > > This API does (or should) scroll to the bottom. I'll debug a little more. > > When > > > > it failed, it looked like there was about one toolbar's worth of content > to > > > > scroll more to go. I am wondering if the timing of when the toolbar goes > > away > > > is > > > > messing with the calculation EG is using to find the bottom. > > > Maybe this is because WebView's height has changed after the scrolling has > > > started? > > > > Yes, that's my guess. This method is used in fullscreen_egtest (and maybe > other > > places) to hide the toolbar, maybe I'll make a helper for it. It seems > > relatively reliable. > > > > What do you think if I make the following changes? > > - add a call to [ChromeEarlGreyUI waitForToolbarVisible:NO] after the first > > swipe. > > - Create a helper (swipeWebViewToHideToolbar), and then call that in place of > > the first swipe. Even if it's calling the same EG method this will make it > more > > readable, and it will also allow us to come up with a better solution or > change > > implementation of how we hide the toolbar. > I don't like swiping twice because it's just happen to be enough in this > specific case. EG API contract does not guarantee that > |grey_swipeFastInDirection| scrolls all the way to the top or bottom, so we > probably should not make any assumptions about it's implementation. You don't think "Returns an action that fast swipes through the whole view." implies that it would make it to the edge? I'll follow up with the EG folks before I add a helper for something, which may be what they intended to achieve. > > Do you want to add a helper which guarantees to scroll to the bottom as a part > of its API contract (and will swipe as many times as needed)? According to > cl/2752583004, |swipeWebViewToHideToolbar| does not provide any guarantees. I'd rather talk to the EG team first, since it may be something they want to handle, and if so it makes more sense in their code. I'll mark this test as flaky for now.
On 2017/03/15 15:17:37, baxley wrote: > On 2017/03/15 15:01:24, Eugene But wrote: > > On 2017/03/14 23:32:10, baxley wrote: > > > > > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > > > File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): > > > > > > > > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > > > ios/chrome/browser/context_menu/context_menu_egtest.mm:215: > > > performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; > > > On 2017/03/13 22:54:28, Eugene But wrote: > > > > On 2017/03/13 22:43:41, baxley wrote: > > > > > On 2017/03/13 22:35:01, Eugene But wrote: > > > > > > Instead of scrolling twice should we keep scrolling until hit the > > bottom? > > > > > > > > > > This API does (or should) scroll to the bottom. I'll debug a little > more. > > > When > > > > > it failed, it looked like there was about one toolbar's worth of content > > to > > > > > scroll more to go. I am wondering if the timing of when the toolbar goes > > > away > > > > is > > > > > messing with the calculation EG is using to find the bottom. > > > > Maybe this is because WebView's height has changed after the scrolling has > > > > started? > > > > > > Yes, that's my guess. This method is used in fullscreen_egtest (and maybe > > other > > > places) to hide the toolbar, maybe I'll make a helper for it. It seems > > > relatively reliable. > > > > > > What do you think if I make the following changes? > > > - add a call to [ChromeEarlGreyUI waitForToolbarVisible:NO] after the first > > > swipe. > > > - Create a helper (swipeWebViewToHideToolbar), and then call that in place > of > > > the first swipe. Even if it's calling the same EG method this will make it > > more > > > readable, and it will also allow us to come up with a better solution or > > change > > > implementation of how we hide the toolbar. > > I don't like swiping twice because it's just happen to be enough in this > > specific case. EG API contract does not guarantee that > > |grey_swipeFastInDirection| scrolls all the way to the top or bottom, so we > > probably should not make any assumptions about it's implementation. > You don't think "Returns an action that fast swipes through the whole view." > implies that it would make it to the edge? I'll follow up with the EG folks > before I add a helper for something, which may be what they intended to achieve. Maybe... "swipes through the whole view" could mean that it scrolls to the edge :) If EG API suppose to provide that guarantee, then they have a bug.
On 2017/03/15 15:26:00, Eugene But wrote: > On 2017/03/15 15:17:37, baxley wrote: > > On 2017/03/15 15:01:24, Eugene But wrote: > > > On 2017/03/14 23:32:10, baxley wrote: > > > > > > > > > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > > > > File ios/chrome/browser/context_menu/context_menu_egtest.mm (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2748873002/diff/20001/ios/chrome/browser/cont... > > > > ios/chrome/browser/context_menu/context_menu_egtest.mm:215: > > > > performAction:grey_swipeFastInDirection(kGREYDirectionUp)]; > > > > On 2017/03/13 22:54:28, Eugene But wrote: > > > > > On 2017/03/13 22:43:41, baxley wrote: > > > > > > On 2017/03/13 22:35:01, Eugene But wrote: > > > > > > > Instead of scrolling twice should we keep scrolling until hit the > > > bottom? > > > > > > > > > > > > This API does (or should) scroll to the bottom. I'll debug a little > > more. > > > > When > > > > > > it failed, it looked like there was about one toolbar's worth of > content > > > to > > > > > > scroll more to go. I am wondering if the timing of when the toolbar > goes > > > > away > > > > > is > > > > > > messing with the calculation EG is using to find the bottom. > > > > > Maybe this is because WebView's height has changed after the scrolling > has > > > > > started? > > > > > > > > Yes, that's my guess. This method is used in fullscreen_egtest (and maybe > > > other > > > > places) to hide the toolbar, maybe I'll make a helper for it. It seems > > > > relatively reliable. > > > > > > > > What do you think if I make the following changes? > > > > - add a call to [ChromeEarlGreyUI waitForToolbarVisible:NO] after the > first > > > > swipe. > > > > - Create a helper (swipeWebViewToHideToolbar), and then call that in place > > of > > > > the first swipe. Even if it's calling the same EG method this will make it > > > more > > > > readable, and it will also allow us to come up with a better solution or > > > change > > > > implementation of how we hide the toolbar. > > > I don't like swiping twice because it's just happen to be enough in this > > > specific case. EG API contract does not guarantee that > > > |grey_swipeFastInDirection| scrolls all the way to the top or bottom, so we > > > probably should not make any assumptions about it's implementation. > > You don't think "Returns an action that fast swipes through the whole view." > > implies that it would make it to the edge? I'll follow up with the EG folks > > before I add a helper for something, which may be what they intended to > achieve. > Maybe... "swipes through the whole view" could mean that it scrolls to the edge > :) > If EG API suppose to provide that guarantee, then they have a bug. Yup, which is why I'm starting with them... https://github.com/google/EarlGrey/issues/448 |
