Bug 142358 - Create a set of scroll snap point tests
Summary: Create a set of scroll snap point tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on: 144482
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-05 12:48 PST by Brent Fulgham
Modified: 2015-05-04 12:52 PDT (History)
4 users (show)

See Also:


Attachments
Patch (17.31 KB, patch)
2015-03-05 21:07 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (17.16 KB, patch)
2015-03-05 21:12 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (530.41 KB, application/zip)
2015-03-05 21:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (679.62 KB, application/zip)
2015-03-05 22:05 PST, Build Bot
no flags Details
Patch to see what testers are encountering. (17.38 KB, patch)
2015-03-05 23:02 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (737.85 KB, application/zip)
2015-03-05 23:45 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-mavericks (549.11 KB, application/zip)
2015-03-05 23:48 PST, Build Bot
no flags Details
Patch (16.71 KB, patch)
2015-03-06 09:19 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch v3 - resolve flakiness. (30.21 KB, patch)
2015-05-01 18:27 PDT, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-03-05 12:48:59 PST
Add layout tests for scroll snap animations, now that they are working properly for the "Mandatory" scroll point case.
Comment 1 Brent Fulgham 2015-03-05 21:00:06 PST
This set of tests exercises the 'slow' code path. Changes will be needed in WebKitTestRunner to support the scrolling-thread code path.
Comment 2 Brent Fulgham 2015-03-05 21:07:15 PST
Created attachment 248036 [details]
Patch
Comment 3 Brent Fulgham 2015-03-05 21:12:22 PST
Created attachment 248037 [details]
Patch
Comment 4 Build Bot 2015-03-05 21:59:54 PST
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
Comment 5 Build Bot 2015-03-05 21:59:58 PST
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 6 Build Bot 2015-03-05 22:05:42 PST
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
Comment 7 Build Bot 2015-03-05 22:05:45 PST
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
Comment 8 Brent Fulgham 2015-03-05 23:02:58 PST
Created attachment 248042 [details]
Patch to see what testers are encountering.
Comment 9 Darin Adler 2015-03-05 23:19:12 PST
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 10 Build Bot 2015-03-05 23:45:14 PST
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
Comment 11 Build Bot 2015-03-05 23:45:18 PST
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 12 Build Bot 2015-03-05 23:48:54 PST
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
Comment 13 Build Bot 2015-03-05 23:48:58 PST
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
Comment 14 Brent Fulgham 2015-03-06 09:19:32 PST
Created attachment 248068 [details]
Patch
Comment 15 Brent Fulgham 2015-03-06 09:32:30 PST
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.
Comment 16 Brent Fulgham 2015-05-01 17:56:12 PDT
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.
Comment 17 Brent Fulgham 2015-05-01 18:27:02 PDT
Created attachment 252211 [details]
Patch v3 - resolve flakiness.
Comment 18 Brent Fulgham 2015-05-01 18:31:28 PDT
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.
Comment 19 Brent Fulgham 2015-05-01 18:46:08 PDT
Committed r183707: <http://trac.webkit.org/changeset/183707>