Fixed PageStateTest.testScrollPositionRestoring flake.

ios/web/shell/test/page_state_egtest.mm

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import <CoreGraphics/CoreGraphics.h>
 #import <EarlGrey/EarlGrey.h>
 #import <Foundation/Foundation.h>
 #import <XCTest/XCTest.h>
 
 #include "base/ios/ios_util.h"
@@ skipping 35 matching lines @@
                         assertWithMatcher:grey_scrollViewContentOffset(offset)
                                     error:&error];
                     return (error == nil);
                   }];
   NSString* error_text =
       [NSString stringWithFormat:@"Scroll view did not scroll to %@",
                                  content_offset_string];
   GREYAssert([condition waitWithTimeout:10], error_text);
 }
 
+// Loads the long page at |url|, scrolls to the top, and waits for the offset to
+// be {0, 0} before returning.
+void LoadLongPageAndScrollToTop(const GURL& url) {
+  // Load the page and swipe down.
+  [ShellEarlGrey loadURL:url];
+  [[EarlGrey selectElementWithMatcher:web::WebViewScrollView()]
+      performAction:grey_swipeFastInDirection(kGREYDirectionDown)];

[baxley, 2017/04/06 13:02:39]

Can you use grey_scrollToContentEdge instead? grey_swipeFastInDirection doesn't
guarantee to reach the content edge, which has bitten us before. Unfortunately
that method is much slower If it's a long distance you need to scroll up. You
can leave the method call you have and then do a second call to
grey_scrollToContentEdge.

[Eugene But (OOO till 7-30), 2017/04/06 15:28:46]

Why do we need to scroll the page to the top? If the page is loaded with
non-zero offset it sounds like a product bug, not a feature.

[kkhorimoto, 2017/04/06 23:48:10]

I've updated to use scrollToContentEdge.

I agree that it's a bug if the page is scrolled down upon loading.  I tried to
investigate, but I haven't been able to reproduce, even when running the test
1000 times.  I've created a bug (crbug.com/709278) to capture this; can you
populate it with any details you have?  You mentioned that there were other
reports of this issue.  This test is about page scroll state restoration, so
it's a different issue than a non CGPointZero contentOffset on load.

I should also note that these tests have historically been flakier because
there's no API to tell when the page has finished rendering; adding steps to
ensure the expected state before testing the actual restoration functionality
seems like a good option here.

[Eugene But (OOO till 7-30), 2017/04/07 00:45:49]

Does this test actually fail on Flaky bots? Should we update
|waitForPageToFinishLoading| to assert that page is not scrolled after the load?
Also we can add a temporary DCHECK to webView:didFinishNavigation: to assert
that page is not scrolled.

[kkhorimoto, 2017/04/12 05:03:28]

I've investigated, and it turns out that that's not always the case; the content
offset is updated by webkit after the didFinishNavigation: call :(

+  // Waits for the {0, 0} offset.
+  WaitForOffset(0.0);

[baxley, 2017/04/06 13:02:38]

This may not be necessary if we do the call to grey_scrollToContentEdge. But I
won't object to having this second check if this helps make sure it passes, or
at least fail here so we know what the problem is.

[kkhorimoto, 2017/04/06 23:48:10]

I left it in since at best it will help reduce flake, and at worst it'll be a
no-op.

+}
+
 }  // namespace
 
 using web::test::HttpServer;
 
 // Page state test cases for the web shell.
 @interface PageStateTestCase : WebShellTestCase
 @end
 
 @implementation PageStateTestCase
 
 // Tests that page scroll position of a page is restored upon returning to the
 // page via the back/forward buttons.
-// TODO(crbug.com/702410): This test flakily fails when it scrolls too far down
-// the page.
-- (void)FLAKY_testScrollPositionRestoring {
+- (void)testScrollPositionRestoring {
   web::test::SetUpFileBasedHttpServer();
 
   // Load first URL which is a long page.

[baxley, 2017/04/06 13:02:39]

Do you want to delete this comment? I think the new method call makes it clear.

[kkhorimoto, 2017/04/06 23:48:10]

Done.

-  [ShellEarlGrey loadURL:HttpServer::MakeUrl(kLongPage1)];
+  LoadLongPageAndScrollToTop(HttpServer::MakeUrl(kLongPage1));
 
   // Scroll the first page and verify the offset.
   [[EarlGrey selectElementWithMatcher:web::WebViewScrollView()]
       performAction:grey_scrollInDirection(kGREYDirectionDown, kOffset1)];
   [[EarlGrey selectElementWithMatcher:web::WebViewScrollView()]
       assertWithMatcher:grey_scrollViewContentOffset(CGPointMake(0, kOffset1))];
 
   // Load second URL, which is also a long page.

[baxley, 2017/04/06 13:02:39]

Delete comment?

[kkhorimoto, 2017/04/06 23:48:10]

Done.

-  [ShellEarlGrey loadURL:HttpServer::MakeUrl(kLongPage2)];
+  LoadLongPageAndScrollToTop(HttpServer::MakeUrl(kLongPage2));
 
   // Scroll the second page and verify the offset.
   [[EarlGrey selectElementWithMatcher:web::WebViewScrollView()]
       performAction:grey_scrollInDirection(kGREYDirectionDown, kOffset2)];
   [[EarlGrey selectElementWithMatcher:web::WebViewScrollView()]
       assertWithMatcher:grey_scrollViewContentOffset(CGPointMake(0, kOffset2))];
 
   // Go back and verify that the first page offset has been restored.
   [[EarlGrey selectElementWithMatcher:web::BackButton()]
       performAction:grey_tap()];
   WaitForOffset(kOffset1);
 
   // Go forward and verify that the second page offset has been restored.
   [[EarlGrey selectElementWithMatcher:web::ForwardButton()]
       performAction:grey_tap()];
   WaitForOffset(kOffset2);
 }
 
 @end