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.

Sign up for GitHub

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

RFC 212: Introduce require_webdriver_bidi metadata tag #212

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

RFC 212: Introduce require_webdriver_bidi metadata tag #212

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9a6e20a
RFC 212: Introduce `require_webdriver_bidi` metadata tag
sadym-chromium Oct 17, 2024
c7f4448
fix link
sadym-chromium Oct 17, 2024
7a9f49d
update wording, do not mention enabling BiDi by default
sadym-chromium Oct 23, 2024
3ffd67d
update wording, do not mention enabling BiDi by default
sadym-chromium Oct 23, 2024
da8d841
Slight wording improvements
sadym-chromium Nov 5, 2024
0ef11f1
s/deprecated/removed/
sadym-chromium Nov 11, 2024
2fa805b
add alternative of query parameter
sadym-chromium Nov 11, 2024
ec241c1
Add link to the alternative
sadym-chromium Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions rfcs/require_webdriver_bidi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# RFC 212: Introduce `require_webdriver_bidi` metadata tag

## Summary

To enable the [testdriver BiDi API](https://github.com/web-platform-tests/rfcs/blob/master/rfcs/testdriver_bidi.md) for specific WPT tests, this RFC proposes a new metadata tag: `require_webdriver_bidi`. This tag, applicable to HTML and JS files, would let test runners choose between [**WebDriverProtocol**](https://github.com/web-platform-tests/wpt/blob/9c76757f5332678f9952f6ccb3824f62d30eca1f/tools/wptrunner/wptrunner/executors/executorwebdriver.py#L644) and [**WebDriverBidiProtocol**](https://github.com/web-platform-tests/wpt/blob/9c76757f5332678f9952f6ccb3824f62d30eca1f/tools/wptrunner/wptrunner/executors/executorwebdriver.py#L736), or leverage this metadata in implementation-specific protocol selection and bindings on the test bases. This approach does not require all the browsers to implement BiDi.

[Prototype](https://github.com/web-platform-tests/wpt/pull/48622).

## Background

[RFC 185](https://github.com/web-platform-tests/rfcs/blob/master/rfcs/testdriver_bidi.md) proposes adding BiDi support to `testdriver.js`, which involves using [WebDriverBidiProtocol](https://github.com/web-platform-tests/wpt/blob/9c76757f5332678f9952f6ccb3824f62d30eca1f/tools/wptrunner/wptrunner/executors/executorwebdriver.py#L736) instead of [WebDriverProtocol](https://github.com/web-platform-tests/wpt/blob/9c76757f5332678f9952f6ccb3824f62d30eca1f/tools/wptrunner/wptrunner/executors/executorwebdriver.py#L644) for test runners that use the WebDriver protocol. However, this change could cause regressions in tests that don't use the testdriver BiDi API due to unintended side effects of the transport change. To address this, the `require_webdriver_bidi` flag will inform the test runner whether the given test requires the testdriver BiDi API, allowing the runner to choose the appropriate transport, if the default one does not support BiDi protocol parts.

If at a certain point all the browsers implement testdriver BiDi and enable it by default, this flag can be deprecated.

## Details

### Examples

[html test](https://github.com/web-platform-tests/wpt/blob/9395d384f5c69a9a3a7fc4de04249f77500b2d3f/infrastructure/webdriver/bidi/subscription.html#L3)
```javascript
<!DOCTYPE html>
<meta charset="utf-8">
<meta name="require_webdriver_bidi" content="true" />
<title>Test console log are present</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-vendor.js"></script>
<script>
promise_test(async () => {
const some_message = "SOME MESSAGE";
// Subscribe to `log.entryAdded` BiDi events. This will not add a listener to the page.
await test_driver.bidi.log.entry_added.subscribe();
// Add a listener for the log.entryAdded event. This will not subscribe to the event, so the subscription is
// required before. The cleanup is done automatically after the test is finished.
const log_entry_promise = test_driver.bidi.log.entry_added.once();
// Emit a console.log message.
// Note: Lint rule is disabled in `lint.ignore` file.
console.log(some_message);
// Wait for the log.entryAdded event to be received.
const event = await log_entry_promise;
// Assert the log.entryAdded event has the expected message.
assert_equals(event.args.length, 1);
const event_message = event.args[0];
assert_equals(event_message.value, some_message);
}, "Assert testdriver can subscribe and receive events");
```

```javascript
// META: require_webdriver_bidi=true
// META: script=/resources/testdriver.js

'use strict';

promise_test(async () => {
const some_message = "SOME MESSAGE";
// Subscribe to `log.entryAdded` BiDi events. This will not add a listener to the page.
await test_driver.bidi.log.entry_added.subscribe();
// Add a listener for the log.entryAdded event. This will not subscribe to the event, so the subscription is
// required before. The cleanup is done automatically after the test is finished.
const log_entry_promise = test_driver.bidi.log.entry_added.once();
// Emit a console.log message.
// Note: Lint rule is disabled in `lint.ignore` file.
console.log(some_message);
// Wait for the log.entryAdded event to be received.
const event = await log_entry_promise;
// Assert the log.entryAdded event has the expected message.
assert_equals(event.args.length, 1);
const event_message = event.args[0];
assert_equals(event_message.value, some_message);
}, "Assert testdriver can subscribe and receive events");
```

### Required changes

The `require_webdriver_bidi` property has to be passed from `SourceFile` through `TestharnessTest` to `Test`, where it can be consumed by specific test runners.

The `require_webdriver_bidi` property is a flag that indicates that the test requires testdriver BiDi API support. This property is used by the test runner to determine which tests to run and how to configure the test session.

> [!NOTE]
> Not having that flag does not require implementation to restrict BiDi.

The `require_webdriver_bidi` property can be set in the test source file, either `.html` or `.js`. The [SourceFile](https://github.com/web-platform-tests/wpt/blob/9395d384f5c69a9a3a7fc4de04249f77500b2d3f/tools/manifest/sourcefile.py#L503) object reads that property and passes it to the [TestharnessTest](https://github.com/web-platform-tests/wpt/blob/9395d384f5c69a9a3a7fc4de04249f77500b2d3f/tools/manifest/item.py#L169) object. The `TestharnessTest` object then passes the `require_webdriver_bidi` property to the `Test` object.

The test runner can use the `Test`’s `require_webdriver_bidi` property to determine whether to enable BiDi support for the test. If the `require_webdriver_bidi` property is set to `true`, the test runner will create a session with BiDi support.

Even though the `TestExecutor` needs information about whether to activate BiDi or not at `connect`, which happens before the runner receives the test to run, the configuration can be done at the point [the browser is configured](https://github.com/web-platform-tests/wpt/pull/48618/files#diff-0df24b5b583c460182e687f7dc7a6a79dd2cd3389bc4a96f48483f60fceb51f7R272). This data can be used later by the specific executor implementation to [decide which protocol to use](https://github.com/web-platform-tests/wpt/pull/48618/files#diff-c2f15328bc1ddfa8fb93c09ae41651e2bcfdad4f257932263a3e92c3f8deffceR248).

#### Alternatives

* [Add metadata to `.ini` files](#add-metadata-to-ini-files).

#### Lint rules

Some lint rules has to be added to verify is used correctly:
* It is unique per test file.
* It has the correct value (either `true` or `false`).

### Deprecating of the `require_webdriver_bidi` metadata tag

Expectations are that eventually all the implementations would support BiDi either via WebDriverBiDi, or via implementation-specific bindings. At that point, the `require_webdriver_bidi` metadata tag can be deprecated. This can be done by simply removing the tag.
sadym-chromium marked this conversation as resolved.
Show resolved Hide resolved

## Risks

### Having metadata in the Test is not sufficient

`TestExecutor` needs information about whether to activate BiDi or not at `connect`, which happens before the runner receives the test metadata. This can be addressed by passing the data to implementation-specific `TestharnessExecutor` via the implementation-specific `BrowserSettings` and like in the [example](https://github.com/web-platform-tests/wpt/pull/48618/files#diff-0df24b5b583c460182e687f7dc7a6a79dd2cd3389bc4a96f48483f60fceb51f7R269).

### Potential false-negative due to missing `require_webdriver_bidi` tag

Some implementations can enable BiDi by default, while others would still need enabling it via `require_webdriver_bidi`. This could lead to false-negative test passes by the tests requiring BiDi but not having the `require_webdriver_bidi` tag. This risk can be mitigated by adding documentation describing the logic behind.
Copy link
Contributor

Choose a reason for hiding this comment

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

The BiDi testdriver APIs could enforce a check that they are only being called by a test with this flag set, perhaps? At least when they're called from a top-level test. I don't think documentation alone will be enough here.


## Alternatives considered

### Add metadata to `.ini` files

In this alternative approach, the `require_webdriver_bidi` metadata would be added to the corresponding `.ini` file. However, this approach was ultimately declined because the `.ini` file metadata is primarily concerned with the expectations of the test run rather than the specific API requirements. On the other hand, the `require_webdriver_bidi` metadata is directly related to the test API itself.

### Extract testdriver BiDi API into a separate file
Copy link
Contributor

Choose a reason for hiding this comment

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

A different alternative here would be to make testdriver.js have a features query parameter that it could check before enabling the features, and which could be supported in the manifest to extract the required features. So the author would need to write <script src="/resources/testdriver.js?features=bidi"></script>, and that would be used both by the js code to decide whether to enable the feature, and also the manifest code to set the flag on the test. The problem would be cases where you include testdriver in a support file but not in the top-level test.

Copy link
Contributor Author

@sadym-chromium sadym-chromium Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify: do you think it worse mentioning as an alternative, or you insist we should implement this way instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought of it when reviewing, and I think it's worth discussing here :)

The fact that it doesn't really work for tests that are using testdriver outside the test document seems like a notable downside because you'd still need some mechanism e.g. the meta tag in the test document. But it does naturally fail closed in the sense that if you don't specify the feature you can't use it, which is a notable advantage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think think this approach would allow adding or not adding bidi in the test_driver object. So in fact in both cases of testdriver.js?features=bidi and testdriver.js, the bidi will be in the test_driver object. However, it's up to the backend to restrict the bidi calls if "bidi" parameter is not set. Please confirm I understood your proposal correctly, and if so, I think it makes sense and I will re-write the proposal accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPD: actually, testdriver can use document.currentScript.src to get it's query parameters, and enable/disable BiDi based on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/sadym-chromium/wpt_rfcs/blob/sadym/require_webdriver_bidi/rfcs/require_webdriver_bidi.md#add-feature-query-parameter-to-testdriverjs

Copy link

Choose a reason for hiding this comment

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

UPD: actually, testdriver can use document.currentScript.src to get it's query parameters, and enable/disable BiDi based on this.

Note that this doesn’t work in <script type=module>. Is that a use case testdriver needs/wants to support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started drafting this alternative in the #214

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this doesn’t work in <script type=module>. Is that a use case testdriver needs/wants to support?

I think testdriver.is currently not a module but import.meta.url could be used if it becomes a module.


In this alternative, the testdriver BiDi API is extracted from testdriver. Include of the file is used as a marker to enable BiDi sessions. This alternative involves unnecessary indirections, and would require non-trivial tracking of nested includes. Separating the testdriver BiDi API from the main codebase adds complexity and maintenance challenges. It also requires careful tracking of nested includes to ensure accurate BiDi session enabling. A simpler implementation should be considered.