web-platform-tests / rfcs

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

RFC 89: [testdriver] Add an execute_script function to testdriver. #89

Closed jgraham closed 2 years ago

jgraham commented 2 years ago

This provides a direct way to execute script in another browsing context.

jgraham commented 2 years ago

Gecko has something like this, but instead of passing in source text you pass in a function. Maybe we could have an API like

test_driver.execute_script(callable, {args: []})

e.g.

test_driver.execute_script(async (elem_id) => {
  await new Promise(resolve => setTimeout(resolve, 0));
  return document.getElementById(elem_id).innerHTML
}, {args: ["test"]})

If the function returns a promise, we always await the promise, if it returns a value we always just return the value.

On the backend, we'd toString() the function and json-encode the args and do something like

webdriver.execute_async_script(f"""
let callback = arguments[arguments.length - 1];
let result = ({function_string}).apply(null, {json.loads(args_array)});
Promise.resolve(result).then(callback);
""")
jgraham commented 2 years ago

I updated this based on https://github.com/web-platform-tests/rfcs/pull/89#issuecomment-888369412 But the more I think about it, the more worried I am that the fact that WebDriver is single-threaded and waiting on some complex script running in a remote context seems like it comes with a high risk of tests that have to be killed by the harness.

hiroshige-g commented 2 years ago

I tested BFCache test migration to jgraham's draft PR.

It works mostly similarly to the JavaScript-based COEP dispatcher framework, and I made a wrapper class that keeps most of the tests as-is. Diffs/wrappers needed are uploaded at https://chromium-review.googlesource.com/c/chromium/src/+/3082215.

Functionality/implementation needed in the TestDriver version:

Differences that can handled by pure JS code changes (mainly around navigations):

Others:

hiroshige-g commented 2 years ago

Now I think I've figured out sufficient API specification to run BFCache tests at https://github.com/web-platform-tests/wpt/pull/28950 (See /common/dispatcher/README.md). This is basically equivalent with execute_script()-related parts of RFCs 88/89/91, plus additional clarifications around navigation (like https://github.com/web-platform-tests/rfcs/pull/89#issuecomment-896327379), minus testdriver integration.

If it looks good, then I'd like to land the Pure-JS-based RemoteContext.execute_script() and BFCache tests in

and continue discussions on test_driver integration, as I think we will have sufficient concensus about the API surface itself, except for whether it is integrated with test_driver, and BFCache tests based on this can be migrated between JS-based and test_driver-integrated versions of RemoteContext with minimum changes.

Does this sound good?

(underlying design doc for visibility, but most of the content is now included here and in https://github.com/web-platform-tests/wpt/pull/28950)

jgraham commented 2 years ago

I don't have any objections to that. Note that https://github.com/web-platform-tests/rfcs/pull/90#issuecomment-914222361 has a new proposal (which I'll write up properly this week) to provide cross-context execute script using only js. I think it's basically compatible with what you're doing, so it should be easy to update your code to that model in the future if desired.

hiroshige-g commented 2 years ago

Yeah it looks mostly compatible (I added some comments there but they don't affect much). I'll aim to land the PRs relatively soon while I'll continue to follow the updates to the PRs.

hiroshige-g commented 2 years ago

cc/ @ArthurSonzogni.

My proposal is at https://github.com/web-platform-tests/rfcs/pull/89#issuecomment-917260985, i.e. to have consensus around RFCs 88/89/91 plus BFCache-specific clarifications except for test_driver integration and RFC 90, which is summarized in https://github.com/web-platform-tests/wpt/pull/28950.

I'd like to land this part soon and after that continue discussion around test_driver integration and RFC 90 (including jgraham@'s upcoming RFC/impl updates at https://github.com/web-platform-tests/rfcs/pull/90#issuecomment-914222361) separately, because BFCache tests don't directly depend on these two (BFCache tests are using send()/receive() indirectly, but they are encapsulated under RemoteContext.execute_script()).

jgraham commented 2 years ago

I'm withdrawning this in favour of https://github.com/web-platform-tests/rfcs/pull/98 which has a non-WebDriver based approach to the same problem.