Provide a cleaner delineation between block vs value expectations

[myronmarston]

I've occasionally heard confusion from users about when to use expect {...} vs expect(...). Most people I've talked to haven't been confused (the matchers that need a block obviously need a block when you stop and think about it) but I can understand the confusion, particularly for users who are new to ruby (or even programming) and don't understand the reasons why particular matchers require blocks but others don't. One example of this confusion is in #525.

Currently, if you use the wrong form, you can get confusing errors or false positives/negatives. For example, passing an arg rather than a block to yield_control raises a confusing error:

expect(nil).not_to yield_control # undefined method `arity' for nil:NilClass

...but passing an arg to raise_error can result in a passing expectation:

expect(nil).not_to raise_error # passes, but prints a warning: `raise_error` was called with non-proc object nil

Meanwhile, when you pass a block rather than an arg (intending for the expectation to be set on the return value of the block), you can get false positives:

expect { Foo.bar }.not_to be_nil # passes since a block is not nil, but `Foo.bar` was never called

So, I'd like to try to come up with a way to give the user good error messages that warn them they've done the wrong thing in these situations. At a minimum, we should fix the block matchers so they fail with a consistent message when no block has been passed; then we could tell users "if you're confused, always pass an argument, and the matcher will tell you if you need a block instead". It would be nice to also give the user an error if they used a block for a value matcher, but that's much more complicated.

For starters, the block form is just syntactic sugar; the following are equivalent:

expect(lambda { raise "boom" }).to raise_error("boom")
expect { raise "boom" }.to raise_error("boom")

And in fact, you can use the block form in place of passing a lambda object for matchers that do not normally expect blocks:

expect { }.to be_a(Proc)

...works just fine, although it's a bit of a tautology.

I think I'm OK with no longer allowing these to be used interchangeably (as you generally intend one or the other, and if you have a lambda or proc object you want to use with a block matcher, you can always prefix it with & to pass as a block), but it would be a bit of a breaking change that would need to go in 3.0.

Once we do that, it would be good to provide an error for when users wrongly pass a block...but I haven't figured out a good way to do that. Ideas:

• Wrap the block in a proxy object that observers whether or not the block is called. After calling matches?, query the proxy object to see if it was called, and raise an error if it was not -- the fact that the block was not called means the matcher didn't expect a block and the user has made a mistake. The problem here is that wrapping the block in a proxy object will probably introduce edge-case bugs: for example, the proxy object can delegate call to the wrapped proc easy enough, but what if the matcher wants to use instance_eval? I don't think there's an easy way to delegate that. Also, the matcher may have Proc === actual checks that would start to fail once we pass the matcher our proxy object.
• Add a new method to the matcher protocol whereby matchers indicate if they expect a block. In practice, we'd probably only make it required for matchers that expect blocks (as the vast majority don't and wouldn't have to be changed). Then, if a block is passed to expect and the matcher does not indicate it expects a block via this new API, we could raise an error. The problem here is that it's all or nothing: we can't use this API as a means to detect a matcher expects a block unless all block matchers (including custom block matchers) have been updated to implement this API. That's problematic. I also don't want to make the matcher protocol even bigger.

Any other ideas?

[JonRowe]

I think I'm OK with no longer allowing these to be used interchangeably

Me too, I think we should force usage of the block for block matchers.

Add a new method to the matcher protocol whereby matchers indicate if they expect a block.

That was going to be my suggestion, I think that's "better" than magically inferring which matchers support block usage or not. (Note I'm ok with matchers being allowed for both block and non block usage, but they must manage it themselves).

[cupakromer]

I agree with what @JonRowe stated. Those were going to be my comments as well.

@myronmarston were you thinking of a query method such as allow_block? or setting up a 2nd matches method matches_with_block??

[myronmarston]

@myronmarston were you thinking of a query method such as allow_block? or setting up a 2nd matches method matches_with_block??

matches_with_block? is problematic in that it would require us to make a companion does_not_match_with_block? method, and I don't want to add 2 new methods to the matcher protocol.

allow_block? is better, but "allowing" a block suggests the block is optional....and yet, the point of this is to enforce that a block (and no arg) is always passed for a block matcher and never passed for a value matcher. I'm thinking block_matcher? as the new method: a matcher is either a block matcher or it isn't.

One thing that concerns me is that if we do this, it'll break any custom block matchers created by users or 3rd parties. I guess we can add a deprecation warning to 2.99, though.

[cupakromer]

@myronmarston, @JonRowe had mentioned:

Note I'm ok with matchers being allowed for both block and non block usage, but they must manage it themselves

In general I tend to agree with this. Checking my work projects, I thought I had done this more, but it appears I've only done this once. It was for a rails app where I added a have_status matcher. This accepted both a response object, as well as a block. If a block was passed, it cleared the response before running the block, and verified the new status after. While I could easily make this dual matchers, it was nice to keep the API the same for either a response or request action.

[myronmarston]

In general I tend to agree with this. Checking my work projects, I thought I had done this more, but it appears I've only done this once. It was for a rails app where I added a have_status matcher. This accepted both a response object, as well as a block. If a block was passed, it cleared the response before running the block, and verified the new status after. While I could easily make this dual matchers, it was nice to keep the API the same for either a response or request action.

It sounds like you're talking about this:

expect(response).to have_status { }

...which would be unaffected. The block vs non block we're talking about is expect() vs expect { }. I can't think of a single matcher I've seen that is designed to work with either both forms.

[cupakromer]

@myronmarston I was referring to expect() and expect{ }. Here's the sample for that one matcher at work:

it "accepts a response object" do
  get :index
  expect(response).to have_status :not_found
end

it "allows an action proc" do
  expect{ get :index }.to have_status :not_found
end

As I've proven to myself, this isn't widely used. Though, I'm increasing a fan of the expect{ } syntax for making the action under test explicit. The dual use matcher is something I'm still playing with.

[myronmarston]

Hmm, yours is the first I've seen to support either. It seems valid to support that (even though it's non-standard) so we'd want the new method to provide a way to signal one of three values: that it requires a block, allows a block, or does not allow a block. We'll need to think more about this to come up with the right interface.

[JonRowe]

Based on above, I'm thinking that @cupakromer's suggestion of adding two methods is best, that way theres an explicit match for (), and for {} and we rescue the NoMethodError as a unsupported matcher type error or something... I'd rather we didn't have to do a conditional each time we pass a matcher to to

[cupakromer]

Mostly thinking out loud here. How about using three methods: matches?, matches_object?, matches_with_block?

I'm not happy with the names so feel free to change them.

To support backwards compatibility, and future extension, we always call matches? from to. For the built-in base matcher, and the DSL, we change matches? to something like:

def matches?(thing)
  if respond_to?(:matches_object?)
    raise "oops" if Proc === thing
    matches_object?(thing)
  elsif respond_to?(:matches_with_block?)
    raise "oops" unless Proc === thing
    matches_with_block?(thing)
  end
end

I'm not happy with that code either, but it demos the general idea. The respond_to? checks should be much faster than dealing with NoMethod errors, so I'm happier about that than simply calling things that don't exist.

If a matcher author wants to support both styles, they simply write a custom matches?. At which point it is up to them to handle improper types.

Caveats:

• This would require updating all the existing matchers
• This would not make custom matchers more helpful by default after upgrading
• It's not possibly to raise warnings or deprecation notices if a matcher does not respond to matches_object? or matches_with_block?
• Much of the benefit will only come from those who read the new docs

Eh....

[JonRowe]

I think we'd just have matches? and matches_block? (with their inverse), thus it's easy to support one, the other, or both. This is a new API in 3.0 so we don't have to worry about "backward" compatibility on going, just warn about it in 2.99 (for those people supporting both they can just alias if they want).

[JonRowe]

I don't want to add a conditional check to matches like you propose because of the overhead of hitting it everytime, I think it's better to rescue NoMethodError in this instance.

[myronmarston]

There's something about adding new matches? methods that rubs me the wrong way. It has repercussions for things like the custom matcher DSL and for 3rd party matchers, and it also feels wrong to me. Semantically, matches? and matches_block? would do the same thing; we'd just be using it for an alternate purpose to discover if the matcher supports a block or not. If that's our best idea, I'd lean towards doing nothing, given that the number of users confused by this is on the smaller side.

I have another idea, though: a new matcher_type method, which can return :value, :block, or [:value, :block] (for the extremely rare situations like @cupakromer's custom matcher). This adds only one new method to the protocol which has an extremely focused, obvious purpose, and doesn't conflate matching with indicating if its a block or value matcher. If we do this, I'd make it an optional part of the protocol that is treated as being :value if not defined, so that gems that have already updated there matchers for rspec 3 (e.g. capybara and shoulda) would only have to define this new matcher on their block matchers, if they have any.

[JonRowe]

I have another idea, though: a new matcher_type method, which can return :value, :block, or [:value, :block]

That was my first instinct, but I couldn't think of a way that didn't involve a conditional check on every single matcher call, which I instantly took a disliking to.

[myronmarston]

That was my first instinct, but I couldn't think of a way that didn't involve a conditional check on every single matcher call, which I instantly took a disliking to.

I think that's far better than rescuing NoMethodError: consider that if we go with matches_block? and a matcher implements that, it may have internal logic that calls an undefined on nil (or some other unexpected value) and rescuing NoMethodError would cause us to report the wrong error. We could parse the error, of course, but that's a path I don't want to go down. Also, I'm not really concerned about the perf hit of one extra method call.

[myronmarston]

After doing some more thinking, I'm leaning towards a single block_matcher? boolean predicate method (rather than matcher_type which can return a symbol or array of symbols).

Originally, I was thinking that ExpectationTarget would check both ways (e.g. if a value is passed, check if the matcher supports that and if a block is passed, check if the matcher supports a block). Now I'm thinking we really only need the block check. expect(value).to raise_error is essentially a type error -- the user has passed the wrong type of object to match against raise_error, and in general, it's the matcher's job to provide a good failure message when the wrong type of object is provided. Thus, I don't think it's the job of ExpectationTarget to do any checks when passed a value argument. OTOH, expect { foo.bar }.to_not be_nil is not a type error: the proc passed to be_nil is a valid type of an object for that matcher to operate against, but that's not what the user intends. This is the case that causes confusion for users that we want to provide a good error message for.

This has some nice implications:

• The interface is simpler: having an API that can return a symbol or an array of symbols is kinda messy. A boolean predicate is much easier to understand.
• @JonRowe was concerned about adding an extra method call to matcher_type for every expectation, but under this schema in the common case (a value expectation) no extra call would be done. It would only happen in the less common case (a block expectation).

Any objections?

[JonRowe]

I like this plan.

[myronmarston]

Cool, I'll take a stab at this soon.

[cupakromer]

I too think this is a good approach.

[myronmarston]

Closing as #530 and #535 are ready for review and merge and they will solve this.