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

Make marionette the default firefox driver implementation. #621

Merged
merged 4 commits into from
Oct 22, 2017
Merged

Make marionette the default firefox driver implementation. #621

merged 4 commits into from
Oct 22, 2017

Conversation

ostap-oleksyn
Copy link
Contributor

Proposed changes

Since Selenium 3.0, marionette is the default firefox driver implementation. I think we should also move into that direction. What is done:

  1. Now, in order to use Firefox 48+, you need to specify "firefox" as browser parameter, instead of "marionette"(which I never really liked, since it can be a bit confusing for newcomers).
  2. To use the legacy Firefox (47 and lower) you need to pass "legacy_firefox" as browser parameter.
  3. Marionette Firefox is now the default browser, if you don't specify other browser parameter.
  4. Additionally I changed the opera driver creation, not sure why it used createInstanceOf() method instead of just returning new OperaDriver object.

Checklist

  • Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 64.312% when pulling 64caeb5 on ostap-oleksyn:default_firefox into 01774b9 on codeborne:master.

@codecov-io
Copy link

codecov-io commented Oct 14, 2017

Codecov Report

Merging #621 into master will decrease coverage by 0.2%.
The diff coverage is 13.04%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #621      +/-   ##
===========================================
- Coverage     60.51%   60.3%   -0.21%     
- Complexity      766     767       +1     
===========================================
  Files           148     148              
  Lines          2730    2741      +11     
  Branches        267     269       +2     
===========================================
+ Hits           1652    1653       +1     
- Misses          974     984      +10     
  Partials        104     104
Impacted Files Coverage Δ Complexity Δ
...borne/selenide/webdriver/FirefoxDriverFactory.java 87.09% <ø> (+2.24%) 9 <0> (ø) ⬇️
...ain/java/com/codeborne/selenide/Configuration.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...codeborne/selenide/webdriver/WebDriverFactory.java 85.45% <ø> (ø) 13 <0> (ø) ⬇️
...n/java/com/codeborne/selenide/WebDriverRunner.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...eborne/selenide/webdriver/RemoteDriverFactory.java 5.26% <0%> (-4.74%) 2 <0> (ø)
...deborne/selenide/webdriver/OperaDriverFactory.java 33.33% <0%> (-16.67%) 2 <0> (ø)
...rne/selenide/webdriver/WebDriverBinaryManager.java 33.33% <0%> (ø) 5 <0> (ø) ⬇️
...selenide/webdriver/LegacyFirefoxDriverFactory.java 37.5% <37.5%> (ø) 3 <3> (?)
...e/selenide/impl/WebDriverThreadLocalContainer.java 80.43% <0%> (+0.72%) 30% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01774b9...7353d6d. Read the comment docs.

@BorisOsipov
Copy link
Member

Hi @ostap-oleksyn
Thank you for changes.

Have look
https://github.com/codeborne/selenide/blob/01774b9cc8cbed18ffa4292f169cdf6718961802/src/main/java/com/codeborne/selenide/webdriver/RemoteDriverFactory.java#L31-L33

In this place remote driver creates and capabilities.setBrowserName sets from Configuration.browser.
We can't pass "legacy_firefox" as broser name here. Correct me if I'm wrong.
Could you please enhance your changes?

Also may be it makes sense find other usages of Configuration.browser and go through them carefully.

@ostap-oleksyn
Copy link
Contributor Author

@BorisOsipov thanks for pointing that out.
Fixed that and made the code a bit safer for other browsers as well. Selenide supports following browser parameters: ie, edge, opera. But those are not valid selenium browser names when using grid, they should be: internet explorer(was already fixed in code), MicrosoftEdge and operablink. So I added a little if block that replaces those parameters with valid ones based on passed browser parameter. Checked other Configuration.browser usages, and everything seems fine.
Also added safari, edge and jbrowser to the browser parameter description.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 64.101% when pulling 7353d6d on ostap-oleksyn:default_firefox into 01774b9 on codeborne:master.

@asolntsev asolntsev added this to the 4.9 milestone Oct 22, 2017
@asolntsev asolntsev self-assigned this Oct 22, 2017
@asolntsev asolntsev merged commit 5a16f48 into selenide:master Oct 22, 2017
@asolntsev
Copy link
Member

@ostap-oleksyn Merged, thank you!

asolntsev added a commit that referenced this pull request Oct 22, 2017
@ostap-oleksyn ostap-oleksyn deleted the default_firefox branch October 24, 2017 07:44
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.

5 participants