RFC 185: Add WebDriver BiDi support to testdriver.js

[sadym-chromium]

Re-apply #182

Add “testdriver.js” support for WebDriver BiDi events and actions.

---

Preview

[foolip]

I've reviewed this in the form of a document before. Spotted one minor thing when skimming again.

[jgraham]

As discussed in the infra meeting I'm going to review this, but it may take some time since it's a rather substantial proposal that is likely to have significant knock-on effects. It would also be useful to get review from anyone who is not planning to use the webdriver backend to wptrunner in the near future; that might include WebKit (e.g. @gsnedders) and Servo (@mrobinson perhaps).

[OrKoN]

Friendly ping to review the RFC @jgraham @gsnedders. There is a meeting tonight that offers an opportunity to discuss the proposed changes if there are any questions.

[jgraham]

Just FYI, I have started looking at this. I'm not sure if it's making the right set of tradeoffs in the current form, but I need more time to look again.

[jgraham]

Apologies for the long delay in providing concrete feedback here.

I'm generally very worried about the potential to leak state between tests. I think the harness needs to take responsibility for managing at least event subscription state rather than depending on developers to correctly clean up after every test.

I'm also worried about the fact that we seem to be keeping some of the hacks that were only really required because of limitations of WebDriver. In particular keeping the ActionsContext for BiDi actions seems likely to cause problems.

I'd also like to see examples of how we could use the API for use cases that currently aren't possible or are prohibitively difficult, for example running actions in cross-origin windows. BiDi should unlock that, since it adds the ability to message arbitrary contexts. However the focus on just getting console.log events doesn't really reveal if this design is a useful stepping stone to that outcome. I worry that choices here could impede our ability to make the best use of BiDi in the future, if tests end up relying on accidental features of the design.

[jgraham]

Making each test handle its own subscriptions seems problematic.

Also, if we use BiDi in the harness itself, allowing the test to unsubscribe from events could break the harness (we have some ideas to help with this in general, but that requires BiDi changes).

[sadym-chromium]

In order to address the concern, we need to add a dependency between testharness and testdriver. I see the following ways:

1. Make testharness aware of the events concept and handle subscriptions there. The events are still implemented in testdriver:

...
promise_test(async (test) => {
    ...
    const log_entry_promise = test.events["log.entryAdded"].once();
    ...
}, "Assert log event is logged", {events: {"log.entryAdded": test_driver.bidi.log.entry_added}});

The test_driver is required to allow testharness managing the subscriptions. The user is supposed to use events via testharness ( test.events...). The specific API can be discussed.
2. Make testdriver aware of testharness:

promise_test(async (test) => {
    ...
    await test_driver.bidi.log.entry_added.subscribe(test);
    ...
}, "Assert log event is logged");

This will not completely address the concern of testharness relying on events.
3. Do not unsubscribe from the events.

I find the option 1 is the preferable. WDYT?

[jgraham]

I didn't really mean each test() in a page, but between top-level test files.

The problem is that if the test file is allowed unfettered access to BiDi event subscriptions we can't isolate it from any subscriptions that the harness itself depends on, and doing something like closing the bidi session after each test would be very high overhead. The most obvious example would be if we wanted to use script.message to drive the harness, but also allow scripts themselves to emit custom messages. But it could also happen that, for example, we wanted the harness to be able to subscribe to console.log messages and re-emit them in the harness logs. Allowing the test to unsubscribe from those events would break that. So probably the harness has to manage subscriptions and know which events it subscribed to, and which should be forwarded back to the browser.

[OrKoN]

Would it be sufficient if we implement automatic clean up of events similar to how BiDi's own test suite does it https://github.com/web-platform-tests/wpt/blob/d7b9fab5017587079985d5ab29781fcf3043533e/webdriver/tests/support/fixtures_bidi.py#L50 ? we could have executewebdriver.py record all subscribe calls coming from the test file and run corresponding unsubscribe calls filtering out events required by the testharness? In that case, unsubscribe calls would not be exposed to testdriver. There will probably be some edge cases around unsubscribing context-specific and global subscriptions at the same time.

Alternative ideas would require changing the spec:

1. support multiple light-weight BiDi sessions that do not require starting/shutting down the browser.
2. returning a subscriptionId in subscribe calls so that only specific subscriptions could be cleaned up.

[sadym-chromium]

I believe the API in option 1 partly addresses the concerns you mentioned. It allows implementing event subscription and filtering in the testharness without exposing that logic to the test file, but the specific filtering of let's say script.message intended to be send to the test or to the testharness can be tricky.

So probably the harness has to manage subscriptions and know which events it subscribed to, and which should be forwarded back to the browser.

Do you mean the backend testharness, or the client-side?

[jgraham]

FWIW @OrKoN's option 2 is something we'd like to propose for the BiDi spec (although it might have to be opt-in to require a handle to unsubscribe since that's not how things currently work).

[sadym-chromium]

The proper long-term solution would be to provide separate sessions for the testharness and for each test files. This would provide proper isolations and tear down.

As an intermediate solution, in the new revision of the example PR I implemented clean up on the wptrunner side as a part of BidiEventsProtocolPart. This approach is used in the WPT BiDi tests (fixtures). This allowed to remove unsubscribe from the testdriver API, but we can always add it, if we have a valid use case.

@jgraham WDYT? If you think this approach makes sense, I will update the RFC accordingly.

[sadym-chromium]

I updated the RFC and the example with the new version. The protocol part BidiEventsProtocolPart has a method unsubscribe_all, which is responsible for keeping track of all the subscriptions.

[jgraham]

Looking at the PR, I don't like the way this is continuing to use ActionContext.

One of the big advantages of BiDi from the wpt point of view is that it should be possible to communicate with any window, not just the one that currently has WebDriver focus. This proposal seems to end up always switching the WebDriver current browsing context to the one that's getting the BiDi command, which in my opinion rather defeats the point.

Also the use of the ActionContext seems possibly unsound; if you have multiple actions running concurrently I think they can switch the current browsing context out from under each other.

[sadym-chromium]

Actually, the ActionContext is not needed in the AsyncCallbackHandler, as there is no need in switching to the required context. Updated the example.

[sadym-chromium]

Updated the AsyncCallbackHandler section and example

[jgraham]

It seems like for BiDi we should be looking to not have the resume script at all, but instead have the harness emit custom script.message events to communicate information with the harness. That's a bigger change, but I'd like to at least have a sketch of where we're going here, to be sure we aren't backing ourselves into a corner.

[sadym-chromium]

Is it s/with the harness/with the testdriver, or I misread the comment?

... but instead have the harness emit custom script.message events to communicate information with the testdriver...

Do you mean in case of BiDi, the testharness should still make a long poll for getting commands from testdriver, but emit message events directly to window, avoiding __wptrunner_process_next_event? I'm not sure if I'm getting the point.

[jgraham]

s/with the harness/with the browser/ yes.

In general the current way the testdriver event queue works, with __process_next_event is a workaround for limitations in WebDriver classic. I've been assuming that once we have BiDi we'd be able to move away from that model of polling an event queue in a single window, and instead use the browser event loop directly, and communicate via WebDriver BiDi events rather than blocking script evaluation.

We don't necessarily have to do that right away, but I'd at least like to see what the migration path looks like,

[sadym-chromium]

Do you mean in case of BiDi, the testharness should still make a long poll for getting commands from testdriver, but emit message events directly to window, avoiding __wptrunner_process_next_event? I'm not sure if I'm getting the point.

Oh, I got it. You mean the harness can provide a channel for the client-side wptrunner. I guess it is possible and makes sense, but I'd prefer to consider it in a different RFC to reduce execution risks.

[jgraham]

If you have some time to prototype that I'd appreciate it. As mentioned one concern in general is that making the wrong first step could make it harder in the long run if we bake in assumptions that don't work for other use cases.

[OrKoN]

Since the communication between the testharness and the browser is internal and it is not exposed to test authors, it looks like it can be addressed separately. What assumptions from this RFC could prevent us from changing the event implementation in the future?

It sounds like a preload script + a channel for setting up biderectional communication would not require API changes on the testdriver.js side.

[sadym-chromium]

I'd also like to see examples of how we could use the API for use cases that currently aren't possible or are prohibitively difficult, for example running actions in cross-origin windows. BiDi should unlock that, since it adds the ability to message arbitrary contexts. However the focus on just getting console.log events doesn't really reveal if this design is a useful stepping stone to that outcome. I worry that choices here could impede our ability to make the best use of BiDi in the future, if tests end up relying on accidental features of the design.

The core issue with handling several browsing context from a testdriver is in the fact the testdriver is not aware of browsing context ids. I see two approaches to address this:

1. When BiDi is available, allow testdriver to use window itself instead of the window.__wptrunner_id as a context. On the testdriver side, parse the testdriver messages and extract the browsing context ID.
2. Make testdriver be aware of it's own browsing context ID. This can be done by one of this ways:
   1. Add to script.addPreloadScript a new argument type browsingContext, which will be resolved to the actual browsingContext ID when called.
   2. Set a promise window.__browsing_context_id via script.addPreloadScript, and resolve it via script.evaluate.

[sadym-chromium]

I'd also like to see examples of how we could use the API for use cases that currently aren't possible or are prohibitively difficult, for example running actions in cross-origin windows. BiDi should unlock that, since it adds the ability to message arbitrary contexts. However the focus on just getting console.log events doesn't really reveal if this design is a useful stepping stone to that outcome. I worry that choices here could impede our ability to make the best use of BiDi in the future, if tests end up relying on accidental features of the design.

I updated the example by adding a parameter contexts to subscribe action, which can be either a window proxy or a string representing browsing context id. This became possible because of the _get_next_message uses BiDi when it is available.

[sadym-chromium]

@jgraham do you think we need to extend other test in the example to make a more explicit use case of BiDi?

[sadym-chromium]

I'd also like to see examples of how we could use the API for use cases that currently aren't possible or are prohibitively difficult, for example running actions in cross-origin windows. BiDi should unlock that, since it adds the ability to message arbitrary contexts. However the focus on just getting console.log events doesn't really reveal if this design is a useful stepping stone to that outcome. I worry that choices here could impede our ability to make the best use of BiDi in the future, if tests end up relying on accidental features of the design.

Here is a draft example of what it can look like: sadym-chromium/wpt#450.
@jgraham @gsnedders WDYT?

[jgraham]

Thanks for making the changes!

Generally I think this looks OK to me now. Ideally I'd like some feedback from someone that doesn't yet have a BiDi implementation they could hook up to this, in case there are additional concerns from that point of view.

[foolip]

I'll go ahead and merge this now. The RFC has been open for over a month and it's fair to say that we've ensured "that all participants have time to consider it." If anyone from Apple wants to leave feedback on the design, it's not too late for that even after implementation has begun.