metadata Procs are called even if the tag is not present and rescued #556

Closed
eregon opened this Issue Jan 17, 2012 · 25 comments

Comments

Projects
None yet
3 participants
Contributor

eregon commented Jan 17, 2012

Hello,
I did a quick search so I hope this is no duplicate.

When you define a filter with a Proc, such as:

RSpec.configure do |config|
  config.filter_run_excluding :fails_on => lambda { |implementations|
    implementations.include? some_ruby
  }
end

The Proc is called for every example (with nil when the example has no :fails_on key in its metadata).
This is silenced to the user by the (in my opinion) bad use of the rescue false in rspec/core/matadata.rb method filter_applies? lines 180-190:

case value
...
when Proc
  if value.arity == 2
    # Pass the metadata hash to allow the proc to check if it even has the key.
    # This is necessary for the implicit :if exclusion filter:
    #   {            } # => run the example
    #   { :if => nil } # => exclude the example
    # The value of metadata[:if] is the same in these two cases but
    # they need to be treated differently.
    value.call(metadata[key], metadata) rescue false
  else
    value.call(metadata[key]) rescue false
  end
...

I somewhat understand the issue with :if, but isn't a check such as key == :if or key == :unless enough?

So the NameError that should be produced by calling nil.include? some_ruby is eaten silently.
It means the filter should actually be written in such a way to handle nil:

  config.filter_run_excluding :fails_on => lambda { |implementations|
    implementations and implementations.include? some_ruby
  }

... which seems very unintuitive for me and might produce inverted filtering.

I believe the Proc should not be called when the example does not have the tag key.

I'm still confused with RSpec tags, partly due to unexpected semantics, so please excuse me if I'm just wrong, but eating errors is never a good idea IMHO (I would much prefer to get the error).

Could someone explain me the rationale behind this?

Owner

dchelimsky commented Jan 17, 2012

Agreed re: silent swallowing of errors == bad idea. I don't recall why this got implemented this way, and removing the rescue false clauses does not cause any failures in rspec's own specs. I'm on the fence about releasing this change as part of 2.8.1 or waiting for 2.9. The reason being that some users will start seeing failures that they might not understand right away and I'd rather not introduce that disruption in a patch release (even if we agree it's the right direction long term). Let me chew on that for a minute (i.e. a day or two) and I'll report back here when I decide what to do.

Contributor

eregon commented Jan 17, 2012

I'm happy you agree about the silent swallowing.

What about not calling the Procs when the metadata key is not present? (so assume the Proc would return false in such a case, which is somewhat the current behavior as I guess exceptions often arise when passing nil, or the condition naturally returns a false value).

Does it seems a good idea to you? I think users simply don't expect to have nil, and that change would avoid most of these failures.

I would usually try myself to dig in the code and maybe do a pull request but I'm afraid I won't have time for that before a couple weeks.

@dchelimsky dchelimsky closed this in fb0b136 Feb 2, 2012

Owner

dchelimsky commented Feb 2, 2012

Next release will be 2.9 so I'm going ahead w/ this.

dchelimsky added a commit that referenced this issue Feb 2, 2012

Contributor

eregon commented Feb 2, 2012

Thanks, I'll try to look deeper into nil handling soon.

Owner

dchelimsky commented Feb 2, 2012

nil might be a perfectly valid value for the Proc to receive, so I'm not sure there's anything to do here.

Contributor

eregon commented Feb 2, 2012

Indeed, I'm not actually wanting to treat nil specially, but rather avoid to run filters with nil when the key is not present in the example metadata.

Are there use cases of filters being run with nil when the key is not present? Could you show me one?
Sorry for my lack of knowledge.

example:

# Using the configuration above
describe Spec do
  it 'example' do # no :fails_on key in the metadata
    # my :fails_on filter Proc should not be run I think as not present in the metadata
  end
end

dchelimsky added a commit that referenced this issue Feb 2, 2012

dchelimsky added a commit that referenced this issue Feb 2, 2012

Don't process filters when the metadata doesn't even have the key.
This allows us to simplify the filter_applies? method on metadata, and
gives us a small performance boost by not evaluating procs that would
always return false.

- See #556.
Owner

dchelimsky commented Feb 2, 2012

@eregon I committed 14798e1 to a branch so I could get your eyes on it for review. @myronmarston, I'd like to hear from you on it as well. Provided you both think it's sane, I'll merge it into master before the next release.

Contributor

eregon commented Feb 2, 2012

@dchelimsky Thank you. I'll be testing this evening.

An obvious change is "global" filters which would exclude when the key is not given will not be allowed anymore.
I can't think of any use case, so I think it is fine, but if this is done it should be documented clearly.

I think this change is very important given the removal of the rescue false, as it might help a lot of filters to not raise NoMethodError when they don't expect nil to be passed.

The implementation of the :if is also a lot better with this change.

Owner

myronmarston commented Feb 2, 2012

This is much improved, I think. And I agree that the semantics are better. I can't think of a case where you'd define a filter using a lambda for a particular hash key, and want the lambda to be invoked for examples that don't have that metadata hash key at all.

One thing that does concern me a bit: the removal of the arity-2 clause that passes along the metadata hash itself as an argument. I agree it's no longer needed internally (since it was there for the awkward :if case), but the cat's out of the bag, so to speak, and there may be people that have filter lambdas that accept 2 arguments that rely on that. The simple fact is that being able to receive the metadata hash itself as a second arguments makes the filtering lambdas even more powerful: you can write lambdas that interact with other metadata entries.

If you do intend to remove that, I'd prefer to see it properly deprecated with a warning. That way, if anyone does depend on it, they'll know to change their code so it doesn't use that feature or they can report an issue explaining why they find it useful.

Contributor

eregon commented Feb 2, 2012

I agree with @myronmarston about the second argument, it might be useful for custom filters.

Owner

dchelimsky commented Feb 3, 2012

I'm OK leaving the optional 2nd argument, but we should formalize and advertise it.

dchelimsky added a commit that referenced this issue Feb 3, 2012

Contributor

eregon commented Feb 3, 2012

@dchelimsky I added a commit in my branch to handle gracefully procs with arity different of 1 and 2.
(Another way to do this would be to use the original if/else, but this error is a bit more precise (1..2 instead of merely 1)

I reviewed the commits and everything seems fine to me (I also tested on my project).

I'd just add this also means filter procs don't have to handle nil anymore, except if nil is present in the metadata (it 'example', :foo => nil do) which is not common case. This is a big improvement to me.

Owner

dchelimsky commented Feb 5, 2012

@eregon add a spec and I'll merge it.

Owner

dchelimsky commented Feb 5, 2012

@eregon actually, 0 should be allowed as well. The error should only occur on arity > 2.

Contributor

eregon commented Feb 5, 2012

You're right, I will do that ASAP

eregon added a commit to eregon/rspec-core that referenced this issue Feb 5, 2012

raise an ArgumentError when the arity of a Proc filter is wrong
Otherwise the filter is not evaluated and considered to return nil

- rspec#556
Contributor

eregon commented Feb 5, 2012

@dchelimsky Done
rspec:simplify-exclusion-filter...eregon:simplify-exclusion-filter

I'm unsure about the usefulness of arity 0 (except for evil stuff like rand < 0.5). Maybe the 0 clause can be removed, and the original if arity == 2 / else be used instead.

oggy pushed a commit to oggy/rspec-core that referenced this issue Feb 7, 2012

Don't process filters when the metadata doesn't even have the key.
This allows us to simplify the filter_applies? method on metadata, and
gives us a small performance boost by not evaluating procs that would
always return false.

- Closes #556.

oggy added a commit to oggy/rspec-core that referenced this issue Feb 7, 2012

Merge branch 'master' into retry-failures
* master: (24 commits)
  fix copy/paste oversight
  Don't process filters when the metadata doesn't even have the key.
  Changelog for #564
  Fix autotest when RSpec executable path contains spaces
  it was rescue false, not rescue nil
  Changelog for #556
  don't rescue from calling proc filters
  dev: need to include dev rspec in case dependent gems depend on rspec
  ci: it's gem, not rubygems
  ci: it's before_install (RTFM)
  ci: one more try ...
  ci: before_script doesn't run before bundling, so no way to run against ree for now
  ci: update rubygems if < 1.8 (needed for ZenTest in ree)
  simplify spec for #reset
  Spec for RSpec::Core::World#reset
  Changelog for prev commit
  doc formatter strips whitespace from group and example descriptions
  changelog
  Pluralization support for runtime duration output (minute).
  Added spec for patched case
  ...
Contributor

eregon commented Feb 25, 2012

@dchelimsky Any update on this?

Owner

myronmarston commented Mar 14, 2012

I just discovered that some of the changes here broke a ton of specs in my VCR spec suite :(. I didn't realize I relied upon some of the subtle old behavior, but I guess I did. Specifically, I got failures because of this:

RSpec.configure do |config|
  config.treat_symbols_as_metadata_keys_with_true_values = true
  config.before(:each, :skip_vcr_reset => lambda { |v| v != true }) do
    VCR.reset!
  end
end

Essentially, this is global before(:each) hook that I want to run before every example except those tagged with :skip_vcr_reset. This worked before the change made in 14798e1.

@dchelimsky -- is there a better way to achieve this same thing? As far as I can tell, a regular metadata filter won't work here because the default behavior I want is to run this before hook...it's only in a few special cases where I want it to be skipped.

Contributor

eregon commented Mar 14, 2012

@myronmarston Indeed, that's a use case this change would not support.

Weirdly enough, your example does not seem to work at all for me, even before the change: the before hook is always run, even if it does not match, and seem to be run twice: https://gist.github.com/24e11287c604137eb565

Am I doing something wrong?

Back to your use-case, another way to do the same thing would be to add :vcr_reset to the whole suite (e.g.: by adding it in describe blocks) and add :vcr_reset => false where you don't want it. But it is not pretty and does not work at the moment (with and without the change).

Maybe it would be worth to add the "exclude" part of before/after/... hooks like filters have: filter_run_{including,excluding}.

And a final, aside thought: it's likely dangerous to exclude a hook like that, as the default is to run the hook, so you might just forget about it and it might produce false positives (but obviously if the use ratio is high I understand the duplication/typing argument).

Owner

myronmarston commented Mar 14, 2012

@eregon -- you put the :skip_vcr_reset thing on an individual example; I my case, I apply it to a group. I forked your gist to demonstrate:

https://gist.github.com/ca04357f34160fd27d61

Back to your use-case, another way to do the same thing would be to add :vcr_reset to the whole suite (e.g.: by adding it in describe blocks) and add :vcr_reset => false where you don't want it. But it is not pretty and does not work at the moment (with and without the change).

True, but that becomes a maintenance burden quick.

Contributor

eregon commented Mar 14, 2012

@myronmarston Indeed.

Sorry for my ignorance, but why the example-based before filtering does not work?
Is there some good documentation on this? I read the features and the specs but they only mention simple cases, like "including" with true.

Owner

myronmarston commented Mar 15, 2012

Sorry for my ignorance, but why the example-based before filtering does not work?

Good question. I don't have an answer for that without looking more into it.

Owner

myronmarston commented Mar 15, 2012

Actually, I found a way to get the same behavior on 2.9, and I think the code is simpler, to boot:

  config.before(:each) do
    unless example.metadata[:skip_vcr_reset]
      VCR.reset!
      VCR.configuration.cassette_library_dir = tmp_dir
    end
  end
Owner

dchelimsky commented Mar 18, 2012

@myronmarston does that mean you're satisfied with this change as/is?

Owner

myronmarston commented Mar 18, 2012

Yeah, I'm OK with it.

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