-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[EXPERIMENTAL] expose testdriver + webdriver bidi #44649
[EXPERIMENTAL] expose testdriver + webdriver bidi #44649
Conversation
90c98f4
to
ac79efc
Compare
d2f0c74
to
491c4ff
Compare
ec2cdcb
to
8b261c9
Compare
8b261c9
to
5e18b51
Compare
|
||
async def subscribe(self, events, contexts): | ||
self.logger.info("Subscribing to events %s in %s" % (events, contexts)) | ||
self._subscriptions.append((events, contexts)) |
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.
A problem with this being a list is that something like
subscribe(["event"], "context")
subscribe(["event"], "context")
unsubscribe_all()
is going to end up throwing an error during unsubscription because we'll try to remove a subscription that doesn't exist. Unfortunately I think you probably need to use the return value of the command to discover the new contexts for which the event was enabled, and store only those.
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.
Good point! I updated the logic. I can see the following edge cases:
- Subscription is done with wrong arguments. In this case the subscription will not happen at all, and nothing will be added to the
_subscriptions
. - Subscription is done for the sub-context which is removed later on. To handle this case, I add top-level contexts to the
_subscriptions
.
@@ -646,6 +754,81 @@ def do_testharness(self, protocol, url, timeout): | |||
|
|||
return rv | |||
|
|||
def bidi_deserialize(self, bidi_value): |
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.
It would be very nice to use the existing WebDriver library rather than duplicating the functionality here.
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.
Do we have the BiDi script deserialization implemented in WPT?
8330261
to
63fec63
Compare
Closing to preserver the RFC references. Continue work in #45823 |
Experimental PR trying to extend testdriver with WebDriver BiDi functionality.
Open questions:
tools/wptrunner/wptrunner/testharnessreport.js
be extended as well?