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

Fix W3C WebDriver API detection for IE #169

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

edhager
Copy link
Contributor

@edhager edhager commented Mar 19, 2019

Around version 3.12 of the IE WebDriver server (IEDriverServer.exe), it switch to using the W3C API. I added code to detect that switch.

Needed in theintern/digdug#73.

@edhager edhager requested a review from jason0x43 March 19, 2019 00:30
Copy link
Member

@jason0x43 jason0x43 left a comment

Choose a reason for hiding this comment

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

The feature check is good. The api update less so, but that's an easy enough fix (or omission). Look into doing an inline check as well (see Server.execute). Ideally we should handle more of the feature tests inline to lower startup time. (This won't work for all checks; e.g., some feature tests can't be run on certain drivers because they'll hang the browser.)

// W3C version, the object returned by the "/session" command contains
// value.capabilities rather than the value object being the capabilities
// object and the capabilities object contains an "se:ieOptions" property.
if (capabilities['se:ieOptions']) {
Copy link
Member

Choose a reason for hiding this comment

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

This is good. We should also add something to Server.execute to handle this on-the-fly (without pre-emptive feature detection). There's already code along those lines in there, but it apparently isn't handling the error being returned by IE.

@@ -15,7 +15,7 @@
"isExported": true,
Copy link
Member

Choose a reason for hiding this comment

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

Let's not include the api update, or do a manual search and replace of \\ with /.

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.

2 participants