-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add the ability to load and test web extensions. #50648
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: master
Are you sure you want to change the base?
Add the ability to load and test web extensions. #50648
Conversation
@@ -0,0 +1,64 @@ | |||
async function testGetURLErrorCases() { |
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 usually the actual WPT tests are added separately from the infrastructure changes and tests for the infrastructure should be added to https://github.com/web-platform-tests/wpt/tree/master/infrastructure
5640ee5
to
d1f118f
Compare
@@ -975,6 +1009,7 @@ def on_environment_change(self, new_environment): | |||
|
|||
def do_test(self, test): | |||
url = self.test_url(test) | |||
self.protocol.test_path = test.path |
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 there is a better way to get that path than the way i'm doing it here. open to suggestions
c04b7f2
to
be054c4
Compare
be054c4
to
18384b4
Compare
18384b4
to
5176abf
Compare
5176abf
to
319a5bd
Compare
319a5bd
to
bec3e11
Compare
Would it be possible to rebase the PR? |
bec3e11
to
724f3a7
Compare
Updated the branch. |
Thanks! locally I get the same error that shows on the bots now:
|
724f3a7
to
83e7425
Compare
thanks! not sure why i wasn't seeing that locally. pushed a fix |
83e7425
to
8f96b02
Compare
7d5ff6c
to
7bb2dcd
Compare
679357f
to
8000041
Compare
8000041
to
7c36afe
Compare
resources/web-extensions-helper.js
Outdated
|
||
setup({ explicit_done: true }) | ||
|
||
globalThis.runTestsWithWebExtension = function(extensionPath) { |
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.
Indentation is off here
resources/web-extensions-helper.js
Outdated
path: extensionPath | ||
}) | ||
.then((result) => { | ||
let test; |
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.
Also inconsistent 2/4 spaces
resources/web-extensions-helper.js
Outdated
if (!data.result) | ||
test.set_status(test.FAIL) | ||
|
||
if (!data.remainingTests) | ||
test_driver.uninstall_web_extension(result.extension).then(() => { done() }) |
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.
Please brace single-line ifs
resources/web-extensions-helper.js
Outdated
let test; | ||
|
||
browser.test.onTestStarted.addListener((data) => { | ||
test = async_test(data.testName) |
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.
This depends on tests always running in serial. Is that absolutely guaranteed by the web extensions API? Would we be bteer having a map from testName
to test
and in onTestFinished
getting the relevant test out of the map? We could at least try to assert that the name is what we expect.
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.
the tests run serially. another test will only start once the previous one finished
resources/testdriver.js
Outdated
* @returns {Promise} Returns the extension identifier as defined in the spec. | ||
* Rejected if the extension fails to load. | ||
*/ | ||
install_web_extension: function(path) { |
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.
The docstring doesn't seem to match the actual implementation.
tools/webdriver/webdriver/client.py
Outdated
@@ -701,6 +701,12 @@ def print(self, | |||
body[prop] = value | |||
return self.send_session_command("POST", "print", body) | |||
|
|||
def install_web_extension(self, extension): |
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 a more idiomatic API in this client would be to have an Extensions
object with install
and uninstall
methods so the API would look like client.extensions.install(path=<path>)
. Also it's unclear what extension
is; I think it's a dict, but we should have named parameters instead and assemble the dict inside the method.
def setup(self): | ||
self.webdriver = self.parent.webdriver | ||
|
||
def install_web_extension(self, extension): |
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.
Similarly here, break apart the extension
argument into named parameters as much as possible.
7c36afe
to
5e75a6d
Compare
Validate this change by writing a few simple tests that verify that some APIs on browser.runtime behave as expected.
5e75a6d
to
4c8dc60
Compare
@jgraham I applied your suggested changes. can you take another look? |
I've just confirmed this still runs and passes the sample tests with a Firefox build from latest checkout, and also with
|
Validate this change by writing a few simple tests that verify that some APIs on
browser.runtime
behave as expected.The RFC document can be viewed here.