search

rubocop / rubocop-rspec

Code style checking for RSpec files.
https://docs.rubocop.org/rubocop-rspec
MIT License
792 stars 272 forks source link

RSpec/RepeatedExample false positive #1932

Open francois opened 2 days ago

francois commented 2 days ago

We had a spec file where we were asserting against an oracle. Then, we discovered issues with the values themselves, so we switched to string interpolation. Unfortunately, with the string interpolation, RSpec/RepeatedExample says there is an issue, when the values are actually different.

Expected behavior

Given the following code:

RSpec.describe OrderTagsService, :order_tags do
  let(:contingency_date) { ... }
  let(:order) { ... }

  context 'given an appointment-based order with one requested inspection' do
    let!(:inspection) { create(:inspection, :requested, order:) }

    context 'with no availabilities at all' do
      describe 'the Ruby implementation' do
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T12:00-0700")) }
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T16:00-0700")) }
      end
    end
  end
end

No Rubocop failures were expected. Note the string interpolation is different between the two lines.

In the trace below, these are lines 287 and 288.

Actual behavior

For /Users/francois/Projects/marketplace: configuration from /Users/francois/Projects/marketplace/.rubocop.yml
configuration from /Users/francois/.rvm/gems/ruby-3.2.4/gems/rubocop-rails-2.17.2/config/default.yml
configuration from /Users/francois/.rvm/gems/ruby-3.2.4/gems/rubocop-rails-2.17.2/config/default.yml
Default configuration from /Users/francois/.rvm/gems/ruby-3.2.4/gems/rubocop-1.43.0/config/default.yml
configuration from /Users/francois/.rvm/gems/ruby-3.2.4/gems/rubocop-rspec-2.15.0/config/default.yml
configuration from /Users/francois/.rvm/gems/ruby-3.2.4/gems/rubocop-rspec-2.15.0/config/default.yml
Inheriting configuration from /Users/francois/Projects/marketplace/.rubocop_todo.yml
Use parallel by default.
Skipping parallel inspection: only a single file needs inspection
Inspecting 1 file
Scanning /Users/francois/Projects/marketplace/spec/services/order_tags_service_spec.rb
Loading cache from /Users/francois/.cache/rubocop_cache/f851b9b9ba5b973bce8ba476853645abc5b75ef2/6d7a3b621ca1730e04accd938619e4bdab66cfb1/e3a436b429fad69dc8aa1d9be4ea25bdea422812
C

Offenses:

spec/services/order_tags_service_spec.rb:287:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T12:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:288:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T16:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:290:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T12:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:291:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T16:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:292:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).to be_urgent(order, now: T("#{contingency_date}T17:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:293:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).to be_urgent(order, now: T("#{contingency_date}T12:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:295:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).to be_urgent(order, now: T("#{contingency_date}T17:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:296:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).to be_urgent(order, now: T("#{contingency_date}T12:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 8 offenses detected
Finished in 0.27164399999992384 seconds

Steps to reproduce the problem

Of course, I can't reproduce this issue on a fresh file. In the original

RuboCop version

$ rubocop -V
1.43.0 (using Parser 3.3.1.0, rubocop-ast 1.24.1, running on ruby 3.2.4) [arm64-darwin23]
  - rubocop-rails 2.17.2
  - rubocop-rspec 2.15.0
$ ruby --version
ruby 3.2.4 (2024-04-23 revision af471c0e01) [arm64-darwin23]
francois commented 2 days ago

After more investigation, I'm sorry but I do in fact have repeats. The failures highlight this:

spec/services/order_tags_service_spec.rb:287:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T12:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/services/order_tags_service_spec.rb:290:9: C: RSpec/RepeatedExample: Don't repeat examples within an example group.
        it { expect(subject).not_to be_urgent(order, now: T("#{contingency_date}T12:00-0700")) }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

287 and 290 are duplicates of each other, but the message didn't highlight this. Maybe a line number to say "duplicates this other line" ?

pirj commented 2 days ago

Thanks for reporting!

The key parts here are: example.implementation call, and how we get this implementation. The latter is 8 years old, older than the cop, and probably insufficient to confidently tell if examples are identical or not.

Would you like to take a stab at this? I think that instead of example.implementation we should use example.ast, but careful - we don’t have sufficient coverage in specs with just one example that would cover different syntaxes.

pirj commented 2 days ago

So do you say that after fixing the duplicate all other offences are gone? That is weird, as eg there was an offence for line 296.

I’d keep this open anyway yo at least add one multi-statement example, and an example where the same code is formatted differently (one-line is_expected.to vs same with newline).

pirj commented 2 days ago

Maybe a line number to say "duplicates this other line" ?

We have cops like this, and this will make a useful improvement.

francois commented 1 day ago

So do you say that after fixing the duplicate all other offences are gone? That is weird, as eg there was an offence for line 296.

You cannot see the full describe block, but there truly were multiple instances, 8 to be exact:

It { expect(subject).to be_urgent(T(“#{contingency_date}T12:00”)) }
It { expect(subject).to be_urgent(T(“#{contingency_date}T17:00”)) }
It { expect(subject).to be_urgent(T(“#{contingency_date}T12:00”)) }
It { expect(subject).to be_urgent(T(“#{contingency_date}T17:00”)) }

In the example above, I have 2 @ 12, and 2 @ 17. The original code had a real oracle: 2024-06-25T12:00, and 2024-06-25T17:00, then 2024-06-26T12:00, and 2024-06-26T17:00. Notice that we had 2 dates: Jun 25 and Jun 26. When I switched over to string interpolation, I mistakenly used the same date & time for everything, and Rubocop CORRECTLY flagged the duplication.

What I mistakenly understood was that the 1st 12:00 line was a duplicate of the 1st 17:00 line, but I forgot / misread the code and didn’t see the 2nd instance of 12:00.

I would be happy to add the necessary code to report the “other” offending line. What if there are multiple instances? Should every line be reported, or only the 1st one?

In our original example, we had 3 pairs of dates, at 12:00 and at 17:00. Should the 1st instance report that the 2nd one is a duplicate, or should all duplicates be found and reported as a block?

  1. RSpec/RepeatedExample: This example is repeated on line XXX
  2. RSpec/RepeatedExample: This example is repeated N times on lines XXX, YYY and ZZZ
pirj commented 1 day ago

Understood, thank you!

What if there are multiple instances? Should every line be reported, or only the 1st one?

I think we accumulate the other lines.

should all duplicates be found and reported as a block?

Yes.