Summary: | Create a set of scroll snap point tests | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, buildbot, rniwa, simon.fraser | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=144591 | ||||||||||||||||||||||
Bug Depends on: | 144482 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2015-03-05 12:48:59 PST
This set of tests exercises the 'slow' code path. Changes will be needed in WebKitTestRunner to support the scrolling-thread code path. Created attachment 248036 [details]
Patch
Created attachment 248037 [details]
Patch
Comment on attachment 248037 [details] Patch Attachment 248037 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6208385754071040 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html Created attachment 248039 [details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248037 [details] Patch Attachment 248037 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6208172079448064 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html Created attachment 248040 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 248042 [details]
Patch to see what testers are encountering.
Comment on attachment 248042 [details] Patch to see what testers are encountering. View in context: https://bugs.webkit.org/attachment.cgi?id=248042&action=review I’m not sure we want to add tests with fixed 2+ second delays in them. We should discuss this. Otherwise I would have already said review+. > LayoutTests/ChangeLog:9 > + regions with sroll snap points defined. scroll > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> The HTML5 DOCTYPE is simpler than this: <!DOCTYPE html> I don’t see any reason to use the longer one; not really sure if we should use any at all. > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:21 > + /*-webkit-scroll-snap-coordinate: 50% 50%;*/ Why include commented out code? > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:24 > + background-color: #FF0000; red instead? > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:27 > + background-color: #00FF00; green > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:30 > + background-color: #0000FF; blue > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:33 > + background-color: #00FFFF; etc. > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:74 > + setTimeout(checkForScrollSnap, 1000); It’s not good to have a 1 second delay in a regression test. That means the test will take a full second to run! Also, we want the tests to run even on loaded machines, so dependencies on certain speeds aren’t great. > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:85 > + setTimeout(scrollSnapTest, 100); Another magic number, 100ms? > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:103 > + setTimeout(checkForScrollGlide, 1000); Another 1 second delay! > LayoutTests/css3/scroll-snap/scroll-snap-mandatory-vertical-expected.txt:10 > +PASS successfullyParsed is true > + > +TEST COMPLETE > +PASS div scrolled to next window. > +PASS div honored snap points. This is really messy output. Maybe we can clean it up somehow? Comment on attachment 248042 [details] Patch to see what testers are encountering. Attachment 248042 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5594101277786112 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html Created attachment 248046 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 248042 [details] Patch to see what testers are encountering. Attachment 248042 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6349123242426368 New failing tests: css3/scroll-snap/scroll-snap-mandatory-vertical.html css3/scroll-snap/scroll-snap-mandatory-horizontal.html Created attachment 248047 [details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 248068 [details]
Patch
Comment on attachment 248042 [details] Patch to see what testers are encountering. View in context: https://bugs.webkit.org/attachment.cgi?id=248042&action=review >> LayoutTests/ChangeLog:9 >> + regions with sroll snap points defined. > > scroll Whoops! >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:1 >> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > The HTML5 DOCTYPE is simpler than this: > > <!DOCTYPE html> > > I don’t see any reason to use the longer one; not really sure if we should use any at all. I'll switch to the short version; we seem to use it in most tests I've looked at (though it's probably not necessary). >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:74 >> + setTimeout(checkForScrollSnap, 1000); > > It’s not good to have a 1 second delay in a regression test. That means the test will take a full second to run! Also, we want the tests to run even on loaded machines, so dependencies on certain speeds aren’t great. What we need is some way to register a callback for when the event has completed processing. Unfortunately, this doesn't exist. The delay is based on the time the scroll snap animation takes to run, with enough extra time to account for some variation in machine loading. Do you have any ideas for how we can better model this in testing? >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:85 >> + setTimeout(scrollSnapTest, 100); > > Another magic number, 100ms? This one could probably be 0. I'll change that. >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-horizontal.html:103 >> + setTimeout(checkForScrollGlide, 1000); > > Another 1 second delay! See answer above. >> LayoutTests/css3/scroll-snap/scroll-snap-mandatory-vertical-expected.txt:10 >> +PASS div honored snap points. > > This is really messy output. Maybe we can clean it up somehow? Yes! When a test is asyncrhonous, it's apparently not enough to stick the "js-test-pos.js" script at the bottom of the file. Instead, you have to set "window.jsTestIsAsync = true;" at the start of the test, then call "finishJSTest();" at the end. Then the test output is correct. These patches are running great in isolation, but when run in conjunction with the normal scroll tests they start failing. There must be some state being retained from run-to-run. Created attachment 252211 [details]
Patch v3 - resolve flakiness.
I had been issuing the wheel event commands in async mode, which doesn't queues them up for use by the test runner. Unfortunately, since they return immediately we could reach the "set the test callback" step before any scroll activity had started. If no one flagged a reason not to run the test, the test would fire immediately, causing it to check state before the wheel events have caused anything to move around. Committed r183707: <http://trac.webkit.org/changeset/183707> |