diff --git a/.rubocop.yml b/.rubocop.yml index 59c5d81f4..f93a6c580 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -294,4 +294,5 @@ Performance/ZipWithoutBlock: {Enabled: true} RSpec/IncludeExamples: {Enabled: true} RSpec/LeakyLocalVariable: {Enabled: true} +RSpec/HaveAttributes: {Enabled: true} RSpec/Output: {Enabled: true} diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a01452ef..8c91226ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Master (Unreleased) - Add support for `itblock` nodes. ([@Darhazer]) +- Add new cop `RSpec/HaveAttributes`. ([@Darhazer]) - `RSpec/ScatteredLet` now preserves the order of `let`s during auto-correction. ([@Darhazer]) - Fix a false negative for `RSpec/EmptyLineAfterFinalLet` inside `shared_examples` / `include_examples` / `it_behaves_like` blocks. ([@Darhazer]) - Add autocorrect support for `RSpec/SubjectDeclaration`. ([@eugeneius]) diff --git a/config/default.yml b/config/default.yml index 351bfc735..52ede781a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -482,6 +482,12 @@ RSpec/Focus: VersionChanged: '2.31' Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Focus +RSpec/HaveAttributes: + Description: Checks for expectations on the same object that can be combined. + Enabled: pending + VersionAdded: "<>" + Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/HaveAttributes + RSpec/HookArgument: Description: Checks the arguments passed to `before`, `around`, and `after`. Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 9c1e7746f..7a66226f6 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -44,6 +44,7 @@ * xref:cops_rspec.adoc#rspecexpectinlet[RSpec/ExpectInLet] * xref:cops_rspec.adoc#rspecexpectoutput[RSpec/ExpectOutput] * xref:cops_rspec.adoc#rspecfocus[RSpec/Focus] +* xref:cops_rspec.adoc#rspechaveattributes[RSpec/HaveAttributes] * xref:cops_rspec.adoc#rspechookargument[RSpec/HookArgument] * xref:cops_rspec.adoc#rspechooksbeforeexamples[RSpec/HooksBeforeExamples] * xref:cops_rspec.adoc#rspecidenticalequalityassertion[RSpec/IdenticalEqualityAssertion] diff --git a/docs/modules/ROOT/pages/cops_rspec.adoc b/docs/modules/ROOT/pages/cops_rspec.adoc index 822aeee51..37afdcc1f 100644 --- a/docs/modules/ROOT/pages/cops_rspec.adoc +++ b/docs/modules/ROOT/pages/cops_rspec.adoc @@ -2395,6 +2395,44 @@ focus 'test' do; end * https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/Focus +[#rspechaveattributes] +== RSpec/HaveAttributes + +|=== +| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed + +| Pending +| Yes +| Always +| <> +| - +|=== + +Checks for expectations on the same object that can be combined. + +[#examples-rspechaveattributes] +=== Examples + +[source,ruby] +---- +# bad +expect(obj.foo).to eq(bar) +expect(obj.fu).to eq(bax) +expect(obj.name).to eq(baz) + +# good +expect(obj).to have_attributes( + foo: bar, + fu: bax, + name: baz +) +---- + +[#references-rspechaveattributes] +=== References + +* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/HaveAttributes + [#rspechookargument] == RSpec/HookArgument diff --git a/lib/rubocop/cop/rspec/have_attributes.rb b/lib/rubocop/cop/rspec/have_attributes.rb new file mode 100644 index 000000000..8f08ac0a5 --- /dev/null +++ b/lib/rubocop/cop/rspec/have_attributes.rb @@ -0,0 +1,228 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module RSpec + # Checks for expectations on the same object that can be combined. + # + # @example + # # bad + # expect(obj.foo).to eq(bar) + # expect(obj.fu).to eq(bax) + # expect(obj.name).to eq(baz) + # + # # good + # expect(obj).to have_attributes( + # foo: bar, + # fu: bax, + # name: baz + # ) + # + class HaveAttributes < Base + extend AutoCorrector + include RangeHelp + + MSG = 'Combine multiple expectations on the same object ' \ + 'using `have_attributes`.' + + # Mapping of RSpec matchers to their have_attributes equivalents + # nil means use the value directly (for eq) + MATCHER_MAPPING = { + eq: nil, + be_an_instance_of: :an_instance_of, + be_within: :a_value_within, + contain_exactly: :a_collection_containing_exactly, + end_with: :a_string_ending_with, + start_with: :a_string_starting_with + }.freeze + + # @!method expect_method_matcher?(node) + def_node_matcher :expect_method_matcher?, <<~PATTERN + (send + (send nil? :expect + (send $_ $_) + ) + :to + (send nil? $_ $_) + ) + PATTERN + + # @!method expectation_statement?(node) + def_node_matcher :expectation_statement?, <<~PATTERN + (send (send nil? :expect ...) {:to :not_to :to_not} ...) + PATTERN + + def on_block(node) # rubocop:disable InternalAffairs/NumblockHandler, InternalAffairs/ItblockHandler + return unless example?(node) + + body = node.body + return unless body + + statements = body.begin_type? ? body.children : [body] + find_consecutive_groups(statements).each do |group| + flag_group(group) + end + end + + private + + def find_consecutive_groups(statements) + groups = [] + current_groups = {} + + statements.each do |statement| + exp = extract_expectation(statement) + if exp + obj_key = exp[:object].source + (current_groups[obj_key] ||= []) << exp + elsif expectation_statement?(statement) + # An expect call we can't combine (unsupported matcher, + # not_to, etc.) — doesn't break consecutive chains + else + # Non-expectation statement breaks all consecutive chains + flush_groups(current_groups, groups) + current_groups = {} + end + end + + flush_groups(current_groups, groups) + groups + end + + def extract_expectation(statement) + expect_method_matcher?(statement) do |obj, method, matcher, value| + next if obj.nil? || !MATCHER_MAPPING.key?(matcher) + + return { + node: statement, + object: obj, + method: method, + matcher: matcher, + value: value + } + end + end + + def flush_groups(current_groups, groups) + current_groups.each_value do |group| + groups << group if group.size >= 2 + end + end + + def flag_group(group) + # Sort by line number to maintain order + sorted_group = group.sort_by { |exp| exp[:node].loc.line } + + # Flag all nodes in the group, but only correct once + sorted_group.each_with_index do |exp, index| + add_offense(exp[:node]) do |corrector| + # Only correct on the first offense to avoid multiple corrections + if index.zero? + AttributesCorrector.new(sorted_group).call(corrector) + end + end + end + end + + # :nodoc: + class AttributesCorrector + include RangeHelp + + def initialize(group) + # Sort nodes by position + @sorted_nodes = group.sort_by do |exp| + exp[:node].source_range.begin_pos + end + end + + def call(corrector) + first_node = sorted_nodes.first[:node] + + # Replace the first node with the combined expectation + replacement = build_replacement + corrector.replace(first_node, replacement) + + # Remove the remaining nodes individually + sorted_nodes[1..].each do |exp| + node_range = range_by_whole_lines( + exp[:node].source_range, + include_final_newline: true, + buffer: exp[:node].source_range.source_buffer + ) + corrector.remove(node_range) + end + end + + private + + attr_reader :sorted_nodes + + def build_attributes + method_groups = {} + sorted_nodes.each do |exp| + (method_groups[exp[:method]] ||= []) << exp + end + + method_groups.map do |method_name, exps| + value_str = if exps.size == 1 + exp = exps.first + transform_value(exp[:matcher], exp[:value]) + else + combine_matchers(exps) + end + "#{method_name}: #{value_str}" + end.join(",\n ") + end + + def combine_matchers(exps) + parts = exps.map do |exp| + transform_value_for_and(exp[:matcher], exp[:value]) + end + parts[1..].inject(parts[0]) { |acc, part| "#{acc}.and(#{part})" } + end + + def transform_value_for_and(matcher, value) + have_attributes_matcher = HaveAttributes::MATCHER_MAPPING[matcher] + if have_attributes_matcher.nil? + "eq(#{wrap_keyword_arguments(value)})" + else + "#{have_attributes_matcher}(#{value.source})" + end + end + + def transform_value(matcher, value) + have_attributes_matcher = HaveAttributes::MATCHER_MAPPING[matcher] + + if have_attributes_matcher.nil? + # For eq, use value directly + # If value is keyword arguments (hash without braces), wrap in {} + wrap_keyword_arguments(value) + else + # For other matchers, wrap value in the have_attributes matcher + "#{have_attributes_matcher}(#{value.source})" + end + end + + def wrap_keyword_arguments(value) + source = value.source + if value.hash_type? && !source.strip.start_with?('{') + "{ #{source} }" + else + source + end + end + + def build_replacement + obj = sorted_nodes.first[:object] + attributes = build_attributes + <<~RUBY.chomp + expect(#{obj.source}).to have_attributes( + #{attributes} + ) + RUBY + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rspec_cops.rb b/lib/rubocop/cop/rspec_cops.rb index 877d466f2..381e24484 100644 --- a/lib/rubocop/cop/rspec_cops.rb +++ b/lib/rubocop/cop/rspec_cops.rb @@ -42,6 +42,7 @@ require_relative 'rspec/expect_in_let' require_relative 'rspec/expect_output' require_relative 'rspec/focus' +require_relative 'rspec/have_attributes' require_relative 'rspec/hook_argument' require_relative 'rspec/hooks_before_examples' require_relative 'rspec/identical_equality_assertion' diff --git a/spec/rubocop/cop/rspec/have_attributes_spec.rb b/spec/rubocop/cop/rspec/have_attributes_spec.rb new file mode 100644 index 000000000..761da6469 --- /dev/null +++ b/spec/rubocop/cop/rspec/have_attributes_spec.rb @@ -0,0 +1,305 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::RSpec::HaveAttributes do + it 'registers an offense for multiple expectations on the same object' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.fu).to eq(bax) + ^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.name).to eq(baz) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: bar, + fu: bax, + name: baz + ) + end + RUBY + end + + it 'does not register an offense for single expectation' do + expect_no_offenses(<<~RUBY) + it 'checks attribute' do + expect(obj.foo).to eq(bar) + end + RUBY + end + + it 'does not register an offense when there is no method receiver' do + expect_no_offenses(<<~RUBY) + it 'checks method calls' do + expect(foo).to eq(bar) + expect(baz).to eq(bar) + end + RUBY + end + + it 'does not register an offense for expectations on different objects' do + expect_no_offenses(<<~RUBY) + it 'checks different objects' do + expect(obj1.foo).to eq(bar) + expect(obj2.fu).to eq(bax) + end + RUBY + end + + it 'does not register an offense for method chains' do + expect_no_offenses(<<~RUBY) + it 'checks method chain' do + expect(obj.foo.bar).to eq(baz) + expect(obj.fu.bax).to eq(qux) + end + RUBY + end + + it 'does not register an offense for method calls with arguments' do + expect_no_offenses(<<~RUBY) + it 'checks method with args' do + expect(obj.foo(1)).to eq(bar) + expect(obj.fu(2)).to eq(bax) + end + RUBY + end + + it 'does not register an offense when only a single matcher is supported' do + expect_no_offenses(<<~RUBY) + it 'checks with other matchers' do + expect(obj.foo).to be(bar) + expect(obj.fu).to include(bax) + end + RUBY + end + + it 'does not register an offense for not_to expectations' do + expect_no_offenses(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).not_to eq(bar) + expect(obj.fu).not_to eq(bax) + end + RUBY + end + + it 'does not register an offense for mixed to and not_to expectations' do + expect_no_offenses(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + expect(obj.fu).not_to eq(bax) + end + RUBY + end + + it 'handles different value types' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq('bar') + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.num).to eq(42) + ^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.flag).to eq(true) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: 'bar', + num: 42, + flag: true + ) + end + RUBY + end + + it 'handles multiple offending objects' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj2.foo).to eq(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.fu).to eq(bax) + ^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj2.fu).to eq(bax) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: bar, + fu: bax + ) + expect(obj2).to have_attributes( + foo: bar, + fu: bax + ) + end + RUBY + end + + it 'does not register an offense when expectations are separated ' \ + 'by other code' do + expect_no_offenses(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + some_helper_method + expect(obj.fu).to eq(bax) + another_statement + expect(obj.name).to eq(baz) + end + RUBY + end + + it 'groups only consecutive expectations separated by other code' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.fu).to eq(bax) + ^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + another_statement + expect(obj.name).to eq(baz) + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: bar, + fu: bax + ) + another_statement + expect(obj.name).to eq(baz) + end + RUBY + end + + it 'does not break consecutive chains when uncombable expect is between ' \ + 'combinable ones' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.name).not_to eq(baz) + expect(obj.fu).to eq(bax) + ^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: bar, + fu: bax + ) + expect(obj.name).not_to eq(baz) + end + RUBY + end + + it 'ignores unsupported matchers' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.fu).to eq(bax) + ^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.name).to be(baz) + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: bar, + fu: bax + ) + expect(obj.name).to be(baz) + end + RUBY + end + + it 'transforms mixed matchers' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to start_with(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.name).to contain_exactly(baz) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.type).to be_an_instance_of(String) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.value).to be_within(0.1) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.suffix).to end_with(qux) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.status).to eq(200) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: a_string_starting_with(bar), + name: a_collection_containing_exactly(baz), + type: an_instance_of(String), + value: a_value_within(0.1), + suffix: a_string_ending_with(qux), + status: 200 + ) + end + RUBY + end + + it 'combines duplicate attribute expectations with different matchers ' \ + 'using .and' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(obj.foo).to eq(bar) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.foo).to start_with(baz) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(obj.name).to eq(qux) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(obj).to have_attributes( + foo: eq(bar).and(a_string_starting_with(baz)), + name: qux + ) + end + RUBY + end + + it 'wraps keyword arguments in braces' do + expect_offense(<<~RUBY) + it 'checks attributes' do + expect(error.sentry_context).to eq(extra: { http_status_code: response_status, http_body: response_body }) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + expect(error.status).to eq(200) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine multiple expectations on the same object using `have_attributes`. + end + RUBY + + expect_correction(<<~RUBY) + it 'checks attributes' do + expect(error).to have_attributes( + sentry_context: { extra: { http_status_code: response_status, http_body: response_body } }, + status: 200 + ) + end + RUBY + end +end diff --git a/spec/rubocop/cop/rspec/mixin/location_help_spec.rb b/spec/rubocop/cop/rspec/mixin/location_help_spec.rb index 851b90081..d5ad2f239 100644 --- a/spec/rubocop/cop/rspec/mixin/location_help_spec.rb +++ b/spec/rubocop/cop/rspec/mixin/location_help_spec.rb @@ -12,8 +12,10 @@ let(:source) { 'a b' } it 'returns #' do - expect(arguments_with_whitespace.begin_pos).to eq 1 - expect(arguments_with_whitespace.end_pos).to eq 3 + expect(arguments_with_whitespace).to have_attributes( + begin_pos: 1, + end_pos: 3 + ) end end @@ -21,8 +23,10 @@ let(:source) { 'foo 1, 2' } it 'returns #' do - expect(arguments_with_whitespace.begin_pos).to eq 3 - expect(arguments_with_whitespace.end_pos).to eq 8 + expect(arguments_with_whitespace).to have_attributes( + begin_pos: 3, + end_pos: 8 + ) end end @@ -30,8 +34,10 @@ let(:source) { 'foo(bar, baz)' } it 'returns #' do - expect(arguments_with_whitespace.begin_pos).to eq 3 - expect(arguments_with_whitespace.end_pos).to eq 13 + expect(arguments_with_whitespace).to have_attributes( + begin_pos: 3, + end_pos: 13 + ) end end end @@ -64,8 +70,10 @@ let(:source) { 'a { b }' } it 'returns #' do - expect(block_with_whitespace.begin_pos).to eq 1 - expect(block_with_whitespace.end_pos).to eq 7 + expect(block_with_whitespace).to have_attributes( + begin_pos: 1, + end_pos: 7 + ) end end @@ -74,8 +82,10 @@ let(:source) { 'foo { bar + baz }' } it 'returns #' do - expect(block_with_whitespace.begin_pos).to eq 3 - expect(block_with_whitespace.end_pos).to eq 17 + expect(block_with_whitespace).to have_attributes( + begin_pos: 3, + end_pos: 17 + ) end end end