1443652 - Port tests from bug 1443344 and bug 1442126 to wpt

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

This will port the <link> tests, at least.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Fix the buggy existing load-event test for stylesheets

The test was adding the load listener to the <link> _after_ the load event on
the <link> had already fired.

MozReview-Commit-ID: JAS94H9SNOo

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add a bunch of web platform tests for load and error events on stylesheet links

MozReview-Commit-ID: 1lieHmpuRtH

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8956628 [details] [diff] [review]
part 2.  Add a bunch of web platform tests for load and error events on stylesheet links

Review of attachment 8956628 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/semantics/document-metadata/the-link-element/resources/link-load-error-events.sub.js
@@ +3,5 @@
> + *
> + * We have a list of tests each of which is an object containing: href value,
> + * expected load success boolean, test description.  Href values are set up in
> + * such a way that we guarantee that all stylesheet URLs are unique.  This
> + * avoids issues around caching of sheets basd on URL.

based.

@@ +122,5 @@
> +  }
> +  link.href = href;
> +  document.head.appendChild(link);
> +}
> +  

Nit: whitespace error.

@@ +129,5 @@
> +function makeUnique(url) {
> +  // Make sure we copy here, even if the thing coming in is a URL, so we don't
> +  // mutate our caller's data.
> +  url = new URL(url, location.href);
> +  url.searchParams.append("r", Math.random());

Not a fan of the Math.random() here. Can you use a global counter instead?

@@ +137,5 @@
> +function existingSheet() {
> +  return makeUnique("resources/good.css");
> +}
> +
> +function addPipe(url, pipeVal) {

Erm, what's all this about, exactly?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> based.
> Nit: whitespace error.

> Can you use a global counter instead?

I could use a page-level counter, but it would not do the right thing when the test is reloaded (or e.g. run in the TV job), right?  I could do a truly global counter in the test server, but that would involve a lot more complexity...

> Erm, what's all this about, exactly?

See <http://wptserve.readthedocs.io/en/latest/pipes.html>.  I'm using two of the built-in pipes: http://wptserve.readthedocs.io/en/latest/pipes.html#status and <http://wptserve.readthedocs.io/en/latest/pipes.html#trickle>.  Because the pipe thing uses a weird microsyntax with '|' as separator instead of just multiple "pipe" paras, I had to add a helper to add pipes instead of just appending to the URLSearchParams...

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> > based.
> > Nit: whitespace error.
> 
> > Can you use a global counter instead?
> 
> I could use a page-level counter, but it would not do the right thing when
> the test is reloaded (or e.g. run in the TV job), right?  I could do a truly
> global counter in the test server, but that would involve a lot more
> complexity...

I guess the randomness is just to make sure the test doesn't hit the cache, and even if we had a collision somehow the test should still pass. Comment to that effect, and explain why we don't use a global counter?

> 
> > Erm, what's all this about, exactly?
> 
> See <http://wptserve.readthedocs.io/en/latest/pipes.html>.  I'm using two of
> the built-in pipes:
> http://wptserve.readthedocs.io/en/latest/pipes.html#status and
> <http://wptserve.readthedocs.io/en/latest/pipes.html#trickle>.  Because the
> pipe thing uses a weird microsyntax with '|' as separator instead of just
> multiple "pipe" paras, I had to add a helper to add pipes instead of just
> appending to the URLSearchParams...

Ok, comment to that effect?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Comment to that effect, and explain why we don't use a global counter?

Will do.

> Ok, comment to that effect?

Also will do.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba6cef216a17
part 1.  Fix the buggy existing load-event test for stylesheets.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/7daaf289c085
part 2.  Add a bunch of web platform tests for load and error events on stylesheet links.  r=bholley

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb093a16f939
followup.  Update the wpt manifest a bit.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/919f86518ed3
another followup.  Remove wpt .ini file that I thought I had removed.

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ba6cef216a17
https://hg.mozilla.org/mozilla-central/rev/7daaf289c085
https://hg.mozilla.org/mozilla-central/rev/eb093a16f939
https://hg.mozilla.org/mozilla-central/rev/919f86518ed3

Comment 11

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10218 for changes under testing/web-platform/tests

Comment 12

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 13

[Robert Ma [:Hexcles]]

Hi Boris, we notice link-load-error-events{.https}.html are flaky in Blink because Math.random is included in the URLs, which are included in the error messages.

Is there any way to avoid the use of Math.random, or avoid including the URLs in the assertion messages? Ideally, we would want to have stable messages even when the tests are failing. Thanks!

Comment 14

[Boris Zbarsky [:bzbarsky]]

We could certainly avoid using the URL in the error messages.  I added it there so it would be easier to see what's going on when the tests fail, but I can see how it can cause problems for adding failure annotations.

Avoiding the use of Math.random() is ... hard.  See earlier comments in this bug.

So it's probably best to just change the two assert_unreached to not include the href in the message, or only include it if a global DEBUG variable is set to true or something along those lines.

Comment 15

[Robert Ma [:Hexcles]]

I see. In that case, removing the URL from the messages would be great :)

Comment 16

[Boris Zbarsky [:bzbarsky]]

https://github.com/w3c/web-platform-tests/pull/10286