web-platform-tests / rfcs

web-platform-tests RFCs
75 stars 63 forks source link

RFC 86: fetch_tests_from_prefixed_local_storage #86

Closed hiroshige-g closed 2 years ago

hiroshige-g commented 3 years ago

Rendered version: https://github.com/hiroshige-g/rfcs/blob/fetch_tests_from_prefixed_local_storage/rfcs/fetch_tests_from_prefixed_local_storage.md

annevk commented 3 years ago

This also seems useful for testing COOP. cc @ArthurSonzogni @mystor

ArthurSonzogni commented 3 years ago

This also seems useful for testing COOP. cc @ArthurSonzogni @mystor

For COOP reporting, COEP credentialless, and the latest COOP tests, we have switched toward the alternative 1 discussed in this RFC: Fetch API + server-side stash. Specifically those files: (dispatcher.js, dispatcher.py, executor.html, executor.js)

This is working very well. I tend to use them for all my tests, even when it is not strictly required, because it is easier to write tests this way when there are multiple ExecutionContext. I am happy. I can write all the logic in a single HTML file "from the outside" without creating many helper files.

The solution above is universal. It supports any ExecutionContext from any origin and any status. For COOP, testing with cross-origin document is essential. For anonymous iframe or partitioned cache, relying on the localStorage seems problematic.

So, I don't believe I would use this proposal myself, but I agree this might be useful, especially given that open fetch requests are preventing a page from entering the BackForwardCache.

Note that you can use "await" with fetch in await send(uuid, message) & await receive(uuid) to wait for the fetch request to resolve before navigating away from the document. This might resolve partially the problems you have with fetch+BackForwardCache, but maybe not always.

hiroshige-g commented 3 years ago

@ArthurSonzogni, thanks for the information! The executor approach looks promising, as it is a concrete design of "from the outside" style testing. I'll try rewriting my tests using the executor approach to see how it works in BFCache cases. Still I might need additional logic to ensure there are no outgoing fetches during navigation, but hopefully I might resolve issues around my service worker related tests.

hiroshige-g commented 3 years ago

cc @nhiroki; WDYT the COEP's approach in the context of prerender tests?

jgraham commented 3 years ago

I think this is clearly a use case that needs addressing. My guess is that using the server is going to be the most effective mechanim, since that allows us to bypass all origin restrictions. If we decide to adopt that approach I'd like to see it integrated into testharness.js and documented as a first class API.

hiroshige-g commented 2 years ago

I rewrote my tests using the COEP executor framework (https://github.com/web-platform-tests/wpt/pull/28950 and https://chromium-review.googlesource.com/c/chromium/src/+/2798554/), and the rewritten tests looks better, especially serviceworker-related ones. So I'll use the COEP framework and withdraw this RFC. Thank you all for very helpful comments!

hiroshige-g commented 2 years ago

(I don't have clear ideas about integrating the COEP approach into testharness.js though; maybe moving the files to /common/?)

ArthurSonzogni commented 2 years ago

I received PR to move the COEP:credentialless implementation to /common/dispatcher/. That's basically the following API:

This provides a message queue, callable from any context.

There are also some dummy HTML for document / JS for worker, executing arbitrary code sent to them by the test driver.

I will be happy to land this after various suggestion. However I shouldn't be the only one making this decision. So I am checking, do we have reached some kind of agreement here? Should a new RFC be created, or we can continue on this one? Do you have any suggestions?

jgraham commented 2 years ago

My only concern here is that this is still ending up as a largely undocumented thing you just have to know about. I'm happy for that PR to land, but I wonder if we should be looking to move it into testharness.js and maybe add some API surface like fetch_tests_from_server that encapsulate common patterns. Those changes would indeed require an RFC. In the meantime I might request that we at least add some documentation under docs for the functionality.

hiroshige-g commented 2 years ago

I imagine that the API would be more like a remote procedure call library outside testharness.js (i.e. more sophisticasted version of send(), like evalOn I'm adding at https://github.com/web-platform-tests/wpt/pull/28950 plus error handling, plumbing exceptions and promise resolution/rejection to the caler, etc.), and checking the remote call results using assert_*().

fetch_tests_from_prefixed_local_storage or fetch_tests_from_server style would be more complicated, because if there are multiple pages involved (which is the case in BFCache tests), there are no single page that hosts e.g. async_test() object, unlike existing fetch_tests_from_*. Introducing something to host the test object persistently (e.g. a test object stored in server side) would resolve this issue, but it might also complicates the tests. Also, Switching from fetch_tests_from_prefixed_local_storage style (where assert_*() is called on the isolated Window to be navigated and results are plumbed to the main Window using fetch_tests_from_prefixed_local_storage) to the COEP executor stye (where assert_*() is called on the persistent main Window) made the tests much easier to write.

I haven't come up with any other ideas around testharness.js integration for now... any thoughts?

hiroshige-g commented 2 years ago

Do you have specific "largely undocumented thing" in mind? Some of undocumented things in my mind that I'm going to improve / document are error handling (currently mostly not handled) and synchronization (e.g. whether await send() waits for completion of injected code (answer: no)), and I'm happy to add more documentation.

jgraham commented 2 years ago

In the sense that afaict nothing under https://web-platform-tests.org/writing-tests/index.html covers this functionality. So not just code-level documentation but something that allows people to discover the feature exists in the first place.

hiroshige-g commented 2 years ago

Sounds reasonable (I also didn't know the COEP executor before this thread).

On the other hand, there are a bunch of other test helpers, frameworks, etc. in WPT, that are not documented in https://web-platform-tests.org/writing-tests. Is this executor framework enough special to be documented there? (if so perhaps we need another RFC?)

jgraham commented 2 years ago

The fact that it's been useful enough for testing two apparently unrelated features suggests to me that it's worth documenting and supporting as an "official" feature.

In terms of the API, I wonder if there's something slightly higher level where instead of passing a token around you get some kind of RemoteWindow object that you're able to message, and the library handles the token management as far as possible. I haven't thought about it in detail, but there could be oppertunities to integrate with the debug logging in testharness.js to help disgnose test failures where this feature is used.

hiroshige-g commented 2 years ago

Thanks for comments, I'll prepare documentation in writing-tests later.

As for API, I drafted a CL https://chromium-review.googlesource.com/c/chromium/src/+/3055392 to introduce wrapper classes.

jgraham commented 2 years ago

Thanks, that looks nice!

I think this stuff requires an RFC to get consensus on the API shape, not just review in the Chromium review system.

I've been looking at the problem for another angle; testdriver has the ability to execute actions in another context, but that didn't work well cross-origin (or at least didn't work for noopener cases). Since we don't currently have the ability to use WebDriver window ids directly, it makes sense to share the same kind of UUID system, and maybe same frontend, for testdriver and for this message passing infrastructure. I'll put that up as a draft PR soon, but I'd really like us to avoid landing any of this stuff before we go through the RFC process.

jgraham commented 2 years ago

OK, so I created a draft PR and a set of RFCs with a variation on this theme. It doesn't currently have all the features of dispatcher.py, but it's a good (set of) places to have the discussion going forward:

hiroshige-g commented 2 years ago

Thank you for drafting the concrete PRs/RFCs! I'll try migrating a couple of BFCache tests to the testdriver version to see how it works.