rspec-expectations: Block matchers not detecting passed in block arguments

Why introducing a breaking change in the latest patch?

Upgrading expectations from 3.8.4 to 3.8.5 breaks specs with: You must pass a block rather than an argument to expectto use the provided block expectation matcher (changePost.count by 1).

Your environment

Ruby version: 2.6.3
rspec-expectations version: 3.8.5

Steps to reproduce

  describe 'POST #create' do
    subject do
      -> { post :create, params: params }
    end

    let(:params) {}

    it 'creates a new post' do
      is_expected.to change { Post.count }.by(1)
    end
  end

Expected behavior

is_expected.to change { Post.count }.by(1) should not raise an error.

Breaking changes should be introduced in major updates like 4.0. Is there any approach to keep the compatibility?

Actual behavior

is_expected.to change { Post.count }.by(1) raises You must pass a block rather than an argument to expectto use the provided block expectation matcher (changePost.count by 1)

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 10
Comments: 22 (11 by maintainers)

Commits related to this issue

lock rspec-expectations this applies until a solution to rspec/rspec-expectations#1134 is created — committed to jmartin-tech/metasploit-framework by jmartin-tech 5 years ago
(maint) fix failing tests due to rspec changes This test was failing due to some changes on rspec-expectactions gems. More info here: https://github.com/rspec/rspec-expectations/issues/1134 — committed to gimmyxd/puppetlabs-puppet_agent by gimmyxd 5 years ago

Most upvoted comments

👋 Hi, sorry about this, this is not a breaking change though, it’s a bug. The change was intended as an improvement to the warning which happens when people incorrectly pass in values to block matchers. This is an edge case that was overlooked and we’ll investigate a fix.

JonRowe on Oct 3, 2019

Fixing is_expected.to raise_error will require to basically revert #1125, which is a fix for the syntax that was never supposed to exist.

@JonRowe WDYT if we revert this change in 3.8.6 and re-introduce in 4.0?

@cyberious This means that is_expected.to raise_error will stop working with a major RSpec version update, and the changes to expect { catalogue }.to raise_error will still be required, but could be postponed. Will that work for you?

pirj on Oct 4, 2019

#1125 Has been reverted for the time being, so this should no longer be an issue. Released as 3.8.6.

JonRowe on Oct 7, 2019

Ended up introducing raise_error to rspec-puppet that supports lambda value targets. @alexjfisher @cyberious appreciate your feedback.

pirj on Oct 6, 2019

That would give us time to make changes. Maybe even a rubocop-rspec enhancement could help, or perhaps someone could even figure out a rubocop-rspec-puppet with an autofix for the most common issues?

alexjfisher on Oct 4, 2019

@alexjfisher There’s RSpec/ImplicitBlockExpectation released in rubocop-rspec 1.35.0, however it doesn’t come with autocorrection. The way the cop does the detection won’t help with rspec-puppet unfortunately, since the cop expects the subject to be defined in the same file, and does not rely on the matcher being used. rspec-puppet defines subject underneath, outside of the file.

Doing otherwise that would only detect built-in RSpec block matchers, and would miss block matchers wrapped in methods, e.g.:

def create_user
  change { User.count }.by(1)
end

it { is_expected.to create_user }

pirj on Oct 4, 2019

While yes the work arounds are good, this is a breaking change, and there are a ton of puppet modules and work arounds, we have is_expected.to raise_error all over the place. When can we expect a fix instead of having to modify tons of modules and tests. Not all of them are public so this might be breaking a bunch of modules in some fortune 100 companies.

cyberious on Oct 4, 2019

So, bundler prefers rspec-expectations 3.8.5 and less recent rspec-puppet

It’s seems non deterministic. In local testing I’ve seen bundle update flip between which gem it prefers… https://gist.github.com/alexjfisher/97dc9618de3167c29ccb13f819410008

alexjfisher on Oct 4, 2019

@msxavi Agree. How do you feel about:

  describe 'POST #create' do
    def send_request
      post :create, params: params
    end

    let(:params) {}

    it 'creates a new post' do
      expect { send_request }.to change { Post.count }.by(1)
    end
  end

Let’s keep the issue open to track Vox Pupuli’s issues with rspec-puppet, and it’s a bit more complicated there since the matchers both accept block and value targets.

pirj on Oct 4, 2019