Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2637113004:
Wait for scroll offset in testScrollPositionRestoring. (Closed)
|
Created:
3 years, 11 months ago by kkhorimoto Modified:
3 years, 11 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWait for scroll offset in testScrollPositionRestoring.
This ensures that the content offset is applied before attempting
to verify using the contentOffset() matcher.
BUG=670700
Review-Url: https://codereview.chromium.org/2637113004
Cr-Commit-Position: refs/heads/master@{#444433}
Committed: https://chromium.googlesource.com/chromium/src/+/c91d38d7912b0076bce36e6ad318c197cd6d5a0a
Patch Set 1 #Patch Set 2 : use condition in egtest #
Total comments: 4
Patch Set 3 : remove extra checks #Patch Set 4 : fix compile #Messages
Total messages: 21 (10 generated)
kkhorimoto@chromium.org changed reviewers: + eugenebut@chromium.org
kkhorimoto@chromium.org changed reviewers: + baxley@chromium.org
I think we should avoid adding APIs which exist only for testing purpose, unless there are no viable alternatives. Also EG tests are whitebox tests and if we need to access some internal state we could do that using helpers or matchers, there is no need to rewrite test as WebIntTest. With that I think we should try to do these things: 1.) Keep test as EG test (WebIntTest does not make accessing internals easier) 2.) Try to get rid of displayStateApplied and rely on scroll state Could you please explain why you need displayStateApplied. Why waiting for specific scroll state does not work and we need this boolean?
On 2017/01/18 06:21:58, Eugene But wrote: > I think we should avoid adding APIs which exist only for testing purpose, unless > there are no viable alternatives. Also EG tests are whitebox tests and if we > need to access some internal state we could do that using helpers or matchers, > there is no need to rewrite test as WebIntTest. > > With that I think we should try to do these things: > 1.) Keep test as EG test (WebIntTest does not make accessing internals easier) > 2.) Try to get rid of displayStateApplied and rely on scroll state > > Could you please explain why you need displayStateApplied. Why waiting for > specific scroll state does not work and we need this boolean? Oh, I guess I know why the test fails. On line page_state_egtest.mm:90 this test does not wait until page is scrolled, but immediately asserts. Can we add a method which waits for specific scroll offset (similar to -[ChromeEarlGrey waitForPageToFinishLoading]) and use it instead of the matcher?
Description was changed from ========== Converted testScrollPositionRestoring EG test to inttest. The fix required inspecting the TestingOnly state of the web controller, which is not possible in the EG target. BUG=670700 ========== to ========== Wait for scroll offset in testScrollPositionRestoring. This ensures that the content offset is applied before attempting to verify using the contentOffset() matcher. BUG=670700 ==========
Yeah, the extra API was added because the check was flakily occurring before the display state could be applied. Your suggestion of waiting for the content offset rather than waiting for display state application also works, so I've updated to that approach.
Cool. lgtm https://codereview.chromium.org/2637113004/diff/20001/ios/web/shell/test/page... File ios/web/shell/test/page_state_egtest.mm (right): https://codereview.chromium.org/2637113004/diff/20001/ios/web/shell/test/page... ios/web/shell/test/page_state_egtest.mm:110: [[EarlGrey selectElementWithMatcher:web::webViewScrollView()] Do you still need this? https://codereview.chromium.org/2637113004/diff/20001/ios/web/shell/test/page... ios/web/shell/test/page_state_egtest.mm:117: [[EarlGrey selectElementWithMatcher:web::webViewScrollView()] ditto
https://codereview.chromium.org/2637113004/diff/20001/ios/web/shell/test/page... File ios/web/shell/test/page_state_egtest.mm (right): https://codereview.chromium.org/2637113004/diff/20001/ios/web/shell/test/page... ios/web/shell/test/page_state_egtest.mm:110: [[EarlGrey selectElementWithMatcher:web::webViewScrollView()] On 2017/01/18 18:36:02, Eugene But wrote: > Do you still need this? Nope https://codereview.chromium.org/2637113004/diff/20001/ios/web/shell/test/page... ios/web/shell/test/page_state_egtest.mm:117: [[EarlGrey selectElementWithMatcher:web::webViewScrollView()] On 2017/01/18 18:36:01, Eugene But wrote: > ditto Done.
The CQ bit was checked by kkhorimoto@chromium.org
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/2637113004/#ps40001 (title: "remove extra checks")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by kkhorimoto@chromium.org
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/2637113004/#ps60001 (title: "fix compile")
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": 60001, "attempt_start_ts": 1484765836992840,
"parent_rev": "2226adf3f8999d114147ffcfc79089cc2e136076", "commit_rev":
"c91d38d7912b0076bce36e6ad318c197cd6d5a0a"}
Message was sent while issue was closed.
Description was changed from ========== Wait for scroll offset in testScrollPositionRestoring. This ensures that the content offset is applied before attempting to verify using the contentOffset() matcher. BUG=670700 ========== to ========== Wait for scroll offset in testScrollPositionRestoring. This ensures that the content offset is applied before attempting to verify using the contentOffset() matcher. BUG=670700 Review-Url: https://codereview.chromium.org/2637113004 Cr-Commit-Position: refs/heads/master@{#444433} Committed: https://chromium.googlesource.com/chromium/src/+/c91d38d7912b0076bce36e6ad318... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/c91d38d7912b0076bce36e6ad318... |
