Prevent block-only matchers from being used with value expectation target

[pirj]

The purpose of this (rather experimental) change is to restrict the usage of:

subject { -> { do_something } }
it { is_expected.to change(something).to(new_value) }

This syntax was referenced to as:

pretty obtuse and not something I’d recommend, generally.
#805 (comment)

We should mention they’re not supported.
#805 (comment)

Problems with this approach

Specs fail quite badly mostly due to the use of this syntax in RSpec Expectations' own specs:

RSpec.describe RSpec::Matchers::BuiltIn::ChangeToValue do
  k = 0
  before { k = 0 }
  it_behaves_like "an RSpec matcher", :valid_value => lambda { k = 2 },
                                      :invalid_value => lambda { k = 3 },
                                      :disallows_negation => true do
    let(:matcher) { change { k }.to(2) }
  end
end

Would you accept this if I change those specs to something more canonical? E.g. like:

RSpec.describe RSpec::Matchers::BuiltIn::ChangeToValue do
  let(:k) { 0 }
  it_behaves_like "an RSpec matcher" do
    let(:valid_value) { k += 1 }
    let(:invalid_value) { }

    def matcher
      change { k }.from(0)
    end
  end
end

The above change didn't work from the first time, but I'll keep trying if that's the right direction.

Related Links

#526
#530
#536
#1066

https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
https://www.reddit.com/r/ruby/comments/cejl3q/call_for_discussion_rspec_implicit_block
rubocop/rspec-style-guide#76
rubocop/rubocop-rspec#789

Example fix for matcher libraries that are forced to keep the interface to provide the syntax for their users rodjek/rspec-puppet#767

[pirj]

It's possible to tell if a matcher supports block expectations, but it's not possible to detect that it doesn't support value expectations, and will be sending call on a value target.

[JonRowe]

Seems reasonable!

[pirj]

@JonRowe Appreciate if you take a look and give some major direction on how to improve this.

[pirj]

I'm still up to adjust this the way you see fit but need some guidance on specs.

[JonRowe]

Sorry this took so long to get back to you!

[JonRowe]

So this is because warnings are generated by the specs, it should be possible to rewrite the relevant specs warning free...

[pirj]

Previously, valid values were defined as e.g.:

  it_behaves_like "an RSpec matcher", :valid_value => lambda { k += 1 },

and that were supposed to check that

expect { k += 1 }.to change { k }

The matcher here is initialized by change { k }, and the target is a block.

So the whole point of those two expectations is to make sure that == on both the lambda and the matcher return false when compared with each other.

After this change the target block is defined using a method, and the reason for warnings to appear is that those output/throw_symbol/raise_error targets are actually get called when target is compared, since they are not a block.

Do you think that writing those examples as the following will achieve the same goal?

    expect(matcher).not_to eq(-> { valid_block })
    expect(-> { valid_block }).not_to eq(matcher)

[pirj]

@JonRowe Builds are green, all specs are in place.
It took a while to work around Ruby 1.8.7 block signature strictness, moved stuff around.
No rush, please take another look when you have a moment.

[pirj]

Squashed commits.

[pirj]

@JonRowe Do you think this needs some adjustment?

[JonRowe]

@JonRowe Do you think this needs some adjustment?

I promise this is high on my list for a re-review!

[JonRowe]

Hey @pirj great work, looks good. Do you want to merge and then write up a change log?

[pirj]

Thanks!

I'm holding off from merging this, found a problem with rspec-collection_matchers, still getting to the bottom of it.

     NoMethodError:
       undefined method `supports_value_expectations?' for #<#<Class:0x00007fb20c0a7128>:0x00007fb20c0a7088>
     # ./lib/rspec/collection_matchers/have.rb:71:in `determine_collection'
     # ./lib/rspec/collection_matchers/have.rb:41:in `matches?'
     # ./spec/rspec/collection_matchers/have_spec.rb:532:in `block (4 levels) in <top (required)>'

[pirj]

This change is due to have matcher from rspec-collection_matchers that is sensitive to methods called on it, i.e. if we call supports_value_expectations? on have(3).players, it starts to think that it should match supports_value_expectations? association, not players.

The problem is with supports_value_expectations?, changing supports_block_expectations? for consistency.

[mvz]

Just found this change and it also breaks the following code. Not sure if that was intended:

      call = lambda do
        Reek::CLI::Silencer.silently do
          described_class.new ['--foo']
        end
      end
      expect(call).to raise_error(SystemExit) do |error|
        expect(error.status).to eq Reek::CLI::Status::DEFAULT_ERROR_EXIT_CODE
      end

[mvz]

@pirj Yes, both are fine options, although I would prefer the second one rather than chaining multi-line blocks. I was just surprised my existing specs started failing.

I fixed my code by changing expect(call) to expect(&call).

[JonRowe]

This PR has been reverted for now.

[JonRowe]

I would like to include this again, but we need to address the following situations:

• Transitionally allow blocks to be in-explicitly passed in for future deprecation and warning. This is quite easy to achieve but has knock on effects for the next thing I'd like to address.
• Still allow passed in blocks to be introspected as values.

[pirj]

@JonRowe Sounds good. I can tackle this in the near future.
What I intend to do is to warn in enforce_value_expectation instead of failing.
Not sure I completely understand what you mean by block introspection, can you please elaborate a bit?

[JonRowe]

Things like expect(-> { }).to be_a(Proc)