From 874ad163a4d56e170bf8c6f548ac9e7bef0603e4 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Sun, 5 Jan 2020 17:50:13 -0800 Subject: [PATCH 1/4] Add autocorrect for RSpec/ExpectActual --- CHANGELOG.md | 2 + lib/rubocop/cop/rspec/expect_actual.rb | 16 ++++ manual/cops_rspec.md | 2 +- spec/rubocop/cop/rspec/expect_actual_spec.rb | 85 ++++++++++++++++++++ 4 files changed, 104 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c29237243..c1d0f1395 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ * Fix `RSpec/InstanceVariable` detection inside custom matchers. ([@pirj][]) * Fix `RSpec/ScatteredSetup` to distinguish hooks with different metadata. ([@pirj][]) +* Add autocorrect support for `RSpec/ExpectActual` cop. ([@dduugg][]) ## 1.37.1 (2019-12-16) @@ -475,3 +476,4 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features. [@mkrawc]: https://github.com/mkrawc [@jfragoulis]: https://github.com/jfragoulis [@ybiquitous]: https://github.com/ybiquitous +[@dduugg]: https://github.com/dduugg diff --git a/lib/rubocop/cop/rspec/expect_actual.rb b/lib/rubocop/cop/rspec/expect_actual.rb index c945499fd..8491ff756 100644 --- a/lib/rubocop/cop/rspec/expect_actual.rb +++ b/lib/rubocop/cop/rspec/expect_actual.rb @@ -49,6 +49,17 @@ def on_send(node) end end + def autocorrect(node) + lambda do |corrector| + expectation = node.parent.parent + rhs = expectation.children.last + return unless rhs.is_a?(RuboCop::AST::MethodDispatchNode) + return if rhs.method_name != :eq + + swap_order(corrector, node, rhs.children.last) + end + end + private # This is not implement using a NodePattern because it seems @@ -65,6 +76,11 @@ def complex_literal?(node) COMPLEX_LITERALS.include?(node.type) && node.each_child_node.all?(&method(:literal?)) end + + def swap_order(corrector, lhs_arg, rhs_arg) + corrector.replace(lhs_arg.source_range, rhs_arg.source) + corrector.replace(rhs_arg.source_range, lhs_arg.source) + end end end end diff --git a/manual/cops_rspec.md b/manual/cops_rspec.md index 6d586c324..1aec29405 100644 --- a/manual/cops_rspec.md +++ b/manual/cops_rspec.md @@ -977,7 +977,7 @@ IgnoredWords | `[]` | Array Enabled by default | Supports autocorrection --- | --- -Enabled | No +Enabled | Yes Checks for `expect(...)` calls containing literal values. diff --git a/spec/rubocop/cop/rspec/expect_actual_spec.rb b/spec/rubocop/cop/rspec/expect_actual_spec.rb index 503fe8fff..91ac8bbe7 100644 --- a/spec/rubocop/cop/rspec/expect_actual_spec.rb +++ b/spec/rubocop/cop/rspec/expect_actual_spec.rb @@ -18,6 +18,17 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq(123) + expect(bar).to eq(12.3) + expect(bar).to eq(1i) + expect(bar).to eq(1r) + end + end + RUBY end it 'flags boolean literal values within expect(...)' do @@ -31,6 +42,15 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq(true) + expect(bar).to eq(false) + end + end + RUBY end it 'flags string and symbol literal values within expect(...)' do @@ -44,6 +64,15 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq("foo") + expect(bar).to eq(:foo) + end + end + RUBY end it 'flags literal nil value within expect(...)' do @@ -55,6 +84,14 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq(nil) + end + end + RUBY end it 'does not flag dynamic values within expect(...)' do @@ -80,6 +117,15 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq([123]) + expect(bar).to eq([[123]]) + end + end + RUBY end it 'flags hashes containing only literal values within expect(...)' do @@ -93,6 +139,15 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq(foo: 1, bar: 2) + expect(bar).to eq(foo: 1, bar: [{}]) + end + end + RUBY end it 'flags ranges containing only literal values within expect(...)' do @@ -106,6 +161,15 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq(1..2) + expect(bar).to eq(1...2) + end + end + RUBY end it 'flags regexps containing only literal values within expect(...)' do @@ -117,6 +181,14 @@ end end RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eq(/foo|bar/) + end + end + RUBY end it 'does not flag complex values with dynamic parts within expect(...)' do @@ -135,6 +207,19 @@ RUBY end + it 'flags but does not autocorrect violations without eq' do + expect_offense(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect([1,2,3]).to include(a) + ^^^^^^^ Provide the actual you are testing to `expect(...)`. + end + end + RUBY + + expect_no_corrections + end + context 'when inspecting rspec-rails routing specs' do let(:cop_config) { {} } From 408f12979b67accd778c0ef4d414c5031b32b433 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Sat, 11 Jan 2020 15:52:06 +0300 Subject: [PATCH 2/4] Pull logic to node pattern --- lib/rubocop/cop/rspec/expect_actual.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/rubocop/cop/rspec/expect_actual.rb b/lib/rubocop/cop/rspec/expect_actual.rb index 8491ff756..5f07643a0 100644 --- a/lib/rubocop/cop/rspec/expect_actual.rb +++ b/lib/rubocop/cop/rspec/expect_actual.rb @@ -41,22 +41,26 @@ class ExpectActual < Cop regexp ].freeze - def_node_matcher :expect_literal, '(send _ :expect $#literal?)' + def_node_matcher :expect_literal, <<~PATTERN + (send + (send nil? :expect $#literal?) + #{Runners::ALL.node_pattern_union} + $(send nil? ...) + ) + PATTERN def on_send(node) expect_literal(node) do |argument| - add_offense(argument) + add_offense(node, location: argument.source_range) end end def autocorrect(node) + argument, matcher = expect_literal(node) lambda do |corrector| - expectation = node.parent.parent - rhs = expectation.children.last - return unless rhs.is_a?(RuboCop::AST::MethodDispatchNode) - return if rhs.method_name != :eq + return if matcher.method_name != :eq - swap_order(corrector, node, rhs.children.last) + swap_order(corrector, argument, matcher.arguments.first) end end From 412e5c5f8b24c136274727bbccbeb97f4619e4a4 Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Sat, 11 Jan 2020 15:56:48 +0300 Subject: [PATCH 3/4] Give params more descriptive names --- lib/rubocop/cop/rspec/expect_actual.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/rubocop/cop/rspec/expect_actual.rb b/lib/rubocop/cop/rspec/expect_actual.rb index 5f07643a0..3821c3088 100644 --- a/lib/rubocop/cop/rspec/expect_actual.rb +++ b/lib/rubocop/cop/rspec/expect_actual.rb @@ -60,7 +60,7 @@ def autocorrect(node) lambda do |corrector| return if matcher.method_name != :eq - swap_order(corrector, argument, matcher.arguments.first) + swap(corrector, argument, matcher.arguments.first) end end @@ -81,9 +81,9 @@ def complex_literal?(node) node.each_child_node.all?(&method(:literal?)) end - def swap_order(corrector, lhs_arg, rhs_arg) - corrector.replace(lhs_arg.source_range, rhs_arg.source) - corrector.replace(rhs_arg.source_range, lhs_arg.source) + def swap(corrector, actual, expected) + corrector.replace(actual.source_range, expected.source) + corrector.replace(expected.source_range, actual.source) end end end From f152aaafed541967fbd057551931f9b0029a501a Mon Sep 17 00:00:00 2001 From: Phil Pirozhkov Date: Sat, 11 Jan 2020 17:16:41 +0300 Subject: [PATCH 4/4] Extend matcher support in ExpectActual auto-correct --- CHANGELOG.md | 2 +- lib/rubocop/cop/rspec/expect_actual.rb | 13 ++- spec/rubocop/cop/rspec/expect_actual_spec.rb | 88 +++++++++++++++++++- 3 files changed, 97 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1d0f1395..aa827e6c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ * Fix `RSpec/InstanceVariable` detection inside custom matchers. ([@pirj][]) * Fix `RSpec/ScatteredSetup` to distinguish hooks with different metadata. ([@pirj][]) -* Add autocorrect support for `RSpec/ExpectActual` cop. ([@dduugg][]) +* Add autocorrect support for `RSpec/ExpectActual` cop. ([@dduugg][], [@pirj][]) ## 1.37.1 (2019-12-16) diff --git a/lib/rubocop/cop/rspec/expect_actual.rb b/lib/rubocop/cop/rspec/expect_actual.rb index 3821c3088..3bffe2179 100644 --- a/lib/rubocop/cop/rspec/expect_actual.rb +++ b/lib/rubocop/cop/rspec/expect_actual.rb @@ -41,11 +41,16 @@ class ExpectActual < Cop regexp ].freeze + SUPPORTED_MATCHERS = %i[eq eql equal be].freeze + def_node_matcher :expect_literal, <<~PATTERN (send (send nil? :expect $#literal?) #{Runners::ALL.node_pattern_union} - $(send nil? ...) + { + (send (send nil? $:be) :== $_) + (send nil? $_ $_ ...) + } ) PATTERN @@ -56,11 +61,11 @@ def on_send(node) end def autocorrect(node) - argument, matcher = expect_literal(node) + actual, matcher, expected = expect_literal(node) lambda do |corrector| - return if matcher.method_name != :eq + return unless SUPPORTED_MATCHERS.include?(matcher) - swap(corrector, argument, matcher.arguments.first) + swap(corrector, actual, expected) end end diff --git a/spec/rubocop/cop/rspec/expect_actual_spec.rb b/spec/rubocop/cop/rspec/expect_actual_spec.rb index 91ac8bbe7..852c1c5c8 100644 --- a/spec/rubocop/cop/rspec/expect_actual_spec.rb +++ b/spec/rubocop/cop/rspec/expect_actual_spec.rb @@ -207,7 +207,93 @@ RUBY end - it 'flags but does not autocorrect violations without eq' do + it 'ignores `be` with no argument' do + expect_no_offenses(<<~RUBY) + describe Foo do + it 'uses expect legitimately' do + expect(1).to be + end + end + RUBY + end + + it 'flags `be` with an argument' do + expect_offense(<<~RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(true).to be(a) + ^^^^ Provide the actual you are testing to `expect(...)`. + end + end + RUBY + + expect_correction(<<~RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(a).to be(true) + end + end + RUBY + end + + it 'flags `be ==`' do + expect_offense(<<~RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(1).to be == a + ^ Provide the actual you are testing to `expect(...)`. + end + end + RUBY + + expect_correction(<<~RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(a).to be == 1 + end + end + RUBY + end + + it 'flags with `eql` matcher' do + expect_offense(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(1).to eql(bar) + ^ Provide the actual you are testing to `expect(...)`. + end + end + RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to eql(1) + end + end + RUBY + end + + it 'flags with `equal` matcher' do + expect_offense(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(1).to equal(bar) + ^ Provide the actual you are testing to `expect(...)`. + end + end + RUBY + + expect_correction(<<-RUBY) + describe Foo do + it 'uses expect incorrectly' do + expect(bar).to equal(1) + end + end + RUBY + end + + it 'flags but does not autocorrect violations for other matchers' do expect_offense(<<-RUBY) describe Foo do it 'uses expect incorrectly' do