From 676fba48e2968c7d4ab7f442a8771c25a6664cb1 Mon Sep 17 00:00:00 2001 From: Thomas Walpole Date: Fri, 5 Aug 2016 14:44:33 -0700 Subject: [PATCH] Optimize Capybara::Result by evaluating filters lazily when possible --- History.md | 1 + lib/capybara/node/finders.rb | 4 +- lib/capybara/node/matchers.rb | 6 +- lib/capybara/queries/selector_query.rb | 10 ++- lib/capybara/result.rb | 88 +++++++++++++++++++++++--- lib/capybara/selector.rb | 1 + spec/result_spec.rb | 25 ++++++++ 7 files changed, 118 insertions(+), 17 deletions(-) diff --git a/History.md b/History.md index dc594b26..2551f301 100644 --- a/History.md +++ b/History.md @@ -18,6 +18,7 @@ Release date: Unreleased * Improved error messages for have_text matcher [Alex Chaffee, Thomas Walpole] * The `:with` option for the field selector now accepts a regular expression for matching the field value [Uwe Kubosch] * Support matching on aria-label attribute when finding fields/links/buttons - Issue #1528 [Thomas Walpole] +* Optimize Capybara::Result to only apply fields as necessary in common use-case of `.all[idx]` [Thomas Walpole] #Version 2.7.1 Release date: 2016-05-01 diff --git a/lib/capybara/node/finders.rb b/lib/capybara/node/finders.rb index b40851fc..93776e58 100644 --- a/lib/capybara/node/finders.rb +++ b/lib/capybara/node/finders.rb @@ -33,14 +33,14 @@ module Capybara synchronize(query.wait) do if query.match == :smart or query.match == :prefer_exact result = query.resolve_for(self, true) - result = query.resolve_for(self, false) if result.size == 0 && !query.exact? + result = query.resolve_for(self, false) if result.empty? && !query.exact? else result = query.resolve_for(self) end if query.match == :one or query.match == :smart and result.size > 1 raise Capybara::Ambiguous.new("Ambiguous match, found #{result.size} elements matching #{query.description}") end - if result.size == 0 + if result.empty? raise Capybara::ElementNotFound.new("Unable to find #{query.description}") end result.first diff --git a/lib/capybara/node/matchers.rb b/lib/capybara/node/matchers.rb index 48f61fd2..d4f7ce7d 100644 --- a/lib/capybara/node/matchers.rb +++ b/lib/capybara/node/matchers.rb @@ -123,8 +123,7 @@ module Capybara query = Capybara::Queries::SelectorQuery.new(*args) synchronize(query.wait) do result = query.resolve_for(self) - matches_count = Capybara::Helpers.matches_count?(result.size, query.options) - unless matches_count && ((result.size > 0) || Capybara::Helpers.expects_none?(query.options)) + unless result.matches_count? && ((!result.empty?) || Capybara::Helpers.expects_none?(query.options)) raise Capybara::ExpectationNotMet, result.failure_message end end @@ -151,8 +150,7 @@ module Capybara query = Capybara::Queries::SelectorQuery.new(*args) synchronize(query.wait) do result = query.resolve_for(self) - matches_count = Capybara::Helpers.matches_count?(result.size, query.options) - if matches_count && ((result.size > 0) || Capybara::Helpers.expects_none?(query.options)) + if result.matches_count? && ((!result.empty?) || Capybara::Helpers.expects_none?(query.options)) raise Capybara::ExpectationNotMet, result.negative_failure_message end end diff --git a/lib/capybara/queries/selector_query.rb b/lib/capybara/queries/selector_query.rb index ebb1b0f3..5bac5b0f 100644 --- a/lib/capybara/queries/selector_query.rb +++ b/lib/capybara/queries/selector_query.rb @@ -45,15 +45,19 @@ module Capybara regexp = options[:text].is_a?(Regexp) ? options[:text] : Regexp.escape(options[:text].to_s) return false if not node.text(visible).match(regexp) end + case visible when :visible then return false unless node.visible? when :hidden then return false if node.visible? end - query_filters.each do |name, filter| + + query_filters.all? do |name, filter| if options.has_key?(name) - return false unless filter.matches?(node, options[name]) + filter.matches?(node, options[name]) elsif filter.default? - return false unless filter.matches?(node, filter.default) + filter.matches?(node, filter.default) + else + true end end end diff --git a/lib/capybara/result.rb b/lib/capybara/result.rb index 90731f6a..c206a0ec 100644 --- a/lib/capybara/result.rb +++ b/lib/capybara/result.rb @@ -25,27 +25,74 @@ module Capybara def initialize(elements, query) @elements = elements - @result = elements.select { |node| query.matches_filters?(node) } - @rest = @elements - @result + @result_cache = [] + @results_enum = lazy_select_elements { |node| query.matches_filters?(node) } @query = query end - def_delegators :@result, :each, :[], :at, :size, :count, :length, - :first, :last, :values_at, :empty?, :inspect, :sample, :index + def_delegators :full_results, :size, :length, :last, :values_at, :inspect, :sample + + alias :index :find_index + + def each(&block) + @result_cache.each(&block) + loop do + next_result = @results_enum.next + @result_cache << next_result + block.call(next_result) + end + self + end + + def [](*args) + if (args.size == 1) && ((idx = args[0]).is_a? Integer) && (idx > 0) + @result_cache << @results_enum.next while @result_cache.size <= idx + @result_cache[idx] + else + full_results[*args] + end + rescue StopIteration + return nil + end + alias :at :[] + + def empty? + !any? + end def matches_count? - Capybara::Helpers.matches_count?(@result.size, @query.options) + return Integer(@query.options[:count]) == count if @query.options[:count] + + return false if @query.options[:between] && !(@query.options[:between] === count) + + if @query.options[:minimum] + begin + @result_cache << @results_enum.next while @result_cache.size < Integer(@query.options[:minimum]) + rescue StopIteration + return false + end + end + + if @query.options[:maximum] + begin + @result_cache << @results_enum.next while @result_cache.size <= Integer(@query.options[:maximum]) + return false + rescue StopIteration + end + end + + return true end def failure_message message = Capybara::Helpers.failure_message(@query.description, @query.options) if count > 0 - message << ", found #{count} #{Capybara::Helpers.declension("match", "matches", count)}: " << @result.map(&:text).map(&:inspect).join(", ") + message << ", found #{count} #{Capybara::Helpers.declension("match", "matches", count)}: " << full_results.map(&:text).map(&:inspect).join(", ") else message << " but there were no matches" end - unless @rest.empty? - elements = @rest.map(&:text).map(&:inspect).join(", ") + unless rest.empty? + elements = rest.map(&:text).map(&:inspect).join(", ") message << ". Also found " << elements << ", which matched the selector but not all filters." end message @@ -54,5 +101,30 @@ module Capybara def negative_failure_message failure_message.sub(/(to find)/, 'not \1') end + + private + + def full_results + loop do + @result_cache << @results_enum.next + end + @result_cache + end + + def rest + @rest ||= @elements - full_results + end + + def lazy_select_elements(&block) + if @elements.respond_to? :lazy #Ruby 2.0+ + @elements.lazy.select &block + else + Enumerator.new do |yielder| + @elements.each do |val| + yielder.yield(val) if block.call(val) + end + end + end + end end end diff --git a/lib/capybara/selector.rb b/lib/capybara/selector.rb index e78c0583..4617de27 100644 --- a/lib/capybara/selector.rb +++ b/lib/capybara/selector.rb @@ -158,6 +158,7 @@ Capybara.add_selector(:field) do with.is_a?(Regexp) ? node.value =~ with : node.value == with.to_s end filter(:type) do |node, type| + type = type.to_s if ['textarea', 'select'].include?(type) node.tag_name == type else diff --git a/spec/result_spec.rb b/spec/result_spec.rb index a3bb4dc2..5f1683d6 100644 --- a/spec/result_spec.rb +++ b/spec/result_spec.rb @@ -63,4 +63,29 @@ RSpec.describe Capybara::Result do el.text == 'Gamma' end).to eq(2) end + + it 'supports all modes of []' do + expect(result[1].text).to eq 'Beta' + expect(result[0,2].map &:text).to eq ['Alpha', 'Beta'] + expect(result[1..3].map &:text).to eq ['Beta', 'Gamma', 'Delta'] + expect(result[-1].text).to eq 'Delta' + end + + #Not a great test but it indirectly tests what is needed + it "should evaluate filters lazily" do + #Not processed until accessed + expect(result.instance_variable_get('@result_cache').size).to be 0 + + #Only one retrieved when needed + result.first + expect(result.instance_variable_get('@result_cache').size).to be 1 + + #works for indexed access + result[2] + expect(result.instance_variable_get('@result_cache').size).to be 3 + + #All cached when converted to array + result.to_a + expect(result.instance_variable_get('@result_cache').size).to eq 4 + end end