Detect repeated descriptions in example groups #259

backus · 2016-12-26T19:02:59Z

No description provided.

backus · 2016-12-26T19:52:00Z

@bquorning @andyw8 this is reviewable

bquorning

I tried this on my project, and…

1199 files inspected, 56 offenses detected

So far, I haven’t found any false positives.

bquorning · 2016-12-26T20:43:27Z

spec/rubocop/cop/rspec/repeated_description_spec.rb

+describe RuboCop::Cop::RSpec::RepeatedDescription do
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense for repeated descriptions' do


Perhaps also add the scenario where the same description is used in different context blocks.

Please also test a scenario where two examples with the same description are separated by a scope change. I’m thinking e.g.

it "does x" do end context "something else" do it "works fine" do end end it "does x" do end

Yeah good catch. I actually thought I had more than one test here but I guess not. I'll add a few more

@bquorning the tests should be sufficient now

bquorning · 2016-12-26T20:53:25Z

lib/rubocop/cop/rspec/repeated_description.rb

+        def find_hooks(node, &block)
+          return if scope_change?(node)
+
+          example(node, &block)


I’m not sure what this method call does. It looks like it matches the node and block against a pattern, but the return value is neither stored nor returned.

Sorry about that. I added some doc comments and I also renamed the misleading name... It was a holdover from code I borrowed from #254. Didn't realize I forgot to change the name sorry about that

This method does a recursive search until it hits a scope changing node. RuboCop's node pattern methods will yield (unless you defined a predicate method ending in ? I believe) if you give it a block and the provided node matches. By writing

def_node_pattern :desired_node, '...' def_node_pattern :node_should_stop_recursion?, '...' def desired_nodes(node, &block) node.each_child_node { |child| desired_node_search(node, &block) } end def desired_node_search(node, &block) return unless node_should_stop_recursion?(node) desired_node(node) desired_nodes(node) end

you get a recursive node search that stops when it hits a match for node_should_stop_recursion?. The end result is that I can write

desired_nodes(node) do |match| add_offense(match, ...) end

which I like. I follow this pattern in a handful of cops

bquorning · 2016-12-26T21:00:19Z

lib/rubocop/cop/rspec/repeated_description.rb

+            .reject { |args, _| args.empty? }
+            .values
+            .reject(&:one?)
+            .flatten


Would this be more readable using local variables?

hooks_with_arguments = hooks_in_group(node).group_by(&:method_args).reject { |args, _| args.empty? } hooks_with_identical_arguments = hooks_with_arguments.values.reject(&:one?) hooks_with_identical_arguments.flatten

If I got the names wrong, it’s because I didn’t understand the code.

I tried to clarify with inline comments. I prefer a functional style like this when possible but only if the code is still clear. Is the updated code clear?

Found these offending spec descriptions while testing out rubocop/rubocop-rspec#259. Searching for duplicate spec descriptions revealed a few categories of issues: 1. Examples with repeated descriptions that tested different cases but didn't describe the specific difference. For example: ```ruby it 'autocorrects' do new_source = autocorrect_source(cop, '"this is the #{}"') expect(new_source).to eq('"this is the "') end it 'autocorrects' do new_source = autocorrect_source(cop, '"this is the #{ }"') expect(new_source).to eq('"this is the "') end ``` 2. Examples with repeated descriptions where one or more descriptions were wrong. For example: ```ruby it 'does not register offense if range startpoint is not constant' do inspect_source(cop, '(a..10).each {}') expect(cop.offenses).to be_empty end it 'does not register offense if range startpoint is not constant' do inspect_source(cop, '(0..b).each {}') expect(cop.offenses).to be_empty end ``` 3. Redundant examples where the spec and its corresponding description showed up twice in the same context. For example: ```ruby it 'picks ruby executable files with no extension' do expect(found_basenames).to include('executable') end # ... it 'picks ruby executable files with no extension' do expect(found_basenames).to include('executable') end ```

backus · 2016-12-26T22:22:06Z

@bquorning ready for another pass.

andyw8 · 2016-12-27T01:16:22Z

lib/rubocop/cop/rspec/repeated_description.rb

+        def_node_matcher :scope_change?,
+                         (ExampleGroups::ALL + SharedGroups::ALL).block_pattern
+
+        MSG = "Don't repeat descriptions withhin an example group.".freeze


Typo in "withhin".

Good catch thanks. Fixed now

andyw8 · 2016-12-27T01:22:30Z

Ran it over a large codebase and it caught 74 issues, all appear to be genuine. Nice work on this!

Found these offending spec descriptions while testing out rubocop/rubocop-rspec#259. Searching for duplicate spec descriptions revealed a few categories of issues: 1. Examples with repeated descriptions that tested different cases but didn't describe the specific difference. For example: ```ruby it 'autocorrects' do new_source = autocorrect_source(cop, '"this is the #{}"') expect(new_source).to eq('"this is the "') end it 'autocorrects' do new_source = autocorrect_source(cop, '"this is the #{ }"') expect(new_source).to eq('"this is the "') end ``` 2. Examples with repeated descriptions where one or more descriptions were wrong. For example: ```ruby it 'does not register offense if range startpoint is not constant' do inspect_source(cop, '(a..10).each {}') expect(cop.offenses).to be_empty end it 'does not register offense if range startpoint is not constant' do inspect_source(cop, '(0..b).each {}') expect(cop.offenses).to be_empty end ``` 3. Redundant examples where the spec and its corresponding description showed up twice in the same context. For example: ```ruby it 'picks ruby executable files with no extension' do expect(found_basenames).to include('executable') end # ... it 'picks ruby executable files with no extension' do expect(found_basenames).to include('executable') end ```

bquorning · 2016-12-27T22:19:54Z

This looks good. Add a CHANGELOG entry (and perhaps copy it to the PR description for future reference), and it’s a 👍 from me.

backus · 2016-12-28T02:15:33Z

@bquorning whoops good catch on the changelog. Added. Will merge when build is green 😄

Detect repeated descriptions in example groups

Found these offending spec descriptions while testing out rubocop/rubocop-rspec#259. Searching for duplicate spec descriptions revealed a few categories of issues: 1. Examples with repeated descriptions that tested different cases but didn't describe the specific difference. For example: ```ruby it 'autocorrects' do new_source = autocorrect_source(cop, '"this is the #{}"') expect(new_source).to eq('"this is the "') end it 'autocorrects' do new_source = autocorrect_source(cop, '"this is the #{ }"') expect(new_source).to eq('"this is the "') end ``` 2. Examples with repeated descriptions where one or more descriptions were wrong. For example: ```ruby it 'does not register offense if range startpoint is not constant' do inspect_source(cop, '(a..10).each {}') expect(cop.offenses).to be_empty end it 'does not register offense if range startpoint is not constant' do inspect_source(cop, '(0..b).each {}') expect(cop.offenses).to be_empty end ``` 3. Redundant examples where the spec and its corresponding description showed up twice in the same context. For example: ```ruby it 'picks ruby executable files with no extension' do expect(found_basenames).to include('executable') end # ... it 'picks ruby executable files with no extension' do expect(found_basenames).to include('executable') end ```

backus mentioned this pull request Dec 26, 2016

Cop Idea: Repeated Description in Same Example Group #258

Closed

backus force-pushed the feature/RepeatedDescription branch from 099f523 to ccd9402 Compare December 26, 2016 19:07

Fix repeated descriptions

3f41d9c

backus force-pushed the feature/RepeatedDescription branch 3 times, most recently from 86daf54 to 3f41d9c Compare December 26, 2016 19:46

backus mentioned this pull request Dec 26, 2016

Rewrite repeated, wrong, and redundant spec descriptions rubocop/rubocop#3828

Merged

11 tasks

bquorning reviewed Dec 26, 2016

View reviewed changes

backus force-pushed the feature/RepeatedDescription branch 5 times, most recently from 6b75167 to b86bd70 Compare December 26, 2016 22:14

andyw8 reviewed Dec 27, 2016

View reviewed changes

backus force-pushed the feature/RepeatedDescription branch from b86bd70 to 5567b75 Compare December 27, 2016 08:15

backus force-pushed the feature/RepeatedDescription branch from 5567b75 to 69ce703 Compare December 28, 2016 02:10

Add RepeatedDescription cop

e3a62fc

backus force-pushed the feature/RepeatedDescription branch from 69ce703 to e3a62fc Compare December 28, 2016 02:19

backus merged commit 853af10 into master Dec 28, 2016

backus deleted the feature/RepeatedDescription branch December 28, 2016 02:22

markburns pushed a commit to markburns/rubocop-rails-ddd that referenced this pull request Nov 7, 2017

Merge pull request rubocop#259 from backus/feature/RepeatedDescription

d439224

Detect repeated descriptions in example groups

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect repeated descriptions in example groups #259

Detect repeated descriptions in example groups #259

backus commented Dec 26, 2016

backus commented Dec 26, 2016

bquorning left a comment

bquorning Dec 26, 2016

bquorning Dec 26, 2016

backus Dec 26, 2016

backus Dec 26, 2016

bquorning Dec 26, 2016

backus Dec 26, 2016

backus Dec 26, 2016

bquorning Dec 26, 2016

backus Dec 26, 2016

backus commented Dec 26, 2016

andyw8 Dec 27, 2016

backus Dec 28, 2016

andyw8 commented Dec 27, 2016

bquorning commented Dec 27, 2016

backus commented Dec 28, 2016

Detect repeated descriptions in example groups #259

Detect repeated descriptions in example groups #259

Conversation

backus commented Dec 26, 2016

backus commented Dec 26, 2016

bquorning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

backus commented Dec 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyw8 commented Dec 27, 2016

bquorning commented Dec 27, 2016

backus commented Dec 28, 2016