[Close #856] implement repeated example group description/body cops

[lazycoder9]

There are 2 cops, that ensures that we don't have repeated specs.

RSpec/RepeatedExampleGroupDescription
Checks for repeated descriptions in example group
Example:

# bad
describe 'cool feature' do
  # example group
end

describe 'cool feature' do
  # example group
end

# good
describe 'cool feature' do
  # example group
end

describe 'another cool feature' do
  # example group
end

RSpec/RepeatedExampleGroupBody
Checks for repeated body or implementation for example group

# bad
context 'when case x' do
  it { cool_predicate }
end

context 'when case y' do
  it { cool_predicate }
end

# good
context 'when case x' do
  it { cool_predicate }
end

context 'when case y' do
  it { another_predicate }
end

Fixes #856

---

Before submitting the PR make sure the following are checked:

• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the changelog if the new code introduces user-observable changes.
• The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have created a new cop:

• Added the new cop to config/default.yml.
• The cop documents examples of good and bad code.
• The tests assert both that bad code is reported and that good code is not reported.

[lazycoder9]

Hey @pirj thanks for your comment on the issue, it allows me to make better merge and implement all that I wanted with 2 cops. Looking for your review

[pirj]

Glanced over. Looks good overall, a few nitpicks.

[pirj]

Most of the comments apply to both of the cops.

This is shaping up pretty quickly, good job!

[pirj]

Looks good. Just a couple of edge cases to fix.

[lazycoder9]

@pirj Hello, I've resolved all of your issues and made some enhances in node matchers. Hope it will be your last review 😄

[pirj]

Looks really good. 👍
Some minor comments here and there, nothing that really needs addressing, it's good as is.

I've tested on a large codebase and found several copy-pasted example groups and descriptions, and also where both description and block were identical, yay! 👏 Immediate value.

1. (optional) However, there are some false positives, e.g.:

    context 'rejected' do
      skip
    end

    context 'processed' do
      skip
    end

I think skip and pending could be skipped, but it's ok to leave this for later.

2. Interpolated docstrings are considered equal:

context "when model is in status #{Statuses::PENDING} some postfix" do
...
context "when model is in status #{Statuses::APPROVED} some OTHER postfix" do

You can look for dstr for an example of how to handle it.

3. All classes are considered equal:

context A::B do
...
context C::D do

4. (completely optional) with two above cases, it might make sense to check if a variable or a method call is used as an argument for docstring:

def description
  "does that"
end
context(description) do
...
context(description) do

I think 2&3 are major.

[lazycoder9]

@pirj You have left some comments, but anyway approved PR, why?) And also, I've fixed your last 4 tensions and added test cases for them. Looking for your review

[lazycoder9]

@pirj sorry, I've requested re-review out of habit 😄

[pirj]

All good from my point of view, no false positives detected. 👍
Really good job, and impressive node pattern skills.

I didn't tell you in advance, but @bquorning and @Darhazer might chime in to add their notes.
As from my side, I'm happy with the current state of those cops.

[pirj]

Flawless from my point of view. Good job!

[pirj]

@bquorning @Darhazer do you mind to take a look?

[Darhazer]

Tests-wise, can we check that repeated examples are flagged if there is something else between them?

describe 'repeated' do
  it { is_expected.to be_truthy }
end

before { do_something }

describe 'this is repeated' do
  it { is_expected.to be_truthy }
end

Can we test that context are not flagged for having the same example on different data?

context 'when admin' do
  let(:user) { admin }
  it { is_expected.to be_truthy }
end

context 'when regular user' do
  let(:user) { staff }
  it { is_expected.to be_truthy }
end

[lazycoder9]

@Darhazer Done

[Darhazer]

Thank you.
I've tested on a real codebase, and it has a false positive when the example refers to a let variable, and the let value is different in the different contexts.

This might be hard to catch using static analysis, and I see value in the RepeatedExampleGroupBody cop. Perhaps we can just mark it as Unsafe cop. Other options include to skip example groups with different variables set; or actually check if those variables are used within the repeated example

[Darhazer]

It also fails on examples/group without descriptions, that are actually different

context do
  let(:preferences) { default_preferences }

  it { is_expected.to eq false }
end

context do
  let(:preferences) { %w[a] }

  it { is_expected.to eq true }
end

[pirj]

false positive when the example refers to a let variable and the let value is different in the different contexts.

@Darhazer But how is this possible for sibling contexts? We account for metadata differences already.

[lazycoder9]

It also fails on examples/group without descriptions, that are actually different

context do
  let(:preferences) { default_preferences }

  it { is_expected.to eq false }
end

context do
  let(:preferences) { %w[a] }

  it { is_expected.to eq true }
end

@Darhazer how we should handle such cases? Just ignore them and another cop (maybe new) should handle it?

[Darhazer]

@pirj it's not metadata, it's a let node in the example group, but the cop selects the example nodes only, e.g. ignores anything else inside the example group

@lazycoder9 there's nothing to handle there - it's a false positive - the examples are different and should not be flagged as repeated.

[lazycoder9]

@Darhazer Fixed false positive with example groups without descriptions. I'm going to think about false positive case with let variables

[pirj]

@Darhazer I couldn't come up with a better example:

  it 'registers an offense for repeated context body' do
    expect_offense(<<-RUBY)
      context 'when awesome case' do
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Repeated context block body on line(s) [5]
        let(:a) { 1 }
        it { cool_predicate_method }
      end

      context 'when another awesome case' do
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Repeated context block body on line(s) [1]
        let(:a) { 2 }
        it { cool_predicate_method }
      end
    RUBY
  end

And it fails since cop considers those two examples different. Can you please provide a minimal reproduction case?

[Darhazer]

cool_predicate_method(a) would be different in the two examples, although their AST would remain the same

[lazycoder9]

@Darhazer I've tried your example with cool_predicate_method(a) as

  it 'does not register offense' do
    expect_no_offenses(<<-RUBY)
      context 'when awesome case' do
        let(:a) { 1 }
        it { cool_predicate_method(a) }
      end

      context 'when another awesome case' do
        let(:a) { 2 }
        it { cool_predicate_method(a) }
      end
    RUBY
  end

And it works. Maybe another cop was triggered in this code? I think about RepeatedExample or I'm missing something. @pirj thoughts?

[pirj]

Tried on two more pretty large codebases, no signs of false positives. Only traces of copy-pasta, inspiring false confidence in code impeccancy.
@Darhazer your only option is to reveal that example.

[Darhazer]

@pirj perhaps it was connected with the previous false positives - with the latest code, I don't get any.

I'll make a final review early next week.

[Darhazer]

Fantastic work!
Please squash the commits.

I left a few questions, but nothing that would really block a merge

[Darhazer]

Should it? if there is a variable and the variable value changes between the two context calls?
Although this is such an edge case that I could live with the false positives. Actually it would be very bad code to start with

[pirj]

I hope there's no one in the world writing describe "when time is #{Time.now.to_f}" do and repeat that exact description :D It's better to flag this even though technically those two will be two different descriptions.

[lazycoder9]

@pirj @Darhazer Hey! Tried to fix all of your latest questions\comments. Squashed commits. Thanks a lot guys, it was a really good experience for me

[Darhazer]

🚀

[pirj]

Thanks for those amazing new cops!

[pirj]

And you are always welcome to contribute more cops and come up with ideas for cops/improvements.

[marcandre]

Great addition 🎉 ! There was a case in rubocop-ast 😄 too :-)