Unexpected target/handler behavior with respect to blocks

[laserlemon]

I would expect the following two assertions to behave similarly, at least to the point that they would both pass, but they don't.

expect("foo").to eq("foo")
expect { "foo" }.to eq("foo")

In the same vein, I would expect these two assertions to behave differently but they are identical.

expect { "foo" }.to eq("foo")
expect(proc { "foo" }).to eq("foo")

It seems that the handoff between the target and the handler could be improved by somehow providing context/insight into whether the incoming "actual" value is something dynamic to be evaluated (a given block) or a static value (even a proc) given as an argument.

I'm hoping it's worth a conversation. Thank you!

[laserlemon]

Right now, the decision of whether to call the incoming "actual" value as a proc is left up to the specific matcher (eq vs. change) but the expectation target actually has the best information as to what the developer intends.

[myronmarston]

We've actually just addressed this in #530 (based on discussion in #526). expect { "foo" }.to eq("foo") now fails with:

You must pass an argument rather than a block to use the provided matcher (eq "foo"),
or the matcher must implement `supports_block_expectations?`.

[myronmarston]

Try master (soon to be released on 3.0.0.rc1) and see if that meets your needs.

[laserlemon]

I'm so bummed I missed the conversation!

It seems like the responsibility of interpreting user intent has been put on the matcher, when the expectation target has the distinct ability to differentiate a block and a proc argument.

But you've already had lengthy conversation about this, so I'll defer to the group wisdom rather than beat a 💀 🐴. Thank you for the quick reply!

[myronmarston]

It seems like the responsibility of interpreting user intent has been put on the matcher,

The matcher has no idea of user intent: it just indicates whether or not the creator of the matcher intends it to be usable in a block expectation expression. If the creator of the matcher does not intend that usage, than passing expect { }.to matcher isn't going to behave as the user expects since the matcher isn't designed to work with blocks.

when the expectation target has the distinct ability to differentiate a block and a proc argument.

The expectation target does differentiate a block vs a proc: expect(lambda { }) is treated differently than expect { }. now.

Note that one complexity of this (that's subtle) is that it's essentially impossible to make an expectation expression like expect { nil }.to be_nil do what the user expects: the existing matcher protocol provides a single matches?(actual) method that has no way to signal to the matcher if a block was provided vs an argument. Given that, the expectation handler has to package the block up as a proc so it can pass it as an argument to matches?. Given that, BeNil#matches?(Proc.new { nil }) correctly returns false because the provided argument is not nil, even though the proc, when called, returns nil. The proc is an object like any other and it is not nil.

[laserlemon]

Might the BlockExpectationTarget evaluate the block first and pass the resulting value if the specified matcher doesn't support blocks?

[JonRowe]

Might the BlockExpectationTarget evaluate the block first and pass the resulting value if the specified matcher doesn't support blocks?

It could, but we'd rather not support that use case, there's no advantage in passing a block to expect in that circumstance.

[myronmarston]

It's never been something we've supported and I don't see a reason to start. IMO, it will cause more confusion...if we supported that, it would suggest to the user that passing an arg vs a block are equivalent, since this would then work:

expect(3).to eq(3)
expect { 3 }.to eq(3)

...but this wouldn't:

expect { do_something }.to raise_error
expect(do_something).to raise_error

I think that to use the block expectation matchers properly, it's important for users to understand that what they are matching on can't be represented as a simple value, and your suggestion makes it harder to understand that.

[laserlemon]

Thank you for the feedback. I have a better understanding of your approach now. Good luck with 3.0!

[SaimonL]

expect { do_something }.to raise_error

That above code works fine form me.
Keep in mind that if your code is catching exception then the above spec WILL NOT work.

For an example:

expect {
  begin
    MultiJson.load('{invalid json}')
  rescue MultiJson::ParseError => exception
    exception.data # => "{invalid json}"
    exception.cause # => JSON::ParserError: 795: unexpected token at '{invalid json}'
  end
}.to raise_error

[mixelpixel]

How does one get the rspec matcher to implement supports_block_expectations??

[myronmarston]

How does one get the rspec matcher to implement supports_block_expectations??

Just define it as a method inside your matcher definition, e.g.

class MyMatcher
  def supports_block_expectations?
    true
  end

  # ...
end

If you are using the matcher definition DSL, you can just call supports_block_expectations:

RSpec::Matchers.define :my_matcher do
  supports_block_expectations

  # ..
end

[mixelpixel]

hi @myronmarston , thanks for the reply - I'm fairly new to Ruby and rspec. I really appreciate the helpful info!

[n-epifanov]

@myronmarston, thanks for the fix, though

You must pass an argument rather than a block to use the provided matcher (eq "foo"),
or the matcher must implement `supports_block_expectations?`.

is a really confusing message. It made me think eq got a block but needs an argument. Couldn't figure it out until googled up this issue.

[myronmarston]

@NickTime what is your code snippet that generated that message?

[n-epifanov]

@myronmarston https://github.com/nicktime/tomograph/blob/add-json-support/spec/lib/tomograph/api_blueprint/yaml_spec.rb#L70
when put in a curly braces like this

      expect { subject.to_tomogram.map(&:to_hash) }.to eq(tomogram_hash)

So I was thinking the problem is with tomogram_hash.

[JonRowe]

Do you have a suggested wording improvement? It means expect was given a block but eg(tomogram_hash) needs a value

[n-epifanov]

I'd suggest

You must pass an argument rather than a block to `expect` to use the provided matcher (eq "foo"),
or the matcher must implement `supports_block_expectations?`.

Confusion is only about which method it is that can't take a block, otherwise message is clear.

[JonRowe]

Good improvement, would you like to tackle it in a PR?

[n-epifanov]

If it's just updating text then yes, here it is #1066

[JonRowe]

Thanks :)