Block matchers not detecting passed in block arguments

[msxavi]

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)

[highb]

I agree. This sort of major breaking change does not belong in a Z release.

[JonRowe]

👋 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.

[pirj]

@msxavi Can you please provide more details on how the subject is defined?
Related:
https://blog.rubystyle.guide/rspec/2019/07/17/rspec-implicit-block-syntax.html
rubocop/rspec-style-guide#76

[alexjfisher]

@pirj Hi! Voxpupuli has noticed build failures in several of our puppet modules this morning.
eg https://github.com/voxpupuli/puppet-cassandra/blob/9954256a6767d51c9dec30b7e7f755bb9734c8d8/spec/defines/schema/permission_spec.rb#L19
https://travis-ci.org/voxpupuli/puppet-cassandra/jobs/593000621#L594

Puppet module testing is based on rspec-puppet

If it helps, I can provide some other examples, or drop by #voxpupuli on Freenode IRC.
Thanks

[pirj]

@alexjfisher This and how block matchers are implemented is causing problems.

Would you agree instead of using is_expected to use:

expect { catalogue }.to raise_error(Puppet::Error)
# ...
expect { catalogue }.to have_resource_count(9)

I can send pull requests to rspec-puppet to:

• make sure its current stable version depends on < RSpec 3.8.5
• change matchers and subject definitions, and update the dependency to >= RSpec 3.8.5 in master

[alexjfisher]

@pirj Hi! Thanks for getting back to me. Is the new syntax just until rspec-puppet can be fixed?

[alexjfisher]

and thank you for your offer of PRs to rspec-puppet. I'm sure they would be very much appreciated.

[pirj]

@alexjfisher I suggest you to explicitly set rspec-expectations in puppet-cassandra to 3.8.4:

# Gemfile

group :test do
  gem 'rspec-expectations', '< 3.8.5'

and update rspec-expectations together with rspec-puppet when they accept the fix.
Sorry for the inconvenience.

[alexjfisher]

Yes, we can certainly do that. We manage a lot of puppet modules though and they almost all use rspec-puppet. I'll see if I can get the dependency updated there (until it's fixed).

[msxavi]

@pirj here it is. It's a regular post request defined as a lambda. TBH that looks a little bit wicked.

  subject do
    -> { post :create, params: params }
  end

[pirj]

@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.

[alexjfisher]

@pirj Hi! rspec-puppet has had a new release, but bundler doesn't prefer the latest version of rspec-puppet over the latest version of rspec-expectations. (Not without us updating every Gemfile, which we're hoping to avoid).
At Vox, we maintain lots of modules, but many other module developers have also been affected. We all use rspec-puppet. If there's anything you can do to help @rodjek implement a proper fix, that'd be awesome!

[pirj]

@alexjfisher So, bundler prefers rspec-expectations 3.8.5 and less recent rspec-puppet, understood. No worries, I'll work on a minimal rspec-puppet patch to add support to their matchers that would accept both values (catalogue) and blocks (-> { catalogue }).
However, I foresee that the following constructs:

let(:catalogue) { -> { load_catalogue } }
subject { catalogue }
...
is_expected.to raise_error ...
expect(subject).to raise_error ...
expect(catalogue).to raise_error ...

won't work, since it's a built-in rspec-expectations matcher, and it only accepts blocks, e.g.:

subject { catalogue }
let(:catalogue) { load_catalogue }
...
expect { subject }.to raise_error ...
expect { catalogue }.to raise_error ...

[alexjfisher]

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

[cyberious]

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.

[JonRowe]

Hi @cyberious as I've mentioned we are looking into it. However please remember this is an open source project staffed by volunteers, so I'm afraid "when" is "when we have time".

[pirj]

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?

[alexjfisher]

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?

[pirj]

@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]

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

[msxavi]

Thanks, @pirj but I agree with @cyberious this is a breaking change regardless and I've got many other specs in the same format. Where are we about reverting #1125? Is that an option?

[JonRowe]

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