Discourage reusing matchers in the same example

[pirj]

Reusage may have undesired side effects, e.g. (see rspec/rspec-expectations#1287):

RSpec.describe 'A', :aggregate_failures do
  specify do
    matcher = contain_exactly(eq(1))

    expect([2]).to matcher
    expect([3]).to matcher
  end
end

will output for the second failure the same extra as for the first one:

the extra elements were:        [2]

instead of

the extra elements were:        [3]

https://github.com/rspec/rspec-expectations/blob/bd10f0cf3970932781efcd09b5e427877d16a6c2/lib/rspec/matchers/composable.rb#L113

# Historically, a single matcher instance was only checked
# against a single value. Given that the matcher was only
# used once, it's been common to memoize some intermediate
# calculation that is derived from the `actual` value in
# order to reuse that intermediate result in the failure
# message.
#
# This can cause a problem when using such a matcher as an
# argument to another matcher in a composed matcher expression,
# since the matcher instance may be checked against multiple
# values and produce invalid results due to the memoization.
#
# To deal with this, we clone any matchers in `expected` via
# this method when using `values_match?`, so that any memoization
# does not "leak" between checks.

rspec/rspec-expectations#518

The change is best viewed as https://github.com/rubocop-hq/rspec-style-guide/blob/discourage-using-memoized-matchers/README.adoc#matcher-reuse

[andyw8]

Let's wait for a response on rspec/rspec-expectations#1287 ?

[pirj]

rspec-expectations bug has been confirmed, and it's most probably an easy fix.

I think I have to come up with a different argumentation, but memoized helpers are rspec-core's concept, while it is arguably misused in the matcher part of the expectation which is rspec-expectations's concept. They don't overlap much and don't use each other's syntax in their examples.

The code that initially triggered me was:

let(:expected_value) { {a: 1, b: 2, c: 3} }

it 'returns a proper hash' do
  expect(parser.parse).to eq expected_value
end

It felt extremely wrong to use memoized helpers in the "right-hand" side of the expectation. Yes, it's possible, and you won't shoot yourself in the leg, and even if you do, it's not critical, just the failure message is off.

With so many options, around, the use of memorized helpers has no reasonable justification in my book.

Namely,

1. Variable

expected_value = {a: 1, b: 2, c: 3}

it 'returns a proper hash' do
  expect(parser.parse).to eq expected_value
end

Beware, the lifecycle of such variables is completely different from let, see https://github.com/rubocop-hq/rubocop-rspec/issues/991

2. Method

def expected_value
  {a: 1, b: 2, c: 3}
end

it 'returns a proper hash' do
  expect(parser.parse).to eq expected_value
end

An additional bonus: methods are flexible, i.e. you can parameterize the expected_value by passing something to it. This is achieved with several lets, but is subjectively harder:

def expected_value(last: 3)
  {a: 1, b: 2, c: last}
end

it 'returns a proper hash' do
  expect(parser.parse("a1b2c3")).to eq expected_value
  expect(parser.parse("a1b2c4")).to eq expected_value(last: 4)
end

I find it hard to achieve the same with lets without using several context example groups.

Additionally, helper methods do have access to memoized helpers.

def go_well
  eq(easy)
end

let(:easy) { 'ha!' }

it { is_expected.to go_well }

3. Constant
   It's a piece of bad advice due to https://rspec.rubystyle.guide/#declare-constants

So, unfortunately, except for the initial argument that there's a slight bug in rspec-expectation, I can only provide to use arguably more efficient testing styles. No real argumentation of why it is inherently bad and should be avoided at all costs.

[pirj]

I've prototyped a cop to enforce this, but it turned out to have a lot of false positives in real-world-rspec that I didn't foresee. rubocop/rubocop-rspec@b7ebdb9

Closing for now, because:

• "matcher reuse" is clearly fixable in RSpec
• let(:expected_value) { {a: 1, b: 2, c: 3} } seems to be my personal view on clean specs
• writing a cop to enforce this is troublesome