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
Start creating a proper client for WebDriver BiDi #28381
Conversation
Now that I've resolved #27377 I can meaningfully try this myself. @jgraham do you have an example of what a no-op test for this is going to look like? I might try running it against https://github.com/sadym-chromium/WebDriverBiDiServerExperiments with some local hacks. |
The PR itself changes some of the existing noop tests to work with this client, so I think you can do something similar. |
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'm not done reviewing, but here's what I have so far.
await self._send(msg) | ||
|
||
async def send(self, data): | ||
if self.connection: |
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.
Can't we raise an exception if send()
is called before start()
? Is the idea convenience here?
772eed6
to
bcd22ca
Compare
An update of https://github.com/web-platform-tests/wpt/blob/master/tools/webdriver/README.md would be great! I see that the "No external PyPI dependencies are needed" claim will have to go. |
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.
Phew, finally review, great work and documentation. I have some nits, but I'll go ahead and approve so you could merge at your discretion.
|
||
async def send_command(self, method: str, params: Mapping[str, Any]) -> Awaitable[Mapping[str, Any]]: | ||
"""Send a command to the remote server""" | ||
# this isn't threadsafe |
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.
Is that OK, or should this be a TODO?
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 it's OK? It at least seems a bit strange to try and use an async class from multiple threads and I wonder a bit if the underlying library is threadsafe.
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.
Seems OK to me, just wondering if the comment was intended to caution the reader or not. Maybe say "and that's OK becase..." or if possible add an assert? Or if it isn't that interesting, just drop the comment, I assume lots of classes aren't threadsafe and yet have no such comment.
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.
Well it was mostly "I happened to think about". But if you did happen to have the class referenced from multiple threads and you were seeing inexplicable behaviour, it seems like the kind of comment that might be helpful.
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 really good, I think the change to F strings would make things better to read
0b9b351
to
180951c
Compare
async def on_message(self, data: Mapping[str, Any]) -> None: | ||
"""Handle a message from the remote server""" | ||
if "id" in data: | ||
# This is a command response or error | ||
future = self.pending_commands.get(data["id"]) | ||
if future is None: | ||
raise ValueError(f"No pending command with id {data['id']}") | ||
if "result" in data: | ||
future.set_result(data["result"]) | ||
elif "error" in data and "message" in data: | ||
assert isinstance(data["error"], str) | ||
assert isinstance(data["message"], str) | ||
future.set_exception(BidiException(data["error"], | ||
data["message"], | ||
data.get("stacktrace"))) | ||
else: | ||
raise ValueError(f"Unexpected message: {data!r}") | ||
elif "method" in data and "params" in data: | ||
# This is an event | ||
method = data["method"] | ||
listeners = self.event_listeners.get(method, []) | ||
if not listeners: | ||
listeners = self.event_listeners.get(None, []) | ||
for listener in listeners: | ||
await listener(method, data["params"]) | ||
else: | ||
raise ValueError(f"Unexpected message: {data!r}") |
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.
Is it intended the method ignores other than expected fields? I'd assume the concept "ignore everything unknown" allows quicker and easier test writings, but there can be some side effects with unexpected fields. The same belongs to the principle of the subscribing to events.
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.
Allowing additional fields is intentional; that's the approach the spec takes to forward compatability. See https://w3c.github.io/webdriver-bidi/#protocol-definition ; for the top-level fields we have the *test => any
definition (and I think the idea is that any complex object will ignore unknown fields in the same way. If we want things to be more tightly defined we'd need to open an issue on the spec itself (although the argument for strict schemas is strong when you control both ends of the wire, it's less obvious when there's an loosly coupled ecosystem of multiple idepednent clients and servers; I think if we want to tighten things up we'd need a story for transitioning clients/servers to future versions of the protocol when it may not be fully implemented everywhere).
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 for additional context, it was what I was asking about.
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 *test => any
bits in the spec indeed mean that we can't assert that there's nothing beyond "id", "method" and "params" here without going further than the spec does. But I also think it would make sense to do both of these things:
- Require that browsers (remote ends) ignore extra keys when interpreting JSON objects as commands
- Test that browsers not send any extra top-level keys with events they emit
If we change the protocol, the tests can change.
I'm not sure I'd make the same argument for parameters (keys inside "params") however, since they're much more likely to change over time, and dealing with those test failures when adding experimental (off-by-default) support for new event parameters would be a nuisance, probably.
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've filed w3c/webdriver-bidi#111, let's not include it in this PR.
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 looks great @jgraham. I'm basically new to Python3 and asyncio so I hope the comments that I added inline make sense. If not please drop the irrelevant ones.
self.command_id += 1 | ||
command_id = self.command_id |
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.
Any way to lock self.command_id
until we have a copy of its value in command_id
?
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.
Well yeah, we could of course require a lock. I don't think there's any way to atomically increment an integer and get the resulting value in Python that doesn't end up using a lock (or drop down to lower level code).
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 looks good to me, no further nits.
fa5b6c7
to
c841fee
Compare
Add a webdriver.bidi.client module with a BidiSession class that implements only the BiDi part of the protocol. An ordinary Session may have an associated BidiSession if the session was created through HTTP. To make this easier, an enable_bidi flag is added to the top-level session which automatically sets the capabilities required to enable BiDi. The session allows sending commands, and registering handlers for events. In addition to the session class, this adds some intial work on creating a structure for commands, with a BidiModule abstract class that can be used as the basis for command implementations and an @command decorator that handles actually sending the command and waiting for the result.
Co-authored-by: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com>
Add a webdriver.bidi.client module with a BidiSession class that
implements only the BiDi part of the protocol. An ordinary Session may
have an associated BidiSession if the session was created through
HTTP. To make this easier, an enable_bidi flag is added to the
top-level session which automatically sets the capabilities required
to enable BiDi.
The session allows sending commands, and registering handlers for
events.
In addition to the session class, this adds some intial work on
creating a structure for commands, with a BidiModule abstract class
that can be used as the basis for command implementations and an
@command decorator that handles actually sending the command and
waiting for the result.