Fix all apply

[myronmarston]

This is a bug in all_apply? that I noticed while looking more into the changes I'm making for #1749.

I'm still a little confused about why some things use any_apply? and some use all_apply?. Will keep investigating, but I'm hoping perhaps @dchelimsky can remember why sometimes metadata filtering uses one and other times it uses the other?

[dchelimsky]

I'll review in the next day or so.

Sent from my iPhone

On Nov 29, 2014, at 4:59 PM, Myron Marston notifications@github.com wrote:

This is a bug in all_apply? that I noticed while looking more into the changes I'm making for #1749.

I'm still a little confused about why some things use any_apply? and some use all_apply?. Will keep investigating, but I'm hoping perhaps @dchelimsky can remember why sometimes metadata filtering uses one and other times it uses the other?

You can merge this Pull Request by running

git pull https://github.com/rspec/rspec-core fix-all-apply
Or view, comment on, or merge it at:

#1796

Commit Summary

Fix typo: expecations -> expectations
Don't wrongly apply metadata-filtered hooks to examples that opt out.
File Changes

M Changelog.md (3)
M features/expectation_framework_integration/configure_expectation_framework.feature (2)
M lib/rspec/core/example.rb (2)
M spec/rspec/core/hooks_filtering_spec.rb (15)
Patch Links:

https://github.com/rspec/rspec-core/pull/1796.patch
https://github.com/rspec/rspec-core/pull/1796.diff
—
Reply to this email directly or view it on GitHub.

[dchelimsky]

@myronmarston from what I can tell, this is the commit that introduced the split between applying all and applying any: 2a03730. Look familiar?

[myronmarston]

That looks vaguely familiar. Funny that it originated with me. I'll dig deeper, thanks.

[myronmarston]

OK, so I looked into that commit some more, and I understand why the change was made there -- before then, all filtering had used all? but for :if and :unless to work it needed to be able to exclude if :if was false (or :unless was true), regardless of any other tags or filters the groups or examples may have had.

I think we may consider removing the :if and :unless support in 4.0; it's added complexity and the original use case that prompted my suggestion and implementation of them (pending in its block form, where you want the block to always run, but to only be considered pending in some circumstances, such as on JRuby) no longer applies as we no longer have a pending block form -- instead you can just use pending "reason" if condition with a normal ruby if.

Anyhow, I also found this:

4e010e0

In that commit you changed module inclusion filtering from having all? semantics to having any? semantics. The commit message doesn't really provide a rationale; it just asserts that "module inclusion should only require matching one filter". Note, however, that hook filtering used all? semantics at that point and still does. On the surface, it seems odd to give module inclusion one kind of filtering semantic and hook filtering another -- it would seem less surprising for them to use the same semantic. Do you remember why you changed one but not the other?

I'm thinking that we might be able to re-unify these in 4.0 (particularly if we remove the :if and :unless support) and the question becomes: do we unify them using all? or any?. I'm not really sure which I prefer (since I can't think of a time in my use of rspec where it mattered) but it seems like you had a reason at the time to prefer any? for module inclusion.

[dchelimsky]

I think we'd want to include any module, hook, etc that matched at least one inclusion filter and no exclusion filters. I can't really remember the motivation for anything different from that.

[JonRowe]

I think we may consider removing the :if and :unless support in 4.0; it's added complexity and the original use case that prompted my suggestion and implementation of them (pending in its block form, where you want the block to always run, but to only be considered pending in some circumstances, such as on JRuby) no longer applies as we no longer have a pending block form -- instead you can just use pending "reason" if condition with a normal ruby if.

I'd be on board with that, although the equivalent is skip now for our usage? We can recommend before(:context) { skip if whatever }.

[myronmarston]

I'd be on board with that, although the equivalent is skip now for our usage? We can recommend before(:context) { skip if whatever }.

They aren't quite equivalent. :if/:unless act as exclusion filters, meaning the examples won't even be included in the set that run, and therefore won't be listed in the formatter output. skip includes the spec in the output (as a pending example) but doesn't run it.

I think the equivalent is a simple if:

RSpec.describe "Something that only works on 1.9+", :if => RUBY_VERSION.to_f > 1.8 do
end

# ...becomes:
RSpec.describe "Something that only works on 1.9+" do
end if RUBY_VERSION.to_f > 1.8

# or:

if RUBY_VERSION.to_f > 1.8
  RSpec.describe "Something that only works on 1.9+" do
  end
end

# or:

RSpec.describe "Something that only works on 1.9+" do
  break if RUBY_VERSION.to_f > 1.8
  # ...
end

Likewise, unless is a replacement for :unless.

Anyhow, I've been doing some more thinking about the filtering having all? vs any? semantics. I'm leaning towards making it all? because it allows us to support both:

RSpec.configure do |c|
  # MyModule will only be included in groups with both `:foo => true` AND `:bar => true`
  c.include MyModule, :foo => true, :bar => true
end

#...but if you want the `any?` semantic, just call `include` twice:

RSpec.configure do |c|
  # MyModule will only be included in groups with either `:foo => true` OR `:bar => true`
  c.include MyModule, :foo => true
  c.include MyModule, :bar => true
end

In effect, you can think of each call to include as adding an entry to a metadata_filter_hashes array, and the logic winds up being:

if metadata_filter_hashes.any? do |hash|
  if hash.all? { |k, v| applies?(k, v) }
    do_it!
  end
end

If we were to standardize on any? semantics, I don't think there'd be any way for users to get the all? semantic when they wanted it.

Thoughts?

Anyhow, regarding this PR: in investigating this I've discovered some more missing spec coverage I'm going to add before this is ready to merge.

[myronmarston]

OK, I think this is ready to review.

[myronmarston]

Anyone want to review this?

[JonRowe]

LGTM