Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RSpec/RepeatedExample false positive #1932

Open
francois opened this issue Jul 1, 2024 · 6 comments
Open

RSpec/RepeatedExample false positive #1932

francois opened this issue Jul 1, 2024 · 6 comments

Comments

@francois
Copy link

francois commented Jul 1, 2024

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]
@koic koic transferred this issue from rubocop/rubocop Jul 1, 2024
@francois
Copy link
Author

francois commented Jul 1, 2024

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" ?

@francois francois closed this as completed Jul 1, 2024
@pirj pirj reopened this Jul 1, 2024
@pirj
Copy link
Member

pirj commented Jul 1, 2024

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
Copy link
Member

pirj commented Jul 1, 2024

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
Copy link
Member

pirj commented Jul 1, 2024

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

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

@francois
Copy link
Author

francois commented Jul 2, 2024

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
Copy link
Member

pirj commented Jul 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants