Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

@pirj
Copy link
Contributor

commented Aug 5, 2019

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

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-hq/rspec-style-guide#76

def supports_block_expectations?
true
end

def supports_value_expectations?

This comment has been minimized.

Copy link
@pirj

pirj Aug 5, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Aug 8, 2019

Member

Seems reasonable!

pirj added some commits Aug 8, 2019

Fix the remaining change matcher spec failure
    matcher.matches?(invalid_value)

doesn't work with block-only matchers, as no block is passed in, and it
fails with a message:

    1) RSpec::Matchers::BuiltIn::Change behaves like an RSpec block-only matcher uses the `ObjectFormatter` for `failure_message`
       Failure/Error: expect(message).to include("detailed inspect")
         expected "expected `@k` to have changed, but was not given a block" to include "detailed inspect"

The redundant (due to existing check in ExpectationTarget) `Proc ===
@event_proc` checks could not be removed safely as well, since
@actual_after is not initialized yet when we haven't executed the block:

     RuntimeError:
       Warnings were generated: rspec-dev/repos/rspec-expectations/lib/rspec/matchers/built_in/change.rb:407: warning: instance variable @actual_after not initialized
Remove redundant proc checks in matchers
This is already checked in ExpectationTarget
@pirj
Copy link
Contributor Author

left a comment

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

@@ -262,13 +262,19 @@ def expect_block

describe "expect(...).to matcher.and(other_matcher)" do

it_behaves_like "an RSpec matcher", :valid_value => 3, :invalid_value => 4, :disallows_negation => true do
it_behaves_like "an RSpec value matcher" do

This comment has been minimized.

Copy link
@pirj

pirj Aug 9, 2019

Author Contributor

Now that value and blocks specs are separate I think that for "an RSpec value matcher" it's possible to keep passing the arguments via shared example, not using let's.
It will also reduce the size of this pull request.


it 'preserves the symmetric property of `==`' do
expect(matcher).to eq(matcher)
# TODO: the following expectations fail due to

This comment has been minimized.

Copy link
@pirj

pirj Aug 9, 2019

Author Contributor

@JonRowe I have two open questions on how to deal with the following examples.

Apart from this one and the pair below everything is green.

# Does it make sense for block matchers?
end

# TODO: Two options here.

This comment has been minimized.

Copy link
@pirj

pirj Aug 9, 2019

Author Contributor

The second set of examples I some have doubts.

@pirj

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

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

@@ -5,8 +5,10 @@
minimum_coverage 97
end

Dir['./spec/support/**/*'].each do |f|
require f.sub(%r|\./spec/|, '')
Dir.chdir('spec') do

This comment has been minimized.

Copy link
@JonRowe
@JonRowe
Copy link
Member

left a comment

Sorry this took so long to get back to you!

# 1) output.to_stderr matcher behaves like an RSpec block-only matcher preserves the symmetric property of `==`
# Failure/Error: raise "Warnings were generated: #{output}" if has_output?
# RuntimeError:
# Warnings were generated: foo

This comment has been minimized.

Copy link
@JonRowe

JonRowe Aug 23, 2019

Member

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

# yield_spec.rb itself.
# 2. Remove this. I'm not too confident with the code base to tell if it's
# absolutely necessary to check this, and why `===` is vital for
# composing matchers, and if it's applicable to block matchers.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Aug 23, 2019

Member

Its applicable to all matchers, its the protocol for them.

# I see two options:
# 1. Introduce another conditional flag, and run the former two examples
# for all specs except yield_spec.rb. Also pull those two latter to
# yield_spec.rb itself.

This comment has been minimized.

Copy link
@JonRowe

JonRowe Aug 23, 2019

Member

How about ->(*args) { args.length == 1 && args[0] === Proc ? invalid_block(&args[0]) : invalid_block } with a note explaining why? How did these pass before? Different inputs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.