jsilvestri commented 7 years ago

This addresses https://github.com/jejacks0n/teaspoon/issues/518. Example usage in teaspoon_env.rb:

client = Selenium::WebDriver::Remote::Http::Default.new
client.timeout = 180 # 3 mins instead of the default 60 seconds
config.driver_options = {client_driver: :firefox, client_driver_opts: {http_client: client}}

jsilvestri commented 7 years ago

I am not quite sure I understand what is failing in the tests...

CyborgMaster commented 6 years ago

I would love to get this merged. We also would like to pass client options. In this case, ours is to use chrome headless to run the tests using something like this:

  options = ::Selenium::WebDriver::Chrome::Options.new
  options.add_argument('--headless')
  options.add_argument('--disable-gpu')
  config.driver_options = { client_driver: :chrome, client_driver_ops: { options: options } }

CyborgMaster commented 6 years ago

Ok, in order to get things working for our use case, passing in the Chrome::Options as indicated above to activate headless mode, I had to modify things a little bit.

The line in question in this PR is

driver = ::Selenium::WebDriver.for(driver_options[:client_driver], driver_options[:client_driver_opts] || {})

Calling driver_options in this manner sets up a HashWithIndifferentAccess, but that causes problems inside Selenium, producing the following crash:

> [#] ArgumentError: wrong number of arguments (given 1, expected 0)
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/remote/bridge.rb:43:in `handshake'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/chrome/driver.rb:58:in `initialize'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/common/driver.rb:46:in `new'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/common/driver.rb:46:in `for'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver.rb:86:in `for'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/bundler/gems/teaspoon-60c2f9a1a466/lib/teaspoon/driver/selenium.rb:26:in `run_specs'

This is trying to pass the options into this function:

        def self.handshake(**opts)

but because the HashWithIndifferentAccess uses string keys, this fails. It only works with hashes with symbol keys.

Changing the line to:

driver = ::Selenium::WebDriver.for(driver_options[:client_driver], @options[:client_driver_opts] || {})

bypasses the HashWithIndifferentAccess creation and allows the tests to run, however, running them a second time using guard-teaspoon crashes with the following:

> [#] Errno::ECONNREFUSED: Failed to open TCP connection to 127.0.0.1:9515 (Connection refused - connect(2) for "127.0.0.1" port 9515)
> [#] /Users/jeremy/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/net/http.rb:882:in `rescue in block in connect'
> [#] /Users/jeremy/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/net/http.rb:879:in `block in connect'
> [#] /Users/jeremy/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/timeout.rb:74:in `timeout'
> [#] /Users/jeremy/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/net/http.rb:878:in `connect'
> [#] /Users/jeremy/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/net/http.rb:863:in `do_start'
> [#] /Users/jeremy/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/net/http.rb:852:in `start'
> [#] /Users/jeremy/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/net/http.rb:1398:in `request'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/remote/http/default.rb:124:in `response_for'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/remote/http/default.rb:78:in `request'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/remote/http/common.rb:61:in `call'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/remote/bridge.rb:170:in `execute'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/remote/bridge.rb:103:in `create_session'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/remote/bridge.rb:54:in `handshake'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/chrome/driver.rb:58:in `initialize'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/common/driver.rb:46:in `new'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver/common/driver.rb:46:in `for'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/gems/selenium-webdriver-3.5.2/lib/selenium/webdriver.rb:86:in `for'
> [#] /Users/jeremy/.rvm/gems/ruby-2.3.1@allynova/bundler/gems/teaspoon-60c2f9a1a466/lib/teaspoon/driver/selenium.rb:26:in `run_specs'

This can be solved by duping the options each time they are used. My guess is that Selenium modifies the object on use somehow and expects a fresh Chrome::Options object each time.

This gives us the final line (which is working for me) here:

driver = ::Selenium::WebDriver.for(driver_options[:client_driver], @options[:client_driver_opts]&.dup || {})

@jsilvestri, do you want to modify your PR to match this, would you like me to open a PR to your repo to merge this change into yours, or should I just open my own PR?

tilsammans commented 6 years ago

@CyborgMaster I would just open a new PR that contains all the needed changes.

Then we can get some movement on this issue! If you need help with anything, don't be shy.

jejacks0n commented 6 years ago

I'm a little lost -- can someone explain like I'm 5? I'm slammed at work, so if you just give me a clean PR with an explanation and reasonable certainty that it takes everything needed into account I'll merge it.

tilsammans commented 6 years ago

530 opened which contains all the fixes.

tilsammans commented 6 years ago

@jsilvestri @jejacks0n pull requests are all failing now because of CodeClimate changes. Can you or anyone look into this? I'd like to get #530 merged soon as I have fixes waiting for them downstream.

zzak commented 6 years ago

@tilsammans I'm trying to fix the Code Climate situation here in #534

zzak commented 6 years ago

Hello ya'll, #534 fixes the build so once that's merged we can hopefully push this forward. I'd like to see people be able to use teaspoon with headless chrome :)

Happy Holidays!

jejacks0n commented 6 years ago

There's several PRs that are linking to Other PRs.. I've merged the one that fixed the build.. what's the status of other ones?

tilsammans commented 6 years ago

In principle #530 contains the fixed code, but tests are now failing because the method signature changed.

I don't have time in the coming weeks to install qmake locally. It's such a pain to do.

zzak commented 6 years ago

@tilsammans I can't tell the difference, maybe you can rebase?

tilsammans commented 6 years ago

@zzak this one—#519—isn't mine. #530 I rebased already.

zzak commented 6 years ago

@tilsammans Yep, I've seen both but I'm clearly not in a place to tell which patch is best -- my suggestion is to rebase so the tests pass and / or explain how they defer. It's very likely someone with more context (i.e. the maintainer) can tell the difference but all your PR proclaims is that it "continues" this PR. How about elaborating on why your patch should be merged instead of this? What does it improve or how does it differ? Adding as much context as possible will help you get your patches merged in an ad-hoc open-source project such as this.

tilsammans commented 6 years ago

@zzak allright: my pull request #530 contains the changes proposed by @CyborgMaster which duplicates the options. He never submitted a pull request so I did that.

The tests are currently failing because the signature of the method changed. (both here and in my PR). Since I don't have qmake nor do I have 6 hours to spare to install it. that's where we all stand.

mathias commented 5 years ago

I've been trying to get this working by using the branch, but every time, I get Selenium::WebDriver::Error::WebDriverError: unable to connect to chromedriver 127.0.0.1:9515 locally.

This works on Travis. Chromedriver is working for the Capybara Rails tests. Any ideas why I get that?

Edit: I tried the branch for #530 too. Not sure what's up.

jejacks0n commented 5 years ago

you might need to use the chromedriver-helper gem. https://github.com/flavorjones/chromedriver-helper

Just add it to your gemfile and it should work -- I'm not fully sure about this either, but I've seen that on other projects too.

mathias commented 5 years ago

@jejacks0n I've got that (Capybara on Rails suite is using it) and tried updating it, but haven't gotten this working locally. Oddly, it works on Travis using the exact same config.

trcarden commented 5 years ago

Since I arrived here trying to get chrome headless working and concluded from this PR originally that chrome headless was not being worked on I thought I would share this update:

I noticed that another PR was merged to support selenium options on master via: https://github.com/jejacks0n/teaspoon/pull/537. If you install from master it looks like you can specify the to use chrome headless via

  config.driver_options = {
    client_driver: :chrome,
    selenium_options: {
      options: Selenium::WebDriver::Chrome::Options.new(args: ['headless', 'disable-gpu'])
    }
  }

I am going to try and help @jejacks0n a little here:

I have updated the wiki with a small note here: https://github.com/jejacks0n/teaspoon/wiki/Using-Selenium-WebDriver
I think the following can be closed:
- https://github.com/jejacks0n/teaspoon/pull/519 - (this PR) unless @jsilvestri objects
- https://github.com/jejacks0n/teaspoon/issues/518 - the issue for this PR
- https://github.com/jejacks0n/teaspoon/pull/530 - (another PR trying to do the same)
A friendly shoutout around https://github.com/solidusio/solidus/pull/2534 as the monkey patch might not be required anymore (or if all the waiting is might be something the mainline might benefit from)

It sure would be great if we could get a new gem cut too.

mathieujobin commented 3 years ago

looks like #537 properly fix this issue, please reopen if you disagree

jejacks0n / teaspoon

Allow client options to be passed into Jasmine #519

530 opened which contains all the fixes.