From d4515ab25862bca111d3c47588188764d3a5629d Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Fri, 19 Feb 2021 00:49:54 +0300 Subject: [PATCH] Unify multi-condition metadata filtering Previously `all?` was only used for hooks, and `any?` was used for everything else (module inclusions, shared example group inclusion, define derived metadata). **Before** this change `MyModule` would be included to example groups defining **any** of `foo: true` or `bar: true`: ```ruby RSpec.configure do |c| c.include MyModule, :foo => true, :bar => true end ``` **After** the change the same `any?` behaviour is achievable splitting into two statements: ```ruby RSpec.configure do |c| c.include MyModule, :foo => true c.include MyModule, :bar => true end ``` `MyModule` will only be included in groups with **both** `:foo => true` AND `:bar => true`: ```ruby RSpec.configure do |c| c.include MyModule, :foo => true, :bar => true end ``` Fixes #1821 --- Changelog.md | 1 + benchmarks/module_inclusion_filtering.rb | 2 +- lib/rspec/core/configuration.rb | 10 +++--- lib/rspec/core/filter_manager.rb | 4 +-- lib/rspec/core/hooks.rb | 10 +++--- lib/rspec/core/metadata_filter.rb | 11 +++--- spec/rspec/core/configuration_spec.rb | 21 +++++++---- spec/rspec/core/filter_manager_spec.rb | 10 +++--- .../core/filterable_item_repository_spec.rb | 36 ++++++------------- 9 files changed, 48 insertions(+), 57 deletions(-) diff --git a/Changelog.md b/Changelog.md index d38150fc56..cf71fde8e8 100644 --- a/Changelog.md +++ b/Changelog.md @@ -31,6 +31,7 @@ Breaking Changes: * Remove deprecated `rerun_argument` example method. (Phil Pirozhkov, #2864) * Raise on attempt to use a legacy formatter without `rspec-legacy_formatters`. (Phil Pirozhkov, #2864) +* Unify multi-condition filtering to use "all" semantic. (Phil Pirozhkov, #2874) Enhancements: diff --git a/benchmarks/module_inclusion_filtering.rb b/benchmarks/module_inclusion_filtering.rb index 8bd71109c9..efc88eb614 100644 --- a/benchmarks/module_inclusion_filtering.rb +++ b/benchmarks/module_inclusion_filtering.rb @@ -21,7 +21,7 @@ def include(mod, *filters) def old_configure_group(group) @include_extend_or_prepend_modules.each do |include_extend_or_prepend, mod, filters| - next unless filters.empty? || RSpec::Core::MetadataFilter.apply?(:any?, filters, group.metadata) + next unless filters.empty? || RSpec::Core::MetadataFilter.apply?(filters, group.metadata) __send__("safe_#{include_extend_or_prepend}", mod, group) end end diff --git a/lib/rspec/core/configuration.rb b/lib/rspec/core/configuration.rb index 5580c8bede..e17dbb9052 100644 --- a/lib/rspec/core/configuration.rb +++ b/lib/rspec/core/configuration.rb @@ -413,9 +413,9 @@ def initialize @start_time = $_rspec_core_load_started_at || ::RSpec::Core::Time.now # rubocop:enable Style/GlobalVars @expectation_frameworks = [] - @include_modules = FilterableItemRepository::QueryOptimized.new(:any?) - @extend_modules = FilterableItemRepository::QueryOptimized.new(:any?) - @prepend_modules = FilterableItemRepository::QueryOptimized.new(:any?) + @include_modules = FilterableItemRepository::QueryOptimized.new + @extend_modules = FilterableItemRepository::QueryOptimized.new + @prepend_modules = FilterableItemRepository::QueryOptimized.new @bisect_runner = RSpec::Support::RubyFeatures.fork_supported? ? :fork : :shell @bisect_runner_class = nil @@ -455,7 +455,7 @@ def initialize @profile_examples = false @requires = [] @libs = [] - @derived_metadata_blocks = FilterableItemRepository::QueryOptimized.new(:any?) + @derived_metadata_blocks = FilterableItemRepository::QueryOptimized.new @threadsafe = true @max_displayed_failure_line_count = 10 @world = World::Null @@ -2134,7 +2134,7 @@ def on_existing_matching_groups(meta) end def metadata_applies_to_group?(meta, group) - meta.empty? || MetadataFilter.apply?(:any?, meta, group.metadata) + meta.empty? || MetadataFilter.apply?(meta, group.metadata) end def safe_prepend(mod, host) diff --git a/lib/rspec/core/filter_manager.rb b/lib/rspec/core/filter_manager.rb index 2380f472c3..4be097daa8 100644 --- a/lib/rspec/core/filter_manager.rb +++ b/lib/rspec/core/filter_manager.rb @@ -46,7 +46,7 @@ def prune(examples) examples.select do |ex| file_scoped_include?(ex.metadata, ids, locations) do - !exclusions.include_example?(ex) && non_scoped_inclusions.include_example?(ex) + (exclusions.empty? || !exclusions.include_example?(ex)) && non_scoped_inclusions.include_example?(ex) end end end @@ -166,7 +166,7 @@ def description end def include_example?(example) - MetadataFilter.apply?(:any?, @rules, example.metadata) + MetadataFilter.apply?(@rules, example.metadata) end end diff --git a/lib/rspec/core/hooks.rb b/lib/rspec/core/hooks.rb index f8bad18f51..6ae8cb6aed 100644 --- a/lib/rspec/core/hooks.rb +++ b/lib/rspec/core/hooks.rb @@ -540,18 +540,18 @@ def hooks_for(position, scope) def ensure_hooks_initialized_for(position, scope) if position == :before if scope == :example - @before_example_hooks ||= @filterable_item_repo_class.new(:all?) + @before_example_hooks ||= @filterable_item_repo_class.new else - @before_context_hooks ||= @filterable_item_repo_class.new(:all?) + @before_context_hooks ||= @filterable_item_repo_class.new end elsif position == :after if scope == :example - @after_example_hooks ||= @filterable_item_repo_class.new(:all?) + @after_example_hooks ||= @filterable_item_repo_class.new else - @after_context_hooks ||= @filterable_item_repo_class.new(:all?) + @after_context_hooks ||= @filterable_item_repo_class.new end else # around - @around_example_hooks ||= @filterable_item_repo_class.new(:all?) + @around_example_hooks ||= @filterable_item_repo_class.new end end diff --git a/lib/rspec/core/metadata_filter.rb b/lib/rspec/core/metadata_filter.rb index 423e2c0c31..2b4a9fa378 100644 --- a/lib/rspec/core/metadata_filter.rb +++ b/lib/rspec/core/metadata_filter.rb @@ -8,8 +8,8 @@ module Core module MetadataFilter class << self # @private - def apply?(predicate, filters, metadata) - filters.__send__(predicate) { |k, v| filter_applies?(k, v, metadata) } + def apply?(filters, metadata) + filters.all? { |k, v| filter_applies?(k, v, metadata) } end # @private @@ -88,8 +88,7 @@ module FilterableItemRepository class UpdateOptimized attr_reader :items_and_filters - def initialize(applies_predicate) - @applies_predicate = applies_predicate + def initialize @items_and_filters = [] end @@ -108,7 +107,7 @@ def delete(item, metadata) def items_for(request_meta) @items_and_filters.each_with_object([]) do |(item, item_meta), to_return| to_return << item if item_meta.empty? || - MetadataFilter.apply?(@applies_predicate, item_meta, request_meta) + MetadataFilter.apply?(item_meta, request_meta) end end end @@ -129,7 +128,7 @@ class QueryOptimized < UpdateOptimized alias find_items_for items_for private :find_items_for - def initialize(applies_predicate) + def initialize super @applicable_keys = Set.new @proc_keys = Set.new diff --git a/spec/rspec/core/configuration_spec.rb b/spec/rspec/core/configuration_spec.rb index 5dd7899b34..654616e029 100644 --- a/spec/rspec/core/configuration_spec.rb +++ b/spec/rspec/core/configuration_spec.rb @@ -1828,20 +1828,18 @@ def exclude?(line) expect(e3.metadata).not_to include(:reverse_description) end - it 'applies if any of multiple filters apply (to align with module inclusion semantics)' do + it 'applies if all of multiple filters apply (to align with module inclusion semantics)' do RSpec.configure do |c| c.define_derived_metadata(:a => 1, :b => 2) do |metadata| metadata[:reverse_description] = metadata[:description].reverse end end - g1 = RSpec.describe("G1", :a => 1) + g1 = RSpec.describe("G1", :a => 1, :b => 2) g2 = RSpec.describe("G2", :b => 2) - g3 = RSpec.describe("G3", :c => 3) expect(g1.metadata).to include(:reverse_description => "1G") - expect(g2.metadata).to include(:reverse_description => "2G") - expect(g3.metadata).not_to include(:reverse_description) + expect(g2.metadata).not_to include(:reverse_description) end it 'allows a metadata filter to be passed as a raw symbol' do @@ -2099,15 +2097,24 @@ def exclude?(line) expect(group.included_modules).to include(mod) end - it "requires only one matching filter" do + it "requires exact matching filter" do mod = Module.new - group = RSpec.describe("group", :foo => :bar) + group = RSpec.describe("group", :foo => :bar, :baz => :bam) config.include(mod, :foo => :bar, :baz => :bam) config.configure_group(group) expect(group.included_modules).to include(mod) end + it "doesn't include on incomplete matching filter" do + mod = Module.new + group = RSpec.describe("group", :foo => :bar) + + config.include(mod, :foo => :bar, :baz => :bam) + config.configure_group(group) + expect(group.included_modules).not_to include(mod) + end + module IncludeExtendOrPrependMeOnce def self.included(host) raise "included again" if host.instance_methods.include?(:foobar) diff --git a/spec/rspec/core/filter_manager_spec.rb b/spec/rspec/core/filter_manager_spec.rb index 2d1e56c6ed..cd565d38e0 100644 --- a/spec/rspec/core/filter_manager_spec.rb +++ b/spec/rspec/core/filter_manager_spec.rb @@ -292,15 +292,15 @@ def add_filter(options) end context "with multiple inclusion filters" do - it 'includes objects that match any of them' do + it 'includes objects that match all of them' do examples = [ - included_1 = example_with(:foo => true), - included_2 = example_with(:bar => true), - example_with(:bazz => true) + included = example_with(:foo => true, :bar => true), + example_with(:foo => true), + example_with(:baz => true) ] filter_manager.include :foo => true, :bar => true - expect(prune(examples)).to contain_exactly(included_1, included_2) + expect(prune(examples)).to contain_exactly(included) end end diff --git a/spec/rspec/core/filterable_item_repository_spec.rb b/spec/rspec/core/filterable_item_repository_spec.rb index b1a91597c4..e5937c787a 100644 --- a/spec/rspec/core/filterable_item_repository_spec.rb +++ b/spec/rspec/core/filterable_item_repository_spec.rb @@ -4,7 +4,7 @@ module Core FilterableItem = Struct.new(:name) def self.it_behaves_like_a_filterable_item_repo(&when_the_repo_has_items_with_metadata) - let(:repo) { described_class.new(:any?) } + let(:repo) { described_class.new } let(:item_1) { FilterableItem.new("Item 1") } let(:item_2) { FilterableItem.new("Item 2") } let(:item_3) { FilterableItem.new("Item 3") } @@ -96,28 +96,12 @@ def self.it_behaves_like_a_filterable_item_repo(&when_the_repo_has_items_with_me end end - context "when initialized with the `:any?` predicate" do - let(:repo) { FilterableItemRepository::QueryOptimized.new(:any?) } + it 'matches against multi-entry items when all of the metadata entries match' do + add_item item_4, :key_1 => "val_1", :key_2 => "val_2" - it 'matches against multi-entry items when any of the metadata entries match' do - add_item item_4, :key_1 => "val_1", :key_2 => "val_2" - - expect(repo.items_for(:key_1 => "val_1")).to contain_exactly(item_4) - expect(repo.items_for(:key_2 => "val_2")).to contain_exactly(item_4) - expect(repo.items_for(:key_1 => "val_1", :key_2 => "val_2")).to contain_exactly(item_4) - end - end - - context "when initialized with the `:all?` predicate" do - let(:repo) { FilterableItemRepository::QueryOptimized.new(:all?) } - - it 'matches against multi-entry items when all of the metadata entries match' do - add_item item_4, :key_1 => "val_1", :key_2 => "val_2" - - expect(repo.items_for(:key_1 => "val_1")).to eq([]) - expect(repo.items_for(:key_2 => "val_2")).to eq([]) - expect(repo.items_for(:key_1 => "val_1", :key_2 => "val_2")).to contain_exactly(item_4) - end + expect(repo.items_for(:key_1 => "val_1")).to eq([]) + expect(repo.items_for(:key_2 => "val_2")).to eq([]) + expect(repo.items_for(:key_1 => "val_1", :key_2 => "val_2")).to contain_exactly(item_4) end module_eval(&when_the_repo_has_items_with_metadata) if when_the_repo_has_items_with_metadata @@ -156,8 +140,8 @@ def self.it_behaves_like_a_filterable_item_repo(&when_the_repo_has_items_with_me [item_2, { :foo => true }] ]). and change { repo.items_for({ :foo => true }) }. - from([item_1, item_1, item_1, item_2]). - to([item_1, item_1, item_2]). + from([item_1, item_1, item_2]). + to([item_1, item_2]). and change { repo.items_for({ :foo => true, :bar => true }) }. from([item_1, item_1, item_1, item_2]). to([item_1, item_1, item_2]). @@ -221,9 +205,9 @@ def self.it_behaves_like_a_filterable_item_repo(&when_the_repo_has_items_with_me def track_metadata_filter_apply_calls Hash.new(0).tap do |call_counts| - allow(MetadataFilter).to receive(:apply?).and_wrap_original do |original, predicate, item_meta, request_meta| + allow(MetadataFilter).to receive(:apply?).and_wrap_original do |original, item_meta, request_meta| call_counts[item_meta] += 1 - original.call(predicate, item_meta, request_meta) + original.call(item_meta, request_meta) end end end