feature: add a default_set_options configuration #2039
Conversation
|
this one fails probably because of my dev: https://travis-ci.org/teamcapybara/capybara/jobs/380140664 altough I can reproduce in local, |
lib/capybara/selenium/node.rb
Outdated
| # @option options [Symbol,Array] :clear (nil) The method used to clear the previous value <br/> | ||
| # nil => clear via javascript <br/> | ||
| # :none => append the new value to the existing value <br/> | ||
| # :backspace => send backspace keystrokes to clear the field <br/> | ||
| # Array => an array of keys to send before the value being set, e.g. [[:command, 'a'], :backspace] | ||
| def set(value, **options) | ||
| raise ArgumentError, "Value cannot be an Array when 'multiple' attribute is not present. Not a #{value.class}" if value.is_a?(Array) && !multiple? | ||
| options = Capybara.default_set_options.merge(options) |
There was a problem hiding this comment.
This should be implemented in capybara/node/element.rb so it affects all drivers, not in selenium specific code. Also it should pull from session_options.default_set_options not Capybara_default_set_options so it works in "threadsafe" mode too - see https://github.com/teamcapybara/capybara/blob/master/spec/per_session_config_spec.rb
| @@ -102,6 +102,12 @@ | |||
| expect { @session.first('//textarea[@readonly]').set('changed') }.to raise_error(Capybara::ReadOnlyElementError) | |||
| end | |||
|
|
|||
| it 'should use global default options', requires: [:js] do | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| @@ -102,6 +102,12 @@ | |||
| expect { @session.first('//textarea[@readonly]').set('changed') }.to raise_error(Capybara::ReadOnlyElementError) | |||
| end | |||
|
|
|||
| it 'should use global default options', requires: [:js] do | |||
| Capybara.default_set_options = { clear: :backspace } | |||
| expect_any_instance_of(Capybara::Selenium::Node).to receive(:set_text).with('gorilla', clear: :backspace) | |||
There was a problem hiding this comment.
Find the element first, and then set the expectation on element.base rather than using any_instance_of. Also you want to expect on set, not set_text since set_text is selenium specific.
|
I'm guessing the failures were Travis hiccups -- it can be flaky when our tests have to rely on timing (in order to test the waiting behaviors) - I told them to retry and they passed. I've added a few comments with needed fixes - also please add an entry to |
|
for the history, do I create an new entry |
|
@cyrilchampier Yes - create a new section - Since this is an added feature it needs to be 3.2.0 - date should be 'unreleased' |
lib/capybara.rb
Outdated
| @@ -82,6 +82,7 @@ class << self | |||
| # [reuse_server = Boolean] Reuse the server thread between multiple sessions using the same app object (Default: true) | |||
| # [threadsafe = Boolean] Whether sessions can be configured individually (Default: false) | |||
| # [server = Symbol] The name of the registered server to use when running the app under test (Default: :webrick) | |||
| # [default_node_set_options = Hash] The default options passed to Node::set (Default: {}) | |||
There was a problem hiding this comment.
Please change this back to default_set_options - The only public set API is on Node, so there's not need to specify 'node' in the name
|
@cyrilchampier Looking good other than the needing to reset the setting after the test - once that's fixed please squash all the commits and it should be ready for merge - Thanks! |
|
@twalpole I already reset it globally here https://github.com/teamcapybara/capybara/pull/2039/files#diff-b89bf9c5aa2c58704dbee3ea98fa438cR32 you also want it to be resetted at the end of the test method ? |
|
@cyrilchampier Ah - missed that because my original comment was still there - all good, then please just squash all the commits |
|
@twalpole that should be good now, there is only 1 flaky test. Thanks for your patience! |
|
@cyrilchampier Ok -- once the tests pass I'll merge this in - thanks a lot |
relative to #2038