Let's change the default for RSpec/PredicateMatcher

[marcandre]

RSpec/PredicateMatcher tells me to prefer the first form to the second one:

    context 'given some result' do
      let(:error) { 'Oops, a few things blew up and nothing worked' }

      it 'check with nil?' do
        expect(error.nil?).to be(true)
      end

      it 'check with be_nil' do
        expect(error).to be_nil
      end
    end

This is gravely mistaken. It's longer, it's imo less clear, but much more importantly a failure (like in this example) gives uninformative info:

  1) some example given some result check with nil?
     Failure/Error: expect(error.nil?).to be(true)
     
       expected true
            got false

  2) some example given some result check with be_nil
     Failure/Error: expect(error).to be_nil
     
       expected: nil
            got: "Oops, a few things blew up and nothing worked"

Notice how the second failure is much more informative, showing the unexpected error and all. That is the reason to always use precise matchers.

I don't mind if we keep the functionality for the (imo always bad) style, I don't care if we propose be_nil or eq(nil), but the default should change.

Let's minimize the reasons to curse at RuboCop.

[pirj]

We don't have a formal checklist for changing the defaults, but if I had to come up with it, it would be:

• make sure to only change defaults on major version updates
• check settings for that cop in
  • https://github.com/eliotsykes/real-world-rails
  • https://github.com/jeromedalbert/real-world-ruby-apps
• check with the style guide https://rspec.rubystyle.guide/ (https://rspec.rubystyle.guide/#predicate-matchers)

It would be really nice to harmonize the defaults with the style guide and to harmonize the style guide with what is really commonly used in those open-source apps. It looks like a task on its own, but before we've done that, we can follow that "simple" checklist.

Would you like to grep through .rubocop.yml settings for PredicateMatcher of real-world-rails/real-world-ruby-apps? Part of them doesn't use RuboCop or RSpec, unfortunately, but some can be used to make a judgememnt on this topic.

[marcandre]

FWIW, I think be_truthy/be_falsey should be forbidden in a different cop. I can think of no circumstance where they should be used.

[pirj]

be_truthy/be_falsey should be forbidden in a different cop

Completely agree.

Splitting cops ☝️

be_truthy/be_falsey ... can think of no circumstance where they should be used.

If we are talking about predicates - sure, but there are certain cases when it's better to be less strict about expectations.

In general, we recommend you use the loosest matcher that still specifies the behavior you care about.

• Effective Testing with RSpec 3

The need can be implied by a weird interface or even some weirdness in the Ruby itself:

0.zero?
# => true
0.nonzero?
#=> nil
1.zero?
#=> false
1.nonzero?
#=> 1

https://metaredux.com/posts/2019/06/11/weird-ruby-zeroing-in-on-a-couple-of-numeric-predicates.html

[marcandre]

The need can be implied by a weird interface or even some weirdness in the Ruby itself:

0.zero?
# => true
0.nonzero?
#=> nil
1.zero?
#=> false
1.nonzero?
#=> 1

The "weirdness" of nonzero? is misconceived. Probably simply because of the "?". It would also a horrible use of be_truthy (that it returns the receiver and not just a truthy value is paramount in (a <=> b).nonzero? || (c <=> d) for example) or be_falsy (it never returns false).

there are certain cases when it's better to be less strict about expectations.

Legitimate ones? I'll still need to see one be convinced 😸

[pirj]

Hold on for a second. What are the PredicateMatcher configuration where it tells the prefer expect(error.nil?).to be(true) over expect(error).to be_nil? Is it the default setting?

With the default, which is Strict: true, EnforcedStyle: inflected (default) I don't get any offences in your code.

With EnforcedStyle: explicit there's "Prefer using nil? over be_nil matcher" (no matter what Strict is set to.
With EnforcedStyle: inflected Strict: false there's "Prefer using be_nil matcher over nil?".
It's strange, I would expect it to raise an offence with Strict: true as well. Might be a bug.

So are you talking about this bug, or do you propose to retire/deprecate the explicit option altogether? This sounds different from this ticket's title.

Regarding be_falsey/be_truthy there are quite some examples in RSpec's code itself [be_falsey, be_truthy]. Are you up to make those specs better as an example of your point?

[marcandre]

The reason these are in rspec is because these matchers used to be called be_false and be_true which was very confusing as most people thought they meant be(false) and be(true)
When these were (rightfully) renamed, they did the easy thing: rspec/rspec-core@eb5bb45a09

I don't think I needed a PR as an example of my point, but here it is nevertheless: rspec/rspec-core#2736

[pirj]

I'm sorry I didn't check properly initially, and it's good that we've researched this.
So, from my point of view:

1. The code example expect(foo).to be(true) is incorrect
2. For the code in the description, the cop with default settings should have raised "Prefer using be_nil matcher over nil?" but it didn't
3. We may consider splitting the cop into one that takes care of predicates, and another that keeps an eye on be_falsey/be_falsy/be_truthy (check related Deprecate expect { }.not_to raise_error(SpecificErrorClass) rspec/rspec-expectations#244, Determine why (and patch) build fails on rbx-head rspec/rspec-expectations#176 and Properly sub-class Exception so YARD can detect ExpectationNotMetError. rspec/rspec-expectations#693) - it would be nice to clean this all up with a massive sweep

Do you think there is anything else actionable? Are you happy with the current default settings?

PS I deeply respect your persistence and strong opinions.

[marcandre]

1. The code example `expect(foo).to be(true)` is incorrect

Yes, I'm sure expect(foo.something?) was meant.

2. For the code in the description, the cop with default settings should have raised "Prefer using be_nil matcher over nil?" but it didn't

Should prefer foo).to be_nil over foo.nil?).to be(true), without a doubt.

3. We may consider splitting the cop into one that takes care of predicates, and another that keeps an eye on `be_falsey`/`be_falsy`/`be_truthy` (check related rspec/rspec-expectations#244, rspec/rspec-expectations#176 and rspec/rspec-expectations#693) - it would be nice to clean this all up with a massive sweep

Absolutely. I can imagine many people wanting to avoid be_truthy but being ok with predicate?).to be(true)

Do you think there is anything else actionable? Are you happy with the current default settings?

Besides enforcing expect(predicate?).to be(true) => to be_predicate, I'm sure there are many other uses of be(true) that are "incorrect" in my book. The vast majority of specs with be(true) can be improved; something the API is at fault (if one defines foo instead of foo?), or a better matcher can be used.

There are very few legitimate uses of be(true) and be(false) I can think of:

expect { foo }.to change { bar? }.to be(true)
expect(foo?(args)).to be(true)

subject(:enabled?)
it { is_expected.to be(true) }

Note that even foo?(arg) should probably be turned into a matcher. We have a lot of:

expect(described_class.match_path?('c{at,ub}s', 'cubs')).to be(true)
# should be
subject { described_class }
is_expected to.match_path('c{at,ub}s', 'cubs')

Edit: Actually I just realized that automagical predicate matchers accept arguments. So be_match_path would work, if a bit odd,

Other builtin matchers

I didn't check if there's a cop for this, but I see a lot of cases where builtin matchers are not used.
eg.:

expect(word_regexp.is_a?(::Regexp)).to be(true)
# should be
expect(word_regexp).to be_a(::Regexp)

Arrays are a typical example:

expect(ary.size).to be(3)
# should be
expect(ary).to have(3).items

What's worse is that then the elements are tested one after the other:

expect(ary.size).to be(3)
expect(ary[0]).to some_matcher
expect(ary[2]).to some_other_matcher
# should be
expect(ary).to match_exactly [
  some_matcher,
  anything,
  some_other_matcher,
] # Note: size checked implicitly

When that fails, we have the whole array we can compare.

Yes, I put the trailing comma out of spite, that's another default I disagree with 😆

PS I deeply respect your persistence and strong opinions.

Thank, I hope I don't come out too strong to others.

[pirj]

I'm working on a pull request (not pushed yet) to address the issues of the PredicateMatcher.

1. fixed

For the code in the description, the cop with default settings should have raised "Prefer using be_nil matcher over nil?" but it didn't.

Should prefer foo).to be_nil over foo.nil?).to be(true), without a doubt.

It turns out that the default Strict: true, EnforcedStyle: inflected (default) means that a expect(foo).to be_something style is generally preferred, but "strict" checks, e.g. be(true), eq(true) etc are tolerated:

# also good - It checks "true" strictly.
expect(foo.something?).to be(true)

This goes against https://rspec.rubystyle.guide/#predicate-matchers that states "strict" usages as a bad example:

# bad
expect(subject.published?).to be true

To add even more controversy, quoting Effective Testing with RSpec again:

In general, we recommend you use the loosest matcher that still specifies the behaviour you care about.

So from my understanding, "strict" is worse than be_falsey/be_truthy, as it's excessively specific when applied to predicate matchers, e.g. if they happen to return nil which can be considered as "not defined, but presumably false".

To be fair, be_truthy is mentioned in this guideline https://rspec.rubystyle.guide/#be-matcher as one of the allowed replacements for be matcher without arguments passed.
not_to be_nil arguably sounds better, but it misses the case when the SUT is false. And it's less generic than be_truthy.

I guess the justified predicate-look-alike methods like nonzero? are scarce (unjustified example).

3. I'm going to leave the above problem with "strict" ☝️ as-is, probably providing different defaults for separated cops basing on real-world-ruby/real-world-rails usage statistics.
   Since RuboCop 1.0 is about to happen, we may bump our major version too, and shamelessly set the new defaults.

To be continued...

[marcandre]

In general, we recommend you use the loosest matcher that still specifies the behaviour you care about.

Right, I agree with that quote, but only in the context of applications. For gems I believe that the behavior we care about is the exact behavior so specs should be quite strict.

[marcandre]

Just got more autocorrections for:

expect(something).to be_a(SomeClass)
# corrected to
expect(something.is_a?(SomeClass)).to be(true)

That's also equally bad, for the same reasons as the rest.

[pirj]

In general, we recommend you use the loosest matcher that still specifies the behaviour you care about.

Right, I agree with that quote, but only in the context of applications. For gems I believe that the behavior we care about is the exact behavior so specs should be quite strict.

Even though I agree that gems should be more specific in regards to their public API, they may choose to be more relaxed when testing private APIs.
Anyway, rubocop-rspec is not only for gems, it's also for applications.

[pirj]

corrected to

expect(something.is_a?(SomeClass)).to be(true)

That's also equally bad, for the same reasons as the rest.

I'm not sure I understand. Do you think is_a? should be yet another exception to the explicit style? It's getting pretty complicated.

[marcandre]

may choose to be more relaxed when testing private APIs.

They may. I'll disagree with the fact that they are testing private APIs 😆.

[marcandre]

I'm not sure I understand. Do you think is_a? should be yet another exception to the explicit style? It's getting pretty complicated.

Sorry, I haven't looked into the cop, or its settings. I don't know what the explicit style is, or what it should be.

All I know is that expect(something).to be_a(SomeClass) should be the preferred form, which is not the case currently as RuboCop enforces the other form.

To summarize, my position is that, by default (or at least in RuboCop):

• be_falsey / be truthy => never
• be(true) / be(false) => use predicate matchers instead
• most builtin matchers (like be_a, match_array, have(n).items, I'm of course excluding be_falsey / be_truthy) should always be preferred.

I have no informed opinion as to what the settings should be named / how the cops should be split. Just that the defaults are horribly wrong for RuboCop.

[pirj]

To avoid any ambiguity, I didn't mean testing private methods.
I meant private API that is callable but not by regular gem's consumers. E.g. RuboCop's ConfigLoader or ConfigValidator. I don't want to dive into unit vs acceptance discussion, usually, it's a balance of the two, and that means some generic expectations can take place even in gem testing code pretty legitimately.

[mockdeep]

I actually disagree with the quote:

In general, we recommend you use the loosest matcher that still specifies the behaviour you care about.

I think it's best to specify things as tightly as possible. It's too easy for subtle bugs to sneak into code when we aren't precise in what we expect. That being said, I do find the be_ syntax more readable. I wish it was stricter in terms of checking for true and false rather than truthy and falsey.

My thinking on it at this point is basically that we should be explicit if it is the predicate method under test. This ensures the method returns true or false and not just something truthy or nil. This is especially important if we're serializing something to send over the wire. And I would like to use the be_ matchers otherwise:

# good
describe '#foo?' do
  it 'returns false' do
    expect(thing.foo?).to be(false)
  end

  it 'returns true when fooed' do
    thing.foo!

    expect(thing.foo?).to be(true)
  end
end

describe '#foo!' do
  it 'makes it foo' do
    thing.foo!

    expect(thing).to be_foo
  end
end

# bad
describe '#foo?' do
  it 'returns false' do
    expect(thing).not_to be_foo
  end

  it 'returns true when fooed' do
    thing.foo!

    expect(thing).to be_foo
  end
end

describe '#foo!' do
  it 'makes it foo' do
    thing.foo!

    expect(thing.foo?).to be(true)
  end
end

That's a little hard to lint, though, so we generally go with the explicit approach.

[marcandre]

I was convinced that expect(thing).to be_foo was checking for == true but it isn't. 🤯
The doc really seem to imply the contrary, and the error messages too!

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

That's also the behavior I believe it should have.

I'll open an issue. If they agree it's a bug, we'll fix it, otherwise I'll propose a setting for tight predicate matchers that we should turn on.

I still much prefer the error messages with these matchers

[mockdeep]

I think it would break a lot of tests if RSpec were to change it. Maybe they could add a configuration for strict_predicates or something.

[marcandre]

I think it would break a lot of tests if RSpec were to change it.

I'm very curious about that, but if so it might hide bugs.

Maybe they could add a configuration for strict_predicates or something.

I proposed just that in your issue

[pirj]

- expect(ary.size).to be(3)
+ expect(ary).to have(3).items

IIRC, have is from rspec-collection_matchers.
Out of the box, this works:

- expect(ary.size).to be(3)
+ expect(ary).to have_attributes(size: 3)

but for me personally it looks like a bad replacement.

[pirj]

- expect(ary.size).to be(3)
- expect(ary[0]).to some_matcher
- expect(ary[2]).to some_other_matcher
+ expect(ary).to match_array [
+  some_matcher,
+  anything,
+  some_other_matcher,
+ ] # Note: size checked implicitly

This appeals to me and I'm using this a lot with include and have_attributes, but unfortunately, contain_exactly and match_array are order-independent.
Using argument matchers with them also induces a problem of combinatorial comparison:

    expect([1, 2, 3]).to match_array([be.>(2), be_even, eq(1)])

In this case, eq works:

expect(ary).to eq([
  some_matcher,
  anything,
  some_other_matcher,
]) # Note: size checked implicitly

[marcandre]

contain_exactly and match_array are order-independent.

Oh, indeed, my bad. Your example is what I should have written.

[pirj]

Let me start with splitting the spec file into strict and non-strict, it will pave the way to splitting the cop.
We can iteratively spawn new cops or extend existing (and contained!) cops to cover what have discussed above.

[pirj]

I've discovered three two completely valid options for correction of:

    expect([-1,2,3].all? { |x| x.positive? }).to be(true)

The difference is in the failure message.
For the original:

     Failure/Error: expect([-1,2,3].all? { |x| x.positive? }).to be(true)
       expected true
            got false

    expect([-1,2,3]).to be_all { |x| x.positive? }

     Failure/Error: expect([-1,2,3]).to be_all { |x| x.positive? }
       expected `[-1, 2, 3].all?` to return true, got false

2.

    expect([-1,2,3]).to all be(&:positive?)

it's surprisingly a success. I'll file an idea ticket for a cop and a ticket for RSpec to emit a warning/fail if a block is passed to be.

    expect([-1,2,3]).to all be_positive

     Failure/Error: expect([-1,2,3]).to all be_positive

       expected [-1, 2, 3] to all be positive

          object at index 0 failed to match:
             expected `-1.positive?` to return true, got false

The latter seems to be the most informative.

[marcandre]

Good catch on the block 👍

expect([-1,2,3]).to all be_positive

Definitely the best 🎉

[schwern]

To summarize, my position is that, by default (or at least in RuboCop):

* `be_falsey / be truthy` => never

I disagree about be_falsey. There are many situations where be_false would be over-specifying.

I write if !thing not if thing == false because functionally I'm not going to do anything different with a nil vs false. If there is a functional difference between nil and false, the method should be throwing an exception rather than returning nil.

Another example is when the data or code is not entirely under my control. If I'm receiving JSON from an API if it's false today it might be nil tomorrow. I specify it's just false. Similarly, if I'm calling a method in someone else's class gem I do not trust they will be careful about nil vs false. I don't want to have to sanitize every return value with return call_someone_elses_method ? true : false.

Similar for be_truthy. Sometimes I don't care what it is, just that it's true (ie. not false). There may be an argument I should be using something more strict like be_a but sometimes even that is outside the spec's concern.

Perhaps this is a difference between writing application code (be lax in what you receive) and gem code (be strict in what you send).

One thing is clear: be_truthy/be_falsey should be its own cop separate from PredicateMatcher. Over-specifying makes it more likely the cop will be disabled.

[marcandre]

I write if !thing not if thing == false

Agreed, usually it doesn't matter. But it might, e.g. thing&.something?, or collection.group_by(&:thing).

Perhaps this is a difference between writing application code (be lax in what you receive) and gem code (be strict in what you send).

Possibly, but typically you shouldn't spec what you receive. Note that I wrote "at least for RuboCop"

Another example is when the data or code is not entirely under my control

I'm curious as to why you'd want to have specs for that.

be_truthy/be_falsey should be its own cop

👍

[marcandre]

@pirj have you been able to make progress on this front?

[pirj]

@marcandre Not much code-wise. Started splitting up the specs to allow to split cops. https://github.com/rubocop-hq/rubocop-rspec/compare/predicate-matcher-streamline-specs

Adding descriptive tickets with thoughts that born in the discussion above.
Do you think it makes sense to make it a priority right now? I'm afraid to be late for RuboCop 1.0/RuboCop RSpec 2.0 with all that amount of typing.

[marcandre]

I wouldn't want to impose on your priorities; it's only an irritant everytime I see specs like this, so I was just curious.

Definitely way higher priority than this though! 😆

[pirj]

Indeed! 😆
I had to dump my notes before I forgot what I meant when wrote them XD

[schwern]

I gave it some thought and came to this conclusion: be false vs be_falsey is not going to materially change the effectiveness of my specs, but it did force me to think about what false vs nil means. false is in response to a query, nil means "nothing". This is a clarity I did not have before.

I did find some value in examining be_truthy. Usually it should be be true, but often it's a stand-in for a more specific matcher like be_a(...).

Of 5579 examples there's roughly 150 variations of be_falsey, be_truthy, be nil, be false, be true. The concerns I had about API wobble don't seem to be evident. This is not an overwhelming amount to review, but the opportunity cost is high. I would disable the cop in this existing project, but use it in a fresh project.

What I'm finding using Rubocop is when the default settings seem to be over-specified, rather than switching them off, sometimes its teaching me a lesson. However, the rationale is rarely documented and I have to puzzle it out for myself. I would ask that the rationale for the default styles be documented, it would speed up the process.

be_truthy/be_falsey should be its own cop

👍

👍

[schwern]

An additional caveat for RSpec/PredicateMatcher. I found it makes refactoring harder, and its harder for my junior to understand the specs.

My junior programmer followed Naming/PredicateName to change is_foo? to foo?, which is good. They did a simple search and replace. The specs failed because they missed be_is_foo. 😿 They didn't understand why because predicate matchers confuse them. expect( thing ).to be_foo adds another layer between the spec and the code.

I like the concept of predicate matchers, but I'm hitting a lot of caveats.

[pirj]

No doubt there's less magic with the explicit style, and Naming/PredicateName is obviously is not aware of and is missing predicate matchers. It's not the default, but you can opt for the explicit style of RSpec/PredicateMatcher to avoid confusing your juniors @schwern.

the rationale [of the default settings] is rarely documented and I have to puzzle it out for myself. I would ask that the rationale for the default styles be documented

Cop class docs are later published as cop docs.
If you've been this process of figuring out the rationale for some cops, would you please send pull requests to add the rationale for picking a specific config default to cops' docs? This is totally welcome.

[marcandre]

An additional caveat for RSpec/PredicateMatcher. I found it makes refactoring harder, and its harder for my junior to understand the specs.

Interesting anecdote. It's clear this involves a bit of cognitive load. What I like about it is there was very little cost in learning it, in that the spec failed the minute the predicate was renamed. Your junior learned something and will now be able to write specs this way. If ever the renaming was part of a bigger commit (and thus harder to understand), your junior hopefullly learned to break his changes in smaller refact commits 😆

[bquorning]

expect( thing ).to be_foo adds another layer between the spec and the code.

I agree with this sentiment. I tend to use RSpec’s built-in matchers, but stay away from the more “magic” dynamic predicate matchers. And the reduced “greppability” is a big factor here. Probably I miss out on some better error messages, but in my opinion (at least in the code base I most commonly work on) it’s a tradeoff I‘m willing to make.

[marcandre]

@pirj any progress? I had a momentary lapse and wrote expect(some_array).to be_empty and got RSpec/PredicateMatchercomplaining again. Definitely not changing this toexpect(some_array.empty?).to eq true`

[pirj]

No progress here.

Your case is strange.
With the default config I get this:

RSpec.describe do
  it do
    expect(some_array).to be_empty
    expect(some_array.empty?).to be_truthy
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RSpec/PredicateMatcher: Prefer using be_empty matcher over empty?
    expect(some_array.empty?).to be(true)
  end
end

Can you check? It doesn't seems to behave as with the default setting for PredicateMatcher.

[marcandre]

@pirj Oh, you're right, I didn't realize that rubocop-ast had a customized setting for that 🤦‍♂️

As a matter of apology I opened #1040 😅 I'll need to find another way to apologize 😆

[pirj]

No need to apologize, really :D

[pirj]

Getting back to this 2 years later.

rubocop-ast had a customized setting for that

Right, this has its roots in rubocop's RuboCop config. Apparently, this setting has caused a lot of confusion at the beginning of this discussion 😄

Testing inflected with Strict: true on real-world-rspec:

57497 files inspected, 5959 offenses detected, 5959 offenses autocorrectable

inflected with Strict: false (expectedly more offences, as it doesn't tolerate strict be(true)-like matchers):

57497 files inspected, 16259 offenses detected, 16259 offenses autocorrectable

Ouch, nearly 3x more offences.

My plan is to:

1. Add an RSpec/StrictMatchingForPredicates cop that would enforce be_truthy vs be(true) and the other way around, with the default loose style.
2. Remove the Strict option from RSpec/PredicateMatchers, and default to not tolerating be(true)-like "strict" matchers.

With that change, there won't be any direct replacement for the current default.

Do you agree with this course of action, @bquorning, @Darhazer ?

[mculp]

We recently upgraded to the new defaults and have like ~1800 eq true|false that need to be changed to be true|false. ~1400 be true|false by comparison. A coworker created a PR, and the conflicts are just growing and growing 🤣

I think we're going to have to come up with a migration plan.

Or maybe the author will find a time in the middle of the night to merge it.