Skip to content
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

Merged
merged 2 commits into from Jun 23, 2021
Merged

Start creating a proper client for WebDriver BiDi #28381

merged 2 commits into from Jun 23, 2021

Conversation

jgraham
Copy link
Contributor

@jgraham jgraham commented Apr 6, 2021

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.

@foolip
Copy link
Member

foolip commented Apr 30, 2021

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.

@jgraham
Copy link
Contributor Author

jgraham commented May 3, 2021

The PR itself changes some of the existing noop tests to work with this client, so I think you can do something similar.

Copy link
Member

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

tools/webdriver/webdriver/bidi/__init__.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
await self._send(msg)

async def send(self, data):
if self.connection:
Copy link
Member

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?

tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
@jgraham jgraham force-pushed the bidi_client branch 4 times, most recently from 772eed6 to bcd22ca Compare May 25, 2021 10:26
@foolip
Copy link
Member

foolip commented Jun 9, 2021

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.

Copy link
Member

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

tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved

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
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@AutomatedTester AutomatedTester left a 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

tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
@jgraham jgraham force-pushed the bidi_client branch 3 times, most recently from 0b9b351 to 180951c Compare June 11, 2021 13:32
Comment on lines +174 to +200
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}")
Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member

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.

@jgraham jgraham requested a review from foolip June 16, 2021 14:50
Copy link
Contributor

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

tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
Comment on lines +159 to +160
self.command_id += 1
command_id = self.command_id
Copy link
Contributor

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?

Copy link
Contributor Author

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).

tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Outdated Show resolved Hide resolved
tools/webdriver/webdriver/bidi/client.py Show resolved Hide resolved
webdriver/tests/support/fixtures.py Show resolved Hide resolved
Copy link
Member

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

@jgraham jgraham force-pushed the bidi_client branch 2 times, most recently from fa5b6c7 to c841fee Compare June 22, 2021 18:07
jgraham and others added 2 commits June 23, 2021 08:32
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants