Closed Bug 1443652 Opened 2 years ago Closed 2 years ago

Port tests from bug 1443344 and bug 1442126 to wpt

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(2 files)

This will port the <link> tests, at least.
The test was adding the load listener to the <link> _after_ the load event on
the <link> had already fired.

MozReview-Commit-ID: JAS94H9SNOo
Attachment #8956626 - Flags: review?(bobbyholley)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
MozReview-Commit-ID: 1lieHmpuRtH
Attachment #8956628 - Flags: review?(bobbyholley)
Attachment #8956626 - Flags: review?(bobbyholley) → review+
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?
Attachment #8956628 - Flags: review?(bobbyholley) → review+
> 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...
Flags: needinfo?(bobbyholley)
(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?
Flags: needinfo?(bobbyholley)
> Comment to that effect, and explain why we don't use a global counter?

Will do.

> Ok, comment to that effect?

Also will do.
Pushed by bzbarsky@mozilla.com:
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
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb093a16f939
followup.  Update the wpt manifest a bit.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/919f86518ed3
another followup.  Remove wpt .ini file that I thought I had removed.
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10218 for changes under testing/web-platform/tests
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!
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.
I see. In that case, removing the URL from the messages would be great :)
You need to log in before you can comment on or make changes to this bug.