Strict predicate matchers

[mockdeep]

Subject of the issue

It would be great to have a configuration for predicate matchers to be "strict" in the sense that they require the return result to be a boolean and not something that is truthy or falsey. It's easy for these methods to have values that creep their way to the client side in tricky to debug ways, so we try as much as possible in our codebase to enforce that they always return true or false. Right now we avoid the predicate matchers and instead use expect(thing.there?).to be true.

Proposed Solution

RSpec.configure do |config|
  config.expect_with :rspec do |expectations|
    expectations.strict_predicate_matchers = true # defaults to `false`
  end
end

class MyClass
  def there?
    'yep'
  end
end

it 'returns true' do
  expect(MyClass.new).to be_there
end

The above test should fail with a message 'yep' instead of true.

There should be no changes in behaviour if strict_predicate_matchers is explicitly set to false or the default value is used.

[JonRowe]

Hi, rspec-core contains no matchers, so this was the wrong repo to suggest this, however its well documented that rspec-expectations predicate matchers obey Ruby's truthiness rules, and I'm not sure I want to change that. We have the option to be more precise as you've mentioned, so making our matchers ambiguous seems less than ideal.

Have you considered implementing your own custom matcher that does what you want?

[mockdeep]

Yeah, we'll probably continue using the more precise syntax for now. Might end up looking into custom matchers at some point.

[marcandre]

@JonRowe Could you revisit this please?

its well documented that rspec-expectations predicate matchers obey Ruby's truthiness rules

I don't believe this is accurate. For example, the doc gives the following example:

Ruby objects commonly provide predicate methods:

    7.zero? # => false
    0.zero? # => true
    [1].empty? # => false
    [].empty? # => true
    { :a => 5 }.has_key?(:b) # => false
    { :b => 5 }.has_key?(:b) # => true

You could use a basic equality matcher to set expectations on these:

7.zero?.should == true # fails with "expected true, got false (using ==)"

...but RSpec provides dynamic predicate matchers that are more readable and
provide better failure output.

Lacking from the doc: "but with a less strict matcher corresponding to be_truthy / be_falsey (and I've stated clearly what I think of those).

All examples given returntrue / false

The doc clearly shows that the failure messages are also deceiving. When we get the following errors, how can we possibly expect the check to be truthy/falsy?

expected `obj.foo?` to return false, got 42
# or
expected `obj.foo?` to return true, got nil

and I'm not sure I want to change that.

I feel that these matchers are imprecise and error prone. A method returning "true" and true are not quite the same; one could be an unwanted effect in the code.

Assuming you still like the imprecise matchers, I would propose:

1. Specify clearly in the doc the default behavior
2. Rephrasing the error messages to say "expected obj.foo? to be falsey/truthy, got ..." instead of "to return false/true"
3. Adding a config strict_predicate_matchers that would actually do == true/false and the error messages would be the current ones.

I will glady provide a PR for this.

We have the option to be more precise as you've mentioned

It's a really bad option, as it makes the spec less readable and the error messages much less informative

[pirj]

Given the inconsistencies of our current doc with the behaviour, I'm in favour of this proposal.

Let's postpone the truthy/falsey discussion and change of the default setting for strict_predicate_matchers until the release of 3.99.

[marcandre]

change of the default setting for strict_predicate_matchers until the release of 3.99

The setting doesn't exist yet; I'm proposing that one is created. I believe the default should be true but as long as the setting exists I'll be happy. Will a PR be accepted?

[pirj]

I clearly understand that. Once this setting is added, it would be easier to discuss to flip its default, but only during a major version update.

[marcandre]

Oh, sorry, I misread then. So I'll work on a PR then, or did you want to handle it @pirj ?

[pirj]

It's up to @JonRowe to decide.

[JonRowe]

For context, I originally assessed this as a bug report and I lacked (and still lack) the time to implement anything to improve the situation, as it stands it works as designed.

However @marcandre @pirj if either of you wants to work on an opt-in config for this matcher to make it stricter I'd be happy to review it. I'm generally a fan of stricter matchers too but I definitely don't want to change the default in 3.x (breaking change). I'd be happy to add a warning in 3.99 and then change the default in 4.x but I'd also want to announce those plans early on to get consensus.

[pirj]

Awesome, thanks!

[JonRowe]

Please open a new PR when you're ready for review, this stays closed.

[pirj]

Sounds good. 👍

I've slightly changed the wording of the description so it looked more like a feature request than a bug report.

[marcandre]

🎉 Awesome 😃

@pirj let me know if you want to handle this, otherwise I'll check it out in the next few days

[pirj]

@marcandre Highly appreciate if you can take care of this, I have a reduced capacity those days unfortunately.