Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multiple tests for prefetch behavior #35707

Merged
merged 10 commits into from
Jan 18, 2023
Merged

Add multiple tests for prefetch behavior #35707

merged 10 commits into from
Jan 18, 2023

Conversation

noamr
Copy link
Contributor

@noamr noamr commented Aug 30, 2022

See whatwg/html#8111 (comment)

Tests that:

  • preload/prefetch-cache.html
    No 5 minute rule (cache headers must be respected)

  • preload/prefetch-document.html
    No special treatment of as=document
    Whether Accept is left as its default value.
    Storage partitioning is applied, including for documents

  • preload/prefetch-load-event.html
    Does not block the load event

  • preload/prefetch-headers.html
    Sec-Purpose and Sec-Destination are correct

  • preload/prefetch-types.html
    Always uses prefetch destination, and ignores as=""

  • preload/prefetch-events.html
    Load & error events are fired, and invalid/empty hrefs are ignored.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great stuff.

Can we also test (or maybe tests already exist?) the behavior for empty string href="" and unparseable URL href=""?

Can we test that these never fire load/error events on the <link> element?

preload/prefetch-headers.html Show resolved Hide resolved
@@ -0,0 +1,14 @@
<!DOCTYPE html>
<title>Ensures that prefetch respects cache headers</title>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong title. You can omit the <title> if there's only a single test, to avoid these sorts of mismatches.

@noamr
Copy link
Contributor Author

noamr commented Sep 5, 2022

Can we test that these never fire load/error events on the <link> element?

Seems like they are fired. Will amend the spec PR

@domenic
Copy link
Member

domenic commented Sep 6, 2022

We still need tests that as="asdf" causes the fetch to not go through (since it leads to https://whatpr.org/html/8111/links.html#link-type-prefetch step 3 setting request to null and thus step 4 returning early).

@noamr
Copy link
Contributor Author

noamr commented Sep 6, 2022

We still need tests that as="asdf" causes the fetch to not go through (since it leads to https://whatpr.org/html/8111/links.html#link-type-prefetch step 3 setting request to null and thus step 4 returning early).

Oh I think as is completely ignored. I'll amend it in the spec and add a test.

@noamr
Copy link
Contributor Author

noamr commented Sep 6, 2022

We still need tests that as="asdf" causes the fetch to not go through (since it leads to https://whatpr.org/html/8111/links.html#link-type-prefetch step 3 setting request to null and thus step 4 returning early).

Oh I think as is completely ignored. I'll amend it in the spec and add a test.

I changed the spec, prefetch overrides the options destination instead of the request destination, so as is practically ignored.

preload/prefetch-cache.html Outdated Show resolved Hide resolved
preload/prefetch-document.html Outdated Show resolved Hide resolved
return await get_prefetch_info(href);
}

async function test_prefetch_document({origin, as}, expected) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this helper is more confusing than helpful. I'd suggest inlining it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I meant the test_prefetch_document helper being confusing... the prefetch_document helper is fine.

preload/prefetch-headers.html Show resolved Hide resolved
preload/prefetch-types.html Show resolved Hide resolved
@@ -0,0 +1,29 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if all these files should go in https://github.com/web-platform-tests/wpt/tree/master/html/semantics/links/linktypes instead? Not sure, and I guess it's OK either way...

return await get_prefetch_info(href);
}

async function test_prefetch_document({origin, as}, expected) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I meant the test_prefetch_document helper being confusing... the prefetch_document helper is fine.

noamr and others added 8 commits January 17, 2023 10:18
See whatwg/html#8111 (comment)

Tests that:
* preload/prefetch-cache.html
  No 5 minute rule (cache headers must be respected)

* preload/prefetch-document.html
  No special treatment of as=document
  Whether Accept is left as its default value.
  Storage partitioning is applied, including for documents

* preload/prefetch-load-event.html
  Does not block the load event

* preload/prefetch-headers.html
  Whether any additional headers (Sec-Purpose, Purpose, X-moz, X-Purpose) are sent (depending on what we put in the spec)

* preload/prefetch-types.html
  Always uses prefetch destination, and ignores as=""
Co-authored-by: Domenic Denicola <d@domenic.me>
annevk added a commit to whatwg/fetch that referenced this pull request Jan 17, 2023
This replaces the existing `Purpose: prefetch` and `x-moz: prefetch` headers.

Tests: web-platform-tests/wpt#35707.

Part of whatwg/html#8111.

Closes w3c/webappsec-fetch-metadata#84 and w3c/resource-hints#74.

Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@domenic
Copy link
Member

domenic commented Jan 17, 2023

I guess prefetch-cache.html is flaky in Firefox, maybe because Firefox prioritizes prefetches low and so sometimes it does the prefetch, sometimes it doesn't? Is there any easy fix for this to make it fail deterministically in Firefox, or should we ask for an admin-merge?

@noamr
Copy link
Contributor Author

noamr commented Jan 17, 2023

I guess prefetch-cache.html is flaky in Firefox, maybe because Firefox prioritizes prefetches low and so sometimes it does the prefetch, sometimes it doesn't? Is there any easy fix for this to make it fail deterministically in Firefox, or should we ask for an admin-merge?

It's not the reason for the flakiness. The test waits until the prefetch is performed.
What's flaky is the reliance on particular behavior of the HTTP cache, which can decide to empty at any moment :(
Perhaps I can run it in a loop 10 times and even a single success is good enough

@domenic
Copy link
Member

domenic commented Jan 17, 2023

Eh, I doubt Firefox is clearing the HTTP cache more aggressively. More likely is that it has some bug where it doesn't actually make the value available until the next page load, or something like that... It's especially suspicious that we had exactly 5/10.

@noamr
Copy link
Contributor Author

noamr commented Jan 18, 2023

Eh, I doubt Firefox is clearing the HTTP cache more aggressively. More likely is that it has some bug where it doesn't actually make the value available until the next page load, or something like that... It's especially suspicious that we had exactly 5/10.

Ok perhaps we should admin merge this and let Mozilla deal with prefetch flakiness in their implementation...

@domenic
Copy link
Member

domenic commented Jan 18, 2023

/cc @web-platform-tests/admins for help admin-merging this. Firefox's flakiness seems to be due to genuinely inconsistent implementation; Noam has tried several workarounds with no luck.

@sideshowbarker sideshowbarker merged commit 1423a32 into master Jan 18, 2023
@sideshowbarker sideshowbarker deleted the prefetch-2 branch January 18, 2023 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants