Add new RSpec/PredicateMatcher cop

[pocke]

Ref #83 #176

This cop is configurable. See this documentation for more details https://github.com/pocke/rubocop-rspec/blob/421baf13936f57c7a97eda09b4ee5f86d2c8025d/lib/rubocop/cop/rspec/predicate_matcher.rb#L232-L268

[backus]

Wow this is amazing! You did this in one day? I'll try to review this beast now

[backus]

I use eql a lot so I'd love an :eql here too

[pocke]

👍
And I guess I should add equalalso.

[backus]

rewritten

[backus]

@pocke a few correctness notes that I noticed after running this on a private project.

I configured the cop with

RSpec/PredicateMatcher:
  Strict: true
  EnforcedStyle: explicit

and ran autocorrect. A lot of tests failed but only for a few reasons it seems:

$ rspec --only-failures > rspec_output.log
$ ack "undefined method \`(.+)'" rspec_output.log --output='$1' | sort | uniq -c | sort -n
      9 downcase
     14 an_instance_of?
     34 has_received?
    357 has_attributes?

1. RSpec's have_attributes should not be autocorrected to has_attributes?

2. Similarly, have_received for spies shouldn't become .has_received?

3. Watch out for RSpec's aliases. For example we had some instances of be_an_instance_of which got inflected to .an_instance_of?. You can grep out most of RSpec's custom aliases pretty easily:

$ ack --no-filename '^\s+alias_matcher :(\w+),\s+:(\w+)' lib
    alias_matcher :a_truthy_value, :be_truthy
    alias_matcher :be_falsy,       :be_falsey
    alias_matcher :a_falsey_value, :be_falsey
    alias_matcher :a_falsy_value,  :be_falsey
    alias_matcher :a_nil_value, :be_nil
    alias_matcher :a_value, :be, :klass => AliasedMatcherWithOperatorSupport
    alias_matcher :an_instance_of, :be_an_instance_of
    alias_matcher :a_kind_of,  :be_a_kind_of
    alias_matcher :a_value_between, :be_between
    alias_matcher :a_value_within, :be_within
    alias_matcher :within,         :be_within
    alias_matcher :a_block_changing,  :change
    alias_matcher :changing,          :change
    alias_matcher :a_collection_containing_exactly, :contain_exactly
    alias_matcher :containing_exactly,              :contain_exactly
    alias_matcher :a_range_covering, :cover
    alias_matcher :covering,         :cover
    alias_matcher :a_collection_ending_with, :end_with
    alias_matcher :a_string_ending_with,     :end_with
    alias_matcher :ending_with,              :end_with
    alias_matcher :an_object_eq_to, :eq
    alias_matcher :eq_to,           :eq
    alias_matcher :an_object_eql_to, :eql
    alias_matcher :eql_to,           :eql
    alias_matcher :an_object_equal_to, :equal
    alias_matcher :equal_to,           :equal
    alias_matcher :an_object_existing, :exist
    alias_matcher :existing,           :exist
    alias_matcher :an_object_having_attributes, :have_attributes
    alias_matcher :having_attributes,           :have_attributes
    alias_matcher :a_collection_including, :include
    alias_matcher :a_string_including,     :include
    alias_matcher :a_hash_including,       :include
    alias_matcher :including,              :include
    alias_matcher :match_regex,        :match
    alias_matcher :an_object_matching, :match
    alias_matcher :a_string_matching,  :match
    alias_matcher :matching,           :match
    alias_matcher :a_block_outputting, :output
    alias_matcher :a_block_raising,  :raise_error do |desc|
    alias_matcher :raising,        :raise_error do |desc|
    alias_matcher :an_object_responding_to, :respond_to
    alias_matcher :responding_to,           :respond_to
    alias_matcher :an_object_satisfying, :satisfy
    alias_matcher :satisfying,           :satisfy
    alias_matcher :a_collection_starting_with, :start_with
    alias_matcher :a_string_starting_with,     :start_with
    alias_matcher :starting_with,              :start_with
    alias_matcher :a_block_throwing, :throw_symbol do |desc|
    alias_matcher :throwing,        :throw_symbol do |desc|
    alias_matcher :a_block_yielding_control,  :yield_control
    alias_matcher :yielding_control,          :yield_control
    alias_matcher :a_block_yielding_with_no_args,  :yield_with_no_args
    alias_matcher :yielding_with_no_args,          :yield_with_no_args
    alias_matcher :a_block_yielding_with_args,  :yield_with_args
    alias_matcher :yielding_with_args,          :yield_with_args
    alias_matcher :a_block_yielding_successive_args,  :yield_successive_args
    alias_matcher :yielding_successive_args,          :yield_successive_args

4. I saw some errors like undefined method downcase for {"Location"=>"/settings"}:Hash. Basically, we had a test like this:

it 'redirects to settings page' do
  expect(response_code).to be(302)
  expect(response_headers).to include('Location' => '/settings')
end

in this case, response_headers is a Rack::Utils::HeaderHash which assumes that the value is a string and calls downcase on it. This wouldn't happen in most cases where #include is called with a Hash but it would still fail the test I believe (expect(some_hash).to include(a: 1) is not the same as expect(some_hash.include?(a: 1)).to be(true)

---

This cop is looking really great! Keep up the good work 😄. I'll probably keep reviewing and commenting a bit but just ping me when you've fixed these cases

[pocke]

Thank for your comment!

RSpec's have_attributes should not be autocorrected to has_attributes?

Similarly, have_received for spies shouldn't become .has_received?

👍

Watch out for RSpec's aliases. For example we had some instances of be_an_instance_of which got inflected to .an_instance_of?. You can grep out most of RSpec's custom aliases pretty easily:

👍 I think this cop should be aware of be_* aliases. Because the cop may report false positive by the aliases.

I believe (expect(some_hash).to include(a: 1) is not the same as expect(some_hash.include?(a: 1)).to be(true)

Definitely.

describe do
  it 'include' do
    hash = {a: 1, b: 2}
    expect(hash).to include a: 1
  end

  it 'include?' do
    hash = {a: 1, b: 2}
    expect(hash.include?(a: 1)).to be true
  end
end

$ rspec a.rb


  include
  include? (FAILED - 1)

Failures:

  1) include?
     Failure/Error: expect(hash.include?(a: 1)).to be true

       expected true
            got false
     # ./a.rb:9:in `block (2 levels) in <top (required)>'

Finished in 0.01871 seconds (files took 0.15634 seconds to load)
2 examples, 1 failure

Failed examples:

rspec ./a.rb:7 # include?

And RSpec documentation mentions it.

The matcher also provides flexible handling for hashes:
{:a => 1, :b => 2}.should include(:a)
{:a => 1, :b => 2}.should include(:a, :b)
{:a => 1, :b => 2}.should include(:a => 1)
{:a => 1, :b => 2}.should include(:b => 2, :a => 1)
{:a => 1, :b => 2}.should_not include(:c)
{:a => 1, :b => 2}.should_not include(:a => 2)
{:a => 1, :b => 2}.should_not include(:c => 3)
https://relishapp.com/rspec/rspec-expectations/v/2-0/docs/matchers/include-matcher

[pocke]

@backus updated!

[backus]

Looks almost perfect! Think I found one more edge case. Try autocorrecting this

# frozen_string_literal: true

RSpec.describe Foo do
  specify do
    expect(a).to be_an_instance_of?(b)
  end
end

with

RSpec/PredicateMatcher:
  Strict: false
  EnforcedStyle: inflected

Once you've fixed that can you squash down? I think I'm good to merge after that

[pocke]

Think I found one more edge case. Try autocorrecting this

Did you mean EnforcedStyle: explicit? If run autocorrecting with the style, it generates invalid code.

# frozen_string_literal: true

RSpec.describe Foo do
  specify do
    expect(a.an_instance_of??(b)).to be_truthy
  end
end

I fixed the bug, I made this cop not to add offense if matcher name ends with a ?.

diff --git a/lib/rubocop/cop/rspec/predicate_matcher.rb b/lib/rubocop/cop/rspec/predicate_matcher.rb
index e67a604..1079c96 100644
--- a/lib/rubocop/cop/rspec/predicate_matcher.rb
+++ b/lib/rubocop/cop/rspec/predicate_matcher.rb
@@ -163,7 +163,8 @@ module RuboCop
         def predicate_matcher_name?(name)
           name = name.to_s
           name.start_with?('be_', 'have_') &&
-            !BUILT_IN_MATCHERS.include?(name)
+            !BUILT_IN_MATCHERS.include?(name) &&
+            !name.end_with?('?')
         end
 
         def message_explicit(matcher)

And squashed. Thanks!

[backus]

Whoops sorry I messed up my example. This is the before case:

# frozen_string_literal: true

RSpec.describe Foo do
  it 'works great' do
    expect(a.instance_of?(b)).to be(true)
  end
end

and this is what happens when you autocorrect

# frozen_string_literal: true

RSpec.describe Foo do
  it 'works great' do
    expect(a).to be_an_instance_of?(b)
  end
end

[pocke]

I fixed the autocorrection bug, it was a typo.

[backus]

Awesome cop! Really great job 😄

[backus]

Thanks for this awesome cop @pocke!