Skip to content
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

Unify multi-condition filtering #2874

Merged
merged 1 commit into from
Mar 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
2 changes: 1 addition & 1 deletion benchmarks/module_inclusion_filtering.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions lib/rspec/core/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions lib/rspec/core/filter_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? (not had enough ☕ yet 😂 )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the hardest part of the change.
Without this, exclusions.include_example?(ex) would just always return true, and its negation - false.
This is due to the change in the underlying logic in the case when there are no rules, that is coming from this:

[].all?(&:odd) # new version => true
[].any?(&:odd) # before => false

Without this check, rspec always finishes with 0 examples, 0 failures.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InclusionFilter has a similar check:

      def include_example?(example)
        @rules.empty? || super
      end

I thought of refactoring ExclusionRules to a similarly defined exclude_example?, but left it if the later refactoring. Especially since !exclude_example? is hard to wrap your head around quickly when called on an instance of ExclusionRules 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a think about wether include should take this into account, this just seems like a trap currently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there could be several rules in the exclusion filter, and we shouldn't check against them with all??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this should be encapsulated somewhere, but for now I think this is fine

end
end
end
Expand Down Expand Up @@ -166,7 +166,7 @@ def description
end

def include_example?(example)
MetadataFilter.apply?(:any?, @rules, example.metadata)
MetadataFilter.apply?(@rules, example.metadata)
end
end

Expand Down
10 changes: 5 additions & 5 deletions lib/rspec/core/hooks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
11 changes: 5 additions & 6 deletions lib/rspec/core/metadata_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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
Expand Down
21 changes: 14 additions & 7 deletions spec/rspec/core/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 5 additions & 5 deletions spec/rspec/core/filter_manager_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
pirj marked this conversation as resolved.
Show resolved Hide resolved
]

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

Expand Down
36 changes: 10 additions & 26 deletions spec/rspec/core/filterable_item_repository_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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") }
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]).
Expand Down Expand Up @@ -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
Expand Down