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

Add new WebDriver support for Edge - JAVA #7164

Merged
merged 4 commits into from May 31, 2019

Conversation

@loly89
Copy link
Contributor

commented May 2, 2019

Following coding pattern from FirefoxDriver to spin up MicrosoftWebDriver.exe or MSEdgeDriver.exe depending on System Property "webdriver.edge.edgehtml".

By default, MicrosoftWebDriver.exe will be picked and launched if the system property does not exist or set to "true". If it is "false", MSEdgeDriver.exe will be used (available for download under Microsoft Edge Insider: https://developer.microsoft.com/en-us/microsoft-edge/tools/webdriver/)

I will probably follow up with a Wiki page if necessary when the PR is completed.


This change is Reviewable

loly89 added some commits Apr 30, 2019

Moving edgehtml to its own package. Add StandardSeleniumTests suite.
Moving edgehtml into its own package since the new Chromium Edge will
be used primarily. Adding StandardSeleniumTests suite for EdgeDriver
so that we can test Edge by running
'go //java/client/test/org/openqa/selenium/edge:edgehtml:run'.
Integrate ChromiumEdge under EdgeDriver and add StandardSeleniumTest.
Integrate ChromiumEdge under EdgeDriver and add StandardSeleneiumTest
suite for ChromiumEdge. We can run the test by calling
'go test_edge'. Also, all tests skipped by CHROME will also be
skipped by CHROMIUMEDGE.
@shs96c
Copy link
Member

left a comment

This looks a lot better. Thank you for the update.

I do worry that this diff clearly states that Edge is a Chrome derivative, and not a Chromium derivative, and I'm not sure that's the intent. I'd be fine extracting an abstract ChromiumDriver in a separate PR. If you'd prefer to land that after this, please LMK.

810 added a commit to 810/application that referenced this pull request May 8, 2019

@loly89

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

This looks a lot better. Thank you for the update.

I do worry that this diff clearly states that Edge is a Chrome derivative, and not a Chromium derivative, and I'm not sure that's the intent. I'd be fine extracting an abstract ChromiumDriver in a separate PR. If you'd prefer to land that after this, please LMK.

Thanks @shs96c. I'll be extracting 'ChromiumDriver' in this PR.

Extract chromium package from chrome
Extracting Chromium package from Chrome and have both Edge and Chrome
inherited from Chromium package. Also, address some PR feedbacks.
@shs96c

shs96c approved these changes May 29, 2019

Copy link
Member

left a comment

This is ready to merge once the naming is more like EdgeHtmlDriver rather than EdgeHTMLDriver. Everything else can be addressed in a later PR.

Show resolved Hide resolved java/client/src/org/openqa/selenium/chromium/ChromiumDriverInfo.java Outdated
Show resolved Hide resolved java/client/src/org/openqa/selenium/chromium/ChromiumOptions.java Outdated
Show resolved Hide resolved java/client/src/org/openqa/selenium/edge/ChromiumEdgeDriverInfo.java Outdated
Show resolved Hide resolved java/client/src/org/openqa/selenium/edge/ChromiumEdgeDriverInfo.java Outdated
* @return A self reference.
*/
@Override
public EdgeDriverService.Builder withVerbose(boolean verbose) {

This comment has been minimized.

Copy link
@shs96c

shs96c May 29, 2019

Member

I'm curious to know if you can be both verbose and silent....

Side story: when I was working in radio, my fellow engineers and I once had to find "noisy silence". You can't have silence on the air, but it had to be clear it was meant to be silent.

Show resolved Hide resolved java/client/src/org/openqa/selenium/edge/edgehtml/EdgeHTMLDriverInfo.java Outdated
Show resolved Hide resolved .../client/src/org/openqa/selenium/edge/edgehtml/EdgeHTMLDriverService.java Outdated
Show resolved Hide resolved java/client/src/org/openqa/selenium/edge/edgehtml/module-info.txt
Show resolved Hide resolved java/client/src/org/openqa/selenium/edge/edgehtml/module-info.txt
Show resolved Hide resolved java/client/src/org/openqa/selenium/edge/edgehtml/module-info.txt Outdated
Rename EdgeHTML to EdgeHtml
Rename EdgeHTML to EdgeHtml and add generics return type for
ChromeOptions.
@loly89

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

This is ready to merge once the naming is more like EdgeHtmlDriver rather than EdgeHTMLDriver. Everything else can be addressed in a later PR.

@shs96c All done :)

@shs96c

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Woohoo! Landing this now :) Congratulations!

@shs96c shs96c merged commit aa546c5 into SeleniumHQ:master May 31, 2019

@loly89 loly89 deleted the loly89:add_new_driver_support_for_edge_java branch May 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.