-
Notifications
You must be signed in to change notification settings - Fork 242
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
allow passing selenium options #537
Conversation
lib/teaspoon/driver/selenium.rb
Outdated
@@ -44,7 +44,8 @@ def driver_options | |||
client_driver: :firefox, | |||
timeout: Teaspoon.configuration.driver_timeout.to_i, | |||
interval: 0.01, | |||
message: "Timed out" | |||
message: "Timed out", | |||
selenium_options: {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid comma after the last parameter of a method call.
lib/teaspoon/driver/selenium.rb
Outdated
@@ -44,7 +44,8 @@ def driver_options | |||
client_driver: :firefox, | |||
timeout: Teaspoon.configuration.driver_timeout.to_i, | |||
interval: 0.01, | |||
message: "Timed out" | |||
message: "Timed out", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
lib/teaspoon/driver/selenium.rb
Outdated
@@ -23,7 +23,7 @@ def initialize(options = nil) | |||
end | |||
|
|||
def run_specs(runner, url) | |||
driver = ::Selenium::WebDriver.for(driver_options[:client_driver]) | |||
driver = ::Selenium::WebDriver.for(driver_options[:client_driver], **driver_options[:selenium_options].to_options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line is too long. [122/120]
Agree with your points about phantom being deprecated. It's been on my list of things to get to -- I would accept a PR that effectively removes phantomjs and makes the chrome driver primary, otherwise I'd rather wait to pull this in to avoid two passes at trying to resolve this. |
b43880f
to
76fabb2
Compare
client_driver: :firefox, | ||
timeout: 180, | ||
interval: 0.01, | ||
message: "Timed out", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
Is the way I passed the options reasonable? If so I don't see why this PR cannot be merged regardless of changing the default. I'm assuming you refer to the docs and the generator? Or do you want to remove support? That would be a hurtful breaking change (that may delay this little change unnecessarily) If you'd lay out exactly what you want done (e.g. do you want to change the default to what I wrote above? do you want to add documentation in the wiki and the generator? etc) I'd be happy to push a PR (though I don't know where the wiki is or the generator). |
7b9b862
to
f64781e
Compare
@jejacks0n There's a few other PRs to do the same thing (#519, #530), perhaps there's enough demand to merge (one of) this change on its own? |
Is there any movement on this? I am looking at migrating off phantomjs onto headless chrome but obviously need this change in. |
@josh08h until the maintainer gets a chance to work on it, you can use the branch directly (I have ever since I opened this PR). In your Gemfile: gem 'teaspoon', github: 'odedniv/teaspoon', branch: 'selenium-options' You can fork and use your own branch if you're intimidated by doing that. The only problem I had with using Chrome is that |
Thanks for the heads up re Your patch looks good to me - I'll most likely fork off your branch. |
If y’all can figure out why console log doesn’t work I’ll merge this. |
@jejacks0n I think it's still better than using a deprecated driver :) Also this PR doesn't break anything, it just allows additional driver options (whatever driver you happen to use) |
f64781e
to
14d5a18
Compare
@odedniv and @jejacks0n Thanks for the work on this! I would love to see this merged in. We are using headless FF in our CI environment for our ruby based feature specs. And I want to ditch PhantomJS and use headless FF for teaspoon, too. Is there something holding this back that I could help with? Thanks! |
@pjmelling would you be willing to sort out CI? IT's pretty hard to merge when CI is broken and I simply haven't had time to work on open source much in the past year or two. |
#552 fixes the build. Perhaps we can proceed to merge this? :-) /cc @odedniv @jejacks0n |
I’m mobile, anybody want to rebase and check things? |
14d5a18
to
d2e82d9
Compare
Done, seem to work |
Oops, if anyone was using my PR branch (including me), your Run |
Teaspoon master has support for driver_options, but no new gem release has been cut for 2+ years (?). The merged PR including this support is here: jejacks0n/teaspoon#537 Monkey patching teaspoon locally to add chrome support until we change test frameworks.
Teaspoon: use chrome driver, via monkey patch Teaspoon master has support for driver_options, but no new gem release has been cut for 2+ years (?). The merged PR including this support is here: jejacks0n/teaspoon#537 Monkey patching teaspoon locally to add chrome support until we change test frameworks. Rubocop: clean up StringLiterals and SafeNavigation Teaspoon: set driver explicitly to :chrome Teaspoon: One more try to spin chrome here.
Teaspoon: use chrome driver, via monkey patch Teaspoon master has support for driver_options, but no new gem release has been cut for 2+ years (?). The merged PR including this support is here: jejacks0n/teaspoon#537 Monkey patching teaspoon locally to add chrome support until we change test frameworks. Rubocop: clean up StringLiterals and SafeNavigation Teaspoon: set driver explicitly to :chrome Teaspoon: One more try to spin chrome here.
Teaspoon: use chrome driver, via monkey patch Teaspoon master has support for driver_options, but no new gem release has been cut for 2+ years (?). The merged PR including this support is here: jejacks0n/teaspoon#537 Monkey patching teaspoon locally to add chrome support until we change test frameworks. Rubocop: clean up StringLiterals and SafeNavigation Teaspoon: set driver explicitly to :chrome Teaspoon: One more try to spin chrome here.
The changes to the CI configuration are needed because teaspoon_env is now[1] searched starting from Rails.root when Rails is available, which breaks our test suite because Rails.root is spec/dummy, while teaspoon_env.rb is in spec. The monkey-patch was removed because Teaspoon now supports passing options to Selenium[2]. [1]: jejacks0n/teaspoon@5b912da [2]: jejacks0n/teaspoon#537
The changes to the CI configuration are needed because teaspoon_env is now[1] searched starting from Rails.root when Rails is available, which breaks our test suite because Rails.root is spec/dummy, while teaspoon_env.rb is in spec. The monkey-patch was removed because Teaspoon now supports passing options to Selenium[2]. [1]: jejacks0n/teaspoon@5b912da [2]: jejacks0n/teaspoon#537
The changes to the CI configuration are needed because teaspoon_env is now[1] searched starting from Rails.root when Rails is available, which breaks our test suite because Rails.root is spec/dummy, while teaspoon_env.rb is in spec. The monkey-patch was removed because Teaspoon now supports passing options to Selenium[2]. [1]: jejacks0n/teaspoon@5b912da [2]: jejacks0n/teaspoon#537
The changes to the CI configuration are needed because teaspoon_env is now[1] searched starting from Rails.root when Rails is available, which breaks our test suite because Rails.root is spec/dummy, while teaspoon_env.rb is in spec. The monkey-patch was removed because Teaspoon now supports passing options to Selenium[2]. [1]: jejacks0n/teaspoon@5b912da [2]: jejacks0n/teaspoon#537
Chrome now allows headless capability, but teaspoon stood between my app and the ability to use it. Here's the fix, and this is how I use it in my
spec/teaspoon_env.rb
(I recommend adding it to the wiki):To my understanding PhantomJS ceased development because of overwhelming security issues, and the fact it can be replaced by Chrome's headless capability. I think it's time to replace it with Selenium as the default driver.