Add preload tests from Blink. #4316

yoavweiss · 2016-12-12T15:33:47Z

This PR adds tests for preload from Blink's layout tests.

These tests are not necessarily a complete test suite as they were built during the feature's implementation, and complemented in Blink by various tests that require internal APIs as well as unit tests. I plan to iterate over these tests and make sure they provide adequate coverage.

/cc @zcorpan @igrigorik

zcorpan · 2016-12-12T20:29:06Z

preload/avoid_delaying_onload_link_preload.html

+<script>
+    var t = async_test('Makes sure link preload preloaded resources are not delaying onload');
+</script>
+<link rel=preload href="resources/dummy.js?pipe=trickle(d1)" as=script>


This seems like a possible source of unstable results. Maybe something like 5 seconds? @jgraham

Do you think the time it'd take the browser to run the test and get to window.load() would exceed 1 second? I'd hate to make this test slower if it's not a must

@jgraham can comment on this more but AIUI the experience when testing (on devices in particular) is that there can be slowness in the network stack for various reasons, and assuming that network will have responded in less than 2 seconds has resulted in unstable test results. (This applies to all trickle()/timeouts in this PR, not just this line.)

OK, bumped up the trickle values

zcorpan · 2016-12-12T20:31:30Z

preload/single_download_late_used_preload.html

+<link rel=preload href="resources/square.png" as=image>
+<script>
+    window.addEventListener("load", t.step_func(function() {
+        setTimeout(t.step_func(function() {


Use t.step_timeout and a longer timeout (2000ms I think)

igrigorik · 2016-12-14T05:15:08Z

preload/delaying_onload_link_preload_after_discovery.html

+        t.done();
+    }));
+    document.write('<sc' + 'ript src="resources/dummy.js?pipe=trickle(d1)"></sc' + 'ript>');
+    document.write("<!--");


What's the point/purpose of the unclosed comment? Would it be simpler and cleaner to simply build and append a script element?

yeah, it would be. I'll switch to that.

igrigorik · 2016-12-14T05:18:12Z

preload/dynamic_remove_preload_href.html

+    link.href = "";
+    window.addEventListener("load", t.step_func(function() {
+        assert_equals(performance.getEntriesByType("resource").length, 3);
+        t.done();


Not sure I follow this test. How does this verify that the preload response is aborted? Meta: does abort even make sense? The browser should start a new download if href is mutated, but it's not clear that the old one can/should be stopped.

It verifies that by seeing 3 resources in RT rather than 4. I could make it explicit looking at a particular URL.

I was following the spec's processing model: "The user agent should abort the current request if the href attribute of the link element of a preload link is changed, removed, or its value is set to an empty string." (which IMO makes sense)

igrigorik · 2016-12-14T05:18:37Z

preload/dynamic_removing_preload.html

+        t.done();
+    }));
+</script>
+<script src="resources/dummy.js?pipe=trickle(d0.2)"></script>


Ditto to above.

Hmm, unlike the above, that behavior is actually not defined. There's no mutation when link is removed from the document. Should we define it as such? (I'd argue that it should be similar to href being removed or set to an empty string)

rel removal is also not defined as a mutation or something the UA should abort on, and probably should be.

igrigorik · 2016-12-14T05:20:21Z

preload/onerror_event.html

+<link rel=preload href="non-existent/test.mp4" as=video onerror="videoFailed = true;">
+<link rel=preload href="non-existent/test.oga" as=audio onerror="audioFailed = true;">
+<link rel=preload href="non-existent/security/captions.vtt" as=media onerror="trackFailed = true;">
+<link rel=preload href="non-existent/dummy.xml" as=foobarxmlthing onerror="gibrishFailed = true;">


gibberish? :P

didn't bother to typo check that word, apparently :D

nicjansma · 2016-12-14T18:50:46Z

preload/single_download_preload.html

+        t.done();
+    }));
+</script>
+<span>PASS - this text is here just so that the browser will download the font.</span


Missing >!

Good catch! :)

zcorpan · 2016-12-16T08:25:59Z

#4316 (comment) is still outstanding comment. (Making tests take longer to run is bad, but having people spend time analyzing unstable results is worse...)

yoavweiss · 2016-12-16T10:42:16Z

Changed the trickle values to higher ones. I'll try to see in follow up PRs if I can somehow collapse the tests, so that we'd wait on these long timeouts once, rather than multiple times...

zcorpan · 2016-12-16T11:10:35Z

Thanks!

Checking test results in https://travis-ci.org/w3c/web-platform-tests/builds/184496213 ...

/preload/preload_with_type.html fails in Chrome. Is that expected?

/preload/dynamic_adding_preload.html times out in Firefox. Could it be made to fail reliably (maybe fail early with a feature-check?)?

yoavweiss · 2016-12-16T11:17:13Z

The failure in Chrome is expected (Noticed it while writing the tests). I can fix it first before landing the tests, if that helps. I'll look into the Firefox timeouts

zcorpan · 2016-12-16T11:33:44Z

It's perfectly fine to land tests that fail, just wanted to check that it was expected.

zcorpan · 2016-12-16T11:47:27Z

preload/dynamic_adding_preload.html

@@ -7,13 +7,17 @@
 <body>
 <script>
    var link = document.createElement("link");
+    if(!link.rellist || !link.rellist.supports("preload")) {


Should be relList

zcorpan · 2016-12-16T11:48:27Z

preload/dynamic_adding_preload.html

    link.as = "script";
    link.rel = "preload";
    link.href = "resources/dummy.js";
-    link.onload = t.step_func(function() {
+    link.onload = t.step_timeout(function() {


How does this make sense?

There's no spec guaranty that the PerformanceEntry is added to the timeline before the resource's onload handler runs

on second thought, you're right. I'll rewrite

zcorpan · 2016-12-16T11:49:19Z

preload/dynamic_adding_preload.html

@@ -7,13 +7,17 @@
 <body>
 <script>
    var link = document.createElement("link");
+    if(!link.rellist || !link.rellist.supports("preload")) {
+        assert_unreached("rellist is not defined or browser doesn't support preload.");
+        t.done();


No need for this line, the line above throws an exception.

yoavweiss · 2016-12-16T13:36:55Z

@zcorpan: addressed your comments

zcorpan · 2016-12-16T14:33:07Z

w3c-test:mirror

zcorpan · 2016-12-16T14:36:24Z

preload/dynamic_adding_preload.html

+</script>
+<body>
+<script>
+    var link = document.createElement("link");


Wrap everything in t.step(function() { ... }) (or use async_test(function(t) { ... }))

yoavweiss · 2016-12-19T08:45:41Z

Thanks @igrigorik and @zcorpan for reviewing! (and @sideshowbarker for merging :D)

Add preload tests from Blink.

f361d19

wpt-pr-bot added wg-webperf preload labels Dec 12, 2016

lint errors

3089bb5

yoavweiss mentioned this pull request Dec 12, 2016

Transition Level 1 to CR w3c/preload#76

Closed

7 tasks

zcorpan reviewed Dec 12, 2016

View reviewed changes

yoavweiss added 3 commits December 12, 2016 23:13

fix CSP test

e5e0e2d

Add default-src CSP test

16af548

Changed timeouts based on review comments

40606e6

igrigorik reviewed Dec 14, 2016

View reviewed changes

yoavweiss mentioned this pull request Dec 14, 2016

Define resource abortion when link element or rel are removed w3c/preload#82

Closed

Review comments from igrigorik

6a6c183

nicjansma reviewed Dec 14, 2016

View reviewed changes

Added missing bracket

55cfe04

igrigorik approved these changes Dec 16, 2016

View reviewed changes

Higher trickle values

38012c6

yoavweiss added 2 commits December 16, 2016 12:35

Eliminate timeout

f672fc8

Fix feature detection

1c22918

zcorpan requested changes Dec 16, 2016

View reviewed changes

yoavweiss added 2 commits December 16, 2016 14:34

Fixed failing tests

baa2a97

Removed t.done()

ce67a3e

lint and s/track/media/

8e2b844

zcorpan reviewed Dec 16, 2016

View reviewed changes

yoavweiss and others added 3 commits December 16, 2016 15:43

feature test in its own test step

14948cc

wrapped entire dynamic adding test in a step

1d65917

Fix some step() that should be step_func()

9ca024f

zcorpan approved these changes Dec 16, 2016

View reviewed changes

sideshowbarker merged commit e8172dd into web-platform-tests:master Dec 19, 2016

yoavweiss deleted the preload_tests branch December 19, 2016 08:45

yoavweiss mentioned this pull request Aug 16, 2017

Aborting a fetch for a link preload request if element is removed or changed whatwg/html#2931

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preload tests from Blink. #4316

Add preload tests from Blink. #4316

yoavweiss commented Dec 12, 2016

zcorpan Dec 12, 2016

yoavweiss Dec 13, 2016

zcorpan Dec 14, 2016

yoavweiss Dec 16, 2016

zcorpan Dec 12, 2016

igrigorik Dec 14, 2016

yoavweiss Dec 14, 2016

igrigorik Dec 14, 2016

yoavweiss Dec 14, 2016

igrigorik Dec 14, 2016

yoavweiss Dec 14, 2016

igrigorik Dec 14, 2016

yoavweiss Dec 14, 2016

nicjansma Dec 14, 2016

yoavweiss Dec 16, 2016

zcorpan commented Dec 16, 2016

yoavweiss commented Dec 16, 2016

zcorpan commented Dec 16, 2016

yoavweiss commented Dec 16, 2016

zcorpan commented Dec 16, 2016

zcorpan Dec 16, 2016

yoavweiss Dec 16, 2016

zcorpan Dec 16, 2016

yoavweiss Dec 16, 2016

yoavweiss Dec 16, 2016

zcorpan Dec 16, 2016

yoavweiss Dec 16, 2016

yoavweiss commented Dec 16, 2016

zcorpan commented Dec 16, 2016

zcorpan Dec 16, 2016

yoavweiss Dec 16, 2016

yoavweiss commented Dec 19, 2016

Add preload tests from Blink. #4316

Add preload tests from Blink. #4316

Conversation

yoavweiss commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcorpan commented Dec 16, 2016

yoavweiss commented Dec 16, 2016

zcorpan commented Dec 16, 2016

yoavweiss commented Dec 16, 2016

zcorpan commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss commented Dec 16, 2016

zcorpan commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoavweiss commented Dec 19, 2016