Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kiaraarose
Copy link

@kiaraarose kiaraarose commented Feb 19, 2025

WPT PR can be found here.

@jgraham
Copy link
Contributor

jgraham commented Mar 3, 2025

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 .extension.js in the filename, a new testdriver API, and a function exposed by the wrapper that implements a protocol for transferring test results back from the extension? In particular the latter protocol needs to be specified somewhere (I assume it's what browser.test does, but unless that has a written specification somewhere we need enough detail in the RFC that one can understand what the requirements are without reading Chromium-specific documents).

Copy link
Member

@gsnedders gsnedders left a 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.


A proposed patch is available at https://github.com/web-platform-tests/wpt/pull/50648.

## `browser.test` API
Copy link
Member

Choose a reason for hiding this comment

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

I think putting this section into the Web Extensions spec would probably be sensible? Like sure, it's not super well-defined, but it's a starting point for a spec to then be iterated on from.

Copy link
Author

Choose a reason for hiding this comment

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

That is the goal. There are still APIs within this namespace that would need documentation. I only added documentation for the APIs used in the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think from a Core Team point of view this is our only real concern with the RFC as it stands; splitting out this section into some CG document would be an improvement.

If that can be done quickly, that would be ideal to do before merging.

@kiaraarose kiaraarose force-pushed the master branch 2 times, most recently from 1c1665c to c337c72 Compare March 21, 2025 22:06
@kiaraarose kiaraarose requested a review from gsnedders March 21, 2025 22:06
@kiaraarose kiaraarose requested a review from OrKoN April 1, 2025 15:13
Copy link

@zombie zombie left a 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.

Copy link
Contributor

@OrKoN OrKoN left a 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.js test_driver.web_extension.install/test_driver.web_extension.uninstall to add future WebDriver BiDi methods there too if needed (e.g., 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.

Copy link

@rdcronin rdcronin left a comment

Choose a reason for hiding this comment

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

Thank you, Kiara!

Copy link

@Rob--W Rob--W left a 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.

Copy link

@rdcronin rdcronin left a comment

Choose a reason for hiding this comment

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

Thanks, Kiara!

@kiaraarose kiaraarose force-pushed the master branch 3 times, most recently from 6c9c771 to 8f53da5 Compare May 29, 2025 15:34
Copy link

@rdcronin rdcronin left a 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.

Copy link

@rdcronin rdcronin left a 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.
@gsnedders
Copy link
Member

Note #219 (comment), given GitHub is terrible at threading:

I think from a Core Team point of view this is our only real concern with the RFC as it stands; splitting out [the browser.test API] section into some CG document would be an improvement.

If that can be done quickly, that would be ideal to do before merging.

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.

This is broadly fine, and although I have some feedback, it's not blocking.

I also think that it would be ideal to move the test API to an actual spec in the WebExt WG (or at least a note in the CG), rather than trying to document it here. But in the absence of that hopefully this is enough to allow people to build more or less interoperable implementations.

\
**`browser.test.assertThrows(fn, expectedError, message)`**
Asserts that a given function throws an error when executed. If the function does not throw, the test fails.
The test will also fail if `expectedError` is defined and the function does not throw an error with a message matching `expectedError`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for web-platform-tests expectedError must not be used unless the error messages are more standardized than normal JS/DOM exceptions, since otherwise it's embedding browser-specific expectations into the test.


This RFC proposes adding a new `testharness.js` wrapper type, `.extension.js`, to handle testing this
API, in addition to using `testdriver.js` to load and unload extensions. A new `feature=extensions`
was also introduced to support enabling `BiDi` specifcally for user agents who use it for the
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a style note, these are usually written in the present or future tense rather than the past, as a description of what the proposal is and what changes it will make.

Comment on lines +116 to +118
The returned promise:
- **Resolves** if all tests complete successfully.
- **Rejects** if any test fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is obviously your existing API, but for web-platform-tests you really want to structure things so that tests are as independent as possible i.e. later tests running doesn't depend on earlier tests passing. We generally have the semantics that asserts fail the test and stop further execution of that one test, but as far as possible don't prevent future tests running. Otherwise you can get into a situation where a browser that would pass all but one of the subtests in a specific file actually stops after the first (failing) subtest.

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.

10 participants