-
Notifications
You must be signed in to change notification settings - Fork 77
Add support for testing the WebExtensions API in WPT #219
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
base: main
Are you sure you want to change the base?
Conversation
Can we get an update here to detail the current proposal? At the moment what's in the RFC doesn't seem to match what's actually in the patch. My understanding is that the proposal is for a new test wrapper indicated by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the RFC should probably also address the question of "do we want to add a new testdriver feature", given that's important to Chromium where running these tests will inherently depend on BiDi, even though that's not true in other implementations.
Just using the "bidi" flag is potentially problematic if we want to closely tie that to BiDi usage generally, given other implementations will run these tests without BiDi.
1c1665c
to
c337c72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have confirmed this design would work for Gecko's implementation of WebExtensions, I have a stack of patches that are ready to be submitted for review once this rfc is approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Chromium will be able to support these tests using the WebDriver BiDi implementation with the current proposal. Optional feedback and some notes:
feature=extensions
allows Chromium to switch over to WebDriver BiDi implementation but the PR might require some changes (I encountered some errors trying to run the PR so I could not test it). The feature flag could also serve as indication if the test globals need to be exposed to not potentially affect other tests.- When discussing internally, we were not sure if a new wrapper type (.extension.js) is really beneficial given that the test code runs in a page similar to the
.window.js
tests and not in an extension. - I still do not think that there is a need for an alternative WebDriver Classic specification that is not meant to become the standard. Even if we end up linking to it, I think it'd be good to put methods under the
extensions
namespace in testdriver.jstest_driver.web_extension.install/test_driver.web_extension.uninstall
to add future WebDriver BiDi methods there too if needed (e.g., Add a command to the "webExtension" module to retrieve a list of all the installed webextensions w3c/webdriver-bidi#898). I do not have a strong opinion here but it was brought up in discussions.
@oliverdunk I have not reviewed browser.test APIs but I assume Chromium will work with what is proposed in this doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Kiara!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with the following feedback addressed. Where possible I provided suggested edits for clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Kiara!
6c9c771
to
8f53da5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Kiara! There are a couple comments here that we've talked about before and have been marked as resolved, but I don't think have been fully integrated into the proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Kiara! I'd still like to iterate on how we pass the test status to the outer / WPT runner, but I think we're generally aligned on this shape, and I don't want to hold this up. Thanks for all your hard work here!
This RFC documents our proposal for adding support for testing the WebExtensions API in the web platform tests.
WPT PR can be found here.