Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

move selector description to the selector object #1352

Merged
merged 1 commit into from

2 participants

@twalpole
Collaborator

No description provided.

lib/capybara/selector.rb
@@ -111,6 +120,9 @@ def filter(name, options={}, &block)
node[:type] == type
end
end
+ describe do |options|
+ " with value #{options[:with].inspect}" if options[:with]
@abotalov Collaborator
abotalov added a note

Filter is validated if options.has_key?(name) - https://github.com/jnicklas/capybara/blob/selector_description/lib/capybara/query.rb#L51. Therefore if options.has_key?(:with) should be here too (otherwise this won't be added to failure message if it's nil)

The same for :selected

@twalpole Collaborator
twalpole added a note

good catch - will fix in a few minutes
for with:nil it equates to an empty string but for :selected what would selected:nil ever be used for, I don't think it can ever match anything

@abotalov Collaborator
abotalov added a note

Yeah, you are right. There is no need to fix :selected

@twalpole Collaborator
twalpole added a note

hmmm -- and :with only put the message in if option[:with] was set before (line removed above in query.rb) - so I guess maybe that was an existing bug

@abotalov Collaborator
abotalov added a note

Yes, I think it was an existing bug but IMO it can be fixed in this PR as you touched this code anyway.

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

Other than this bug the PR looks good to me.

@twalpole
Collaborator

@abotalov I've added descriptions for the rest of the filters - please take a look

lib/capybara/selector.rb
@@ -111,6 +120,16 @@ def filter(name, options={}, &block)
node[:type] == type
end
end
+ describe do |options|
+ desc, states = "", []
+ desc << " of type #{options[:type].inspect}" if options[:type]
+ desc << " with value #{options[:with].to_s.inspect}" if options.has_key?(:with)
+ states << 'checked' if options[:checked] || (options[:unchecked] === false)
@abotalov Collaborator
abotalov added a note

Why do you use === here?

@twalpole Collaborator
twalpole added a note

good question - fixing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/capybara/selector.rb
@@ -111,6 +120,16 @@ def filter(name, options={}, &block)
node[:type] == type
end
end
+ describe do |options|
+ desc, states = "", []
+ desc << " of type #{options[:type].inspect}" if options[:type]
+ desc << " with value #{options[:with].to_s.inspect}" if options.has_key?(:with)
+ states << 'checked' if options[:checked] || (options[:unchecked] == false)
+ states << 'not checked' if options[:unchecked] || (options[:checked] == false)
@abotalov Collaborator
abotalov added a note

I think nil should be treated the same as false (as nil seems to be treated the same as false in nil ^ true):

if options[:unchecked] || !options[:checked]

Also please remove trailing whitespace that you added at this line and another places of this PR.

@twalpole Collaborator
twalpole added a note

doing that means it will always give a description - even if neither :unchecked nor :checked are set as options

@twalpole Collaborator
twalpole added a note

and I'm not sure what checked: nil or unchecked: nil should really mean as options -- only true or false make sense as values there

@abotalov Collaborator
abotalov added a note

IMO the only point of this describe block is to provide user insight on why failure occurred if user used variables instead of values:

have_field(checked: some_value)

It may turn out that for some reason (bug in user code) "some_value" is nil. Capybara considers nil as false as it invokes nil ^ true so it makes sense to consider it as false here too to make it more consistent:

if options[:unchecked] || (options.has_key?(:checked) && !options[:checked])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@twalpole twalpole merged commit 98f674e into master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 2, 2014
  1. @twalpole
This page is out of date. Refresh to see the latest.
View
2  lib/capybara/query.rb
@@ -34,7 +34,7 @@ def label; selector.label or selector.name; end
def description
@description = "#{label} #{locator.inspect}"
@description << " with text #{options[:text].inspect}" if options[:text]
- @description << " with value #{options[:with].inspect}" if options[:with]
+ @description << selector.description(options)
@description
end
View
82 lib/capybara/selector.rb
@@ -5,6 +5,7 @@ def initialize(name, block, options={})
@name = name
@block = block
@options = options
+ @options[:valid_values] = [true,false] if options[:boolean]
end
def default?
@@ -16,6 +17,9 @@ def default
end
def matches?(node, value)
+ if @options.has_key?(:valid_values) && !Array(@options[:valid_values]).include?(value)
+ warn "Invalid value #{value.inspect} passed to filter #{@name}"
+ end
@block.call(node, value)
end
end
@@ -42,6 +46,7 @@ def initialize(name, &block)
@match = nil
@label = nil
@failure_message = nil
+ @description = nil
instance_eval(&block)
end
@@ -68,6 +73,10 @@ def label(label=nil)
@label
end
+ def description(options={})
+ (@description && @description.call(options)).to_s
+ end
+
def call(locator)
if @format==:css
@css.call(locator)
@@ -83,6 +92,10 @@ def match?(locator)
def filter(name, options={}, &block)
@custom_filters[name] = Filter.new(name, block, options)
end
+
+ def describe &block
+ @description = block
+ end
end
end
@@ -100,9 +113,9 @@ def filter(name, options={}, &block)
Capybara.add_selector(:field) do
xpath { |locator| XPath::HTML.field(locator) }
- filter(:checked) { |node, value| not(value ^ node.checked?) }
- filter(:unchecked) { |node, value| (value ^ node.checked?) }
- filter(:disabled, :default => false) { |node, value| not(value ^ node.disabled?) }
+ filter(:checked, boolean: true) { |node, value| not(value ^ node.checked?) }
+ filter(:unchecked, boolean: true) { |node, value| (value ^ node.checked?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| not(value ^ node.disabled?) }
filter(:with) { |node, with| node.value == with.to_s }
filter(:type) do |node, type|
if ['textarea', 'select'].include?(type)
@@ -111,6 +124,16 @@ def filter(name, options={}, &block)
node[:type] == type
end
end
+ describe do |options|
+ desc, states = "", []
+ desc << " of type #{options[:type].inspect}" if options[:type]
+ desc << " with value #{options[:with].to_s.inspect}" if options.has_key?(:with)
+ states << 'checked' if options[:checked] || (options.has_key?(:unchecked) && !options[:unchecked])
+ states << 'not checked' if options[:unchecked] || (options.has_key?(:checked) && !options[:checked])
+ states << 'disabled' if options[:disabled]
+ desc << " that is #{states.join(' and ')}" unless states.empty?
+ desc
+ end
end
Capybara.add_selector(:fieldset) do
@@ -120,7 +143,8 @@ def filter(name, options={}, &block)
Capybara.add_selector(:link_or_button) do
label "link or button"
xpath { |locator| XPath::HTML.link_or_button(locator) }
- filter(:disabled, :default => false) { |node, value| node.tag_name == "a" or not(value ^ node.disabled?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| node.tag_name == "a" or not(value ^ node.disabled?) }
+ describe { |options| " that is disabled" if options[:disabled] }
end
Capybara.add_selector(:link) do
@@ -128,34 +152,55 @@ def filter(name, options={}, &block)
filter(:href) do |node, href|
node.first(:xpath, XPath.axis(:self)[XPath.attr(:href).equals(href.to_s)])
end
+ describe { |options| " with href #{options[:href].inspect}" if options[:href] }
end
Capybara.add_selector(:button) do
xpath { |locator| XPath::HTML.button(locator) }
- filter(:disabled, :default => false) { |node, value| not(value ^ node.disabled?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| not(value ^ node.disabled?) }
+ describe { |options| " that is disabled" if options[:disabled] }
end
Capybara.add_selector(:fillable_field) do
label "field"
xpath { |locator| XPath::HTML.fillable_field(locator) }
- filter(:disabled, :default => false) { |node, value| not(value ^ node.disabled?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| not(value ^ node.disabled?) }
+ describe { |options| " that is disabled" if options[:disabled] }
end
Capybara.add_selector(:radio_button) do
label "radio button"
xpath { |locator| XPath::HTML.radio_button(locator) }
- filter(:checked) { |node, value| not(value ^ node.checked?) }
- filter(:unchecked) { |node, value| (value ^ node.checked?) }
+ filter(:checked, boolean: true) { |node, value| not(value ^ node.checked?) }
+ filter(:unchecked, boolean: true) { |node, value| (value ^ node.checked?) }
filter(:option) { |node, value| node.value == value.to_s }
- filter(:disabled, :default => false) { |node, value| not(value ^ node.disabled?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| not(value ^ node.disabled?) }
+ describe do |options|
+ desc, states = "", []
+ desc << " with value #{options[:option].inspect}" if options[:option]
+ states << 'checked' if options[:checked] || (options.has_key?(:unchecked) && !options[:unchecked])
+ states << 'not checked' if options[:unchecked] || (options.has_key?(:checked) && !options[:checked])
+ states << 'disabled' if options[:disabled]
+ desc << " that is #{states.join(' and ')}" unless states.empty?
+ desc
+ end
end
Capybara.add_selector(:checkbox) do
xpath { |locator| XPath::HTML.checkbox(locator) }
- filter(:checked) { |node, value| not(value ^ node.checked?) }
- filter(:unchecked) { |node, value| (value ^ node.checked?) }
+ filter(:checked, boolean: true) { |node, value| not(value ^ node.checked?) }
+ filter(:unchecked, boolean: true) { |node, value| (value ^ node.checked?) }
filter(:option) { |node, value| node.value == value.to_s }
- filter(:disabled, :default => false) { |node, value| not(value ^ node.disabled?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| not(value ^ node.disabled?) }
+ describe do |options|
+ desc, states = "", []
+ desc << " with value #{options[:option].inspect}" if options[:option]
+ states << 'checked' if options[:checked] || (options.has_key?(:unchecked) && !options[:unchecked])
+ states << 'not checked' if options[:unchecked] || (options.has_key?(:checked) && !options[:checked])
+ states << 'disabled' if options[:disabled]
+ desc << " that is #{states.join(' and ')}" unless states.empty?
+ desc
+ end
end
Capybara.add_selector(:select) do
@@ -170,7 +215,15 @@ def filter(name, options={}, &block)
actual = node.all(:xpath, './/option').select { |option| option.selected? }.map { |option| option.text }
[selected].flatten.sort == actual.sort
end
- filter(:disabled, :default => false) { |node, value| not(value ^ node.disabled?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| not(value ^ node.disabled?) }
+ describe do |options|
+ desc = ""
+ desc << " with options #{options[:options].inspect}" if options[:options]
+ desc << " with at least options #{options[:with_options].inspect}" if options[:with_options]
+ desc << " with #{options[:selected].inspect} selected" if options[:selected]
+ desc << " that is disabled" if options[:disabled]
+ desc
+ end
end
Capybara.add_selector(:option) do
@@ -180,7 +233,8 @@ def filter(name, options={}, &block)
Capybara.add_selector(:file_field) do
label "file field"
xpath { |locator| XPath::HTML.file_field(locator) }
- filter(:disabled, :default => false) { |node, value| not(value ^ node.disabled?) }
+ filter(:disabled, default: false, boolean: true) { |node, value| not(value ^ node.disabled?) }
+ describe { |options| " that is disabled" if options[:disabled] }
end
Capybara.add_selector(:table) do
View
6 lib/capybara/spec/session/find_field_spec.rb
@@ -19,6 +19,12 @@
end.to raise_error(Capybara::ElementNotFound)
end
+ it "should warn if filter option is invalid" do
+ expect_any_instance_of(Kernel).to receive(:warn).
+ with('Invalid value nil passed to filter disabled')
+ @session.find_field('Dog', disabled: nil)
+ end
+
it "should be aliased as 'field_labeled' for webrat compatibility" do
expect(@session.field_labeled('Dog').value).to eq('dog')
expect do
View
12 spec/rspec/matchers_spec.rb
@@ -597,7 +597,7 @@
it "gives proper description for a given value" do
expect(have_field('Text field', with: 'some value').description).to eq("have field \"Text field\" with value \"some value\"")
end
-
+
it "passes if there is such a field" do
expect(html).to have_field('Text field')
end
@@ -639,9 +639,9 @@ def to_s
end
it "gives proper description" do
- expect(have_checked_field('it is checked').description).to eq("have field \"it is checked\"")
+ expect(have_checked_field('it is checked').description).to eq("have field \"it is checked\" that is checked")
end
-
+
context "with should" do
it "passes if there is such a field and it is checked" do
expect(html).to have_checked_field('it is checked')
@@ -688,7 +688,7 @@ def to_s
end
it "gives proper description" do
- expect(have_unchecked_field('unchecked field').description).to eq("have field \"unchecked field\"")
+ expect(have_unchecked_field('unchecked field').description).to eq("have field \"unchecked field\" that is not checked")
end
context "with should" do
@@ -736,6 +736,10 @@ def to_s
it "gives proper description" do
expect(have_select('Select Box').description).to eq("have select box \"Select Box\"")
end
+
+ it "gives proper description for a given selected value" do
+ expect(have_select('Select Box', selected: 'some value').description).to eq("have select box \"Select Box\" with \"some value\" selected")
+ end
it "passes if there is such a select" do
expect(html).to have_select('Select Box')
Something went wrong with that request. Please try again.