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

Conversation

Projects
None yet
5 participants
@ostap-oleksyn
Contributor

ostap-oleksyn commented Oct 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 14, 2017

Coverage Status

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

coveralls commented Oct 14, 2017

Coverage Status

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

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io 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.

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

This comment has been minimized.

Show comment
Hide comment
@BorisOsipov

BorisOsipov Oct 14, 2017

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.

Member

BorisOsipov commented Oct 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ostap-oleksyn

ostap-oleksyn Oct 14, 2017

Contributor

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

Contributor

ostap-oleksyn commented Oct 14, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 14, 2017

Coverage Status

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

coveralls commented Oct 14, 2017

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

1 of 4 checks passed

codecov/patch 13.04% of diff hit (target 60.51%)
Details
codecov/project 60.3% (-0.21%) compared to 01774b9
Details
coverage/coveralls Coverage decreased (-0.2%) to 64.101%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asolntsev

This comment has been minimized.

Show comment
Hide comment
@asolntsev

asolntsev Oct 22, 2017

Contributor

@ostap-oleksyn Merged, thank you!

Contributor

asolntsev commented Oct 22, 2017

@ostap-oleksyn Merged, thank you!

asolntsev added a commit that referenced this pull request Oct 22, 2017

@ostap-oleksyn ostap-oleksyn deleted the ostap-oleksyn:default_firefox branch Oct 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment