Discussion: RSpec/ExpectChange

[foobear]

The ExpectChange cop forces expect { ... }.to change to use ellipsis instead of a block when observing the result of a single method call on a single object, e.g.

expect { run }.to change { foo.baz } # bad
expect { run }.to change(foo, :baz) # good

The supposed benefit is "consistent style" (according to the docs), but when observing anything else, like foo.bar.baz, ellipsis won't work and the cop happily accepts a block.

If anything, we should enforce the block style, especially for the sake of consistency. Or, we just disable the cop.

:+1: change to block :-1: keep method_call :rocket: disable cop

[codener]

I have ever only needed change with a target and a method to call. I prefer the method call syntax over chaining to inline-blocks, so I'm voting for "keep".

expect { run }.to change(foo, :baz).by(1)
expect { run }.to change { foo.baz }.by(1)

Wouldn't foo.bar.baz easily be expressed by change(foo.bar, :baz)? Or are there more complex scenarios I don't have in mind?

[foobear]

Not if foo.bar is a meaningful action, like user.reload or ActionMailer::Base.deliveries. At least for me that is the most common case for such chained statements.

expect { Renamer.run }.to change { user.reload.name }.from('Batman').to('Bruce Wayne')

[FLeinzi]

Not if foo.bar is a meaningful action, like user.reload or ActionMailer::Base.deliveries. At least for me that is the most common case for such chained statements.

expect { Renamer.run }.to change { user.reload.name }.from('Batman').to('Bruce Wayne')

But this also works with:

expect { Renamer.run }.to change(user, :name).from('Batman').to('Bruce Wayne')

Edit This also works:

expect { subject.run }.to change(ApplicationMailer.deliveries, :size).by(1)

[FLeinzi]

TIL: You can rewrite the awkward

expect { renamer.run }.to change { user.reload; [user.username, user.email] }
  .from(['batman', 'b@man.dc']).to(['bruce wayne', 'bruce@wayne.de'])

to the also awkward

expect { renamer.run }.to change(user, :username).from('batman').to('bruce wayne')
  .and(change(user, :email).from('b@man.dc').to('bruce@wayne.de'))

[foobear]

also works

Not necessarily. user may not be the same instance that Renamer uses -- which is why such a test would have to read user.reload.name, not user.name.

ActionMailer::Base.deliveries was a bad example, since deliveries does not cache its size, but there are many other cases where an expression needs to be evaluated twice.

My main issue with the cop is that I find it terribly annoying that I have to write the same "thing" in two different ways depending on whether I needs to re-evaluate or not. I don't want anyone to care about that when writing a test -- which is why I very much prefer the lambda style, because that expression is always re-evaluated in full.

[denzelem]

With the votes from the issues we are going to disable the cop in the default config.

[brunosedler]

Disabled cop in https://github.com/makandra/makandra-rubocop/pull/31 and released in version 7.1.0.