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

RFC 212: Introduce require_webdriver_bidi metadata tag #212

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

Conversation

sadym-chromium
Copy link
Contributor

@sadym-chromium sadym-chromium commented Oct 17, 2024

@sadym-chromium sadym-chromium changed the title RFC XXX: Introduce require_webdriver_bidi metadata tag RFC 212: Introduce require_webdriver_bidi metadata tag Oct 17, 2024
@sadym-chromium sadym-chromium force-pushed the sadym/require_webdriver_bidi branch from 8770a51 to 9a6e20a Compare October 17, 2024 07:52
@sadym-chromium sadym-chromium marked this pull request as ready for review October 17, 2024 08:00
rfcs/require_webdriver_bidi.md Outdated Show resolved Hide resolved
rfcs/require_webdriver_bidi.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

In principle I think this seems like a reasonable approach to the problem of opting in to BiDi and has the short-term benefit that implementations without BiDi support can simply skip the tests that require it.


### 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.

rfcs/require_webdriver_bidi.md Show resolved Hide resolved

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

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants