From e6af2761a32867c38fbf385c59a410ab1f3ca8b4 Mon Sep 17 00:00:00 2001 From: Joseph Carino Date: Mon, 23 Dec 2024 12:02:16 -0500 Subject: [PATCH] [Fix #1396] Add new cop assert_changes_second_argument --- .../new_assert_changes_second_argument_cop.md | 1 + config/default.yml | 7 ++ .../rails/assert_changes_second_argument.rb | 63 +++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + .../assert_changes_second_argument_spec.rb | 104 ++++++++++++++++++ 5 files changed, 176 insertions(+) create mode 100644 changelog/new_assert_changes_second_argument_cop.md create mode 100644 lib/rubocop/cop/rails/assert_changes_second_argument.rb create mode 100644 spec/rubocop/cop/rails/assert_changes_second_argument_spec.rb diff --git a/changelog/new_assert_changes_second_argument_cop.md b/changelog/new_assert_changes_second_argument_cop.md new file mode 100644 index 0000000000..179ffc586e --- /dev/null +++ b/changelog/new_assert_changes_second_argument_cop.md @@ -0,0 +1 @@ +* [#1396](https://github.com/rubocop/rubocop-rails/issues/1396): Add cop to prevent use of assert_changes with non-string second arguments. ([@joseph-carino][]) diff --git a/config/default.yml b/config/default.yml index 0e7d183460..c9759fca82 100644 --- a/config/default.yml +++ b/config/default.yml @@ -222,6 +222,13 @@ Rails/ArelStar: SafeAutoCorrect: false VersionAdded: '2.9' +Rails/AssertChangesSecondArgument: + Description: 'Do not use `assert_changes` with a non-string second argument.' + Enabled: true + VersionAdded: '2.28' + Include: + - '**/test/**/*' + Rails/AssertNot: Description: 'Use `assert_not` instead of `assert !`.' Enabled: true diff --git a/lib/rubocop/cop/rails/assert_changes_second_argument.rb b/lib/rubocop/cop/rails/assert_changes_second_argument.rb new file mode 100644 index 0000000000..2c0ac2e796 --- /dev/null +++ b/lib/rubocop/cop/rails/assert_changes_second_argument.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Checks misuse of the second argument to ActiveSupport `assert_changes` method + # + # `assert_changes`'s second argument is the failure message emitted when the + # first argument (expression) is unchanged in the block. + # + # A common mistake is to use `assert_changes` with the expected change + # value delta as the second argument. + # In that case `assert_changes` will validate only that the expression changes, + # not that it changes by a specific value. + # + # Users should provide the 'from' and 'to' parameters, + # or use `assert_difference` instead, which does support deltas. + # + # @example + # + # # bad + # assert_changes -> { @value }, 1 do + # @value += 1 + # end + # + # # good + # assert_changes -> { @value }, from: 0, to: 1 do + # @value += 1 + # end + # assert_changes -> { @value }, "Value should change" do + # @value += 1 + # end + # assert_difference -> { @value }, 1 do + # @value += 1 + # end + # + class AssertChangesSecondArgument < Base + extend AutoCorrector + + MSG = 'Use assert_changes to assert when an expression changes from and to specific values. ' \ + 'Use assert_difference instead to assert when an expression changes by a specific delta. ' \ + 'The second argument to assert_changes is the message emitted on assert failure.' + + def on_send(node) + return if node.receiver || !node.method?(:assert_changes) + return if node.arguments[1].nil? + + return unless offense?(node.arguments[1]) + + add_offense(node.loc.selector) do |corrector| + corrector.replace(node.loc.selector, 'assert_difference') + end + end + + private + + def offense?(arg) + !arg.hash_type? && !arg.str_type? && !arg.dstr_type? && !arg.sym_type? && !arg.dsym_type? && !arg.variable? + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index b7eddc4fe4..b862ffc9da 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -25,6 +25,7 @@ require_relative 'rails/application_mailer' require_relative 'rails/application_record' require_relative 'rails/arel_star' +require_relative 'rails/assert_changes_second_argument' require_relative 'rails/assert_not' require_relative 'rails/attribute_default_block_value' require_relative 'rails/belongs_to' diff --git a/spec/rubocop/cop/rails/assert_changes_second_argument_spec.rb b/spec/rubocop/cop/rails/assert_changes_second_argument_spec.rb new file mode 100644 index 0000000000..d371c36004 --- /dev/null +++ b/spec/rubocop/cop/rails/assert_changes_second_argument_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +RSpec.describe(RuboCop::Cop::Rails::AssertChangesSecondArgument, :config) do + let(:message) do + 'Use assert_changes to assert when an expression changes from and to specific values. ' \ + 'Use assert_difference instead to assert when an expression changes by a specific delta. ' \ + 'The second argument to assert_changes is the message emitted on assert failure.' + end + + describe('offenses') do + it 'adds offense when the second positional argument is an integer' do + expect_offense(<<~RUBY) + assert_changes @value, -1 do + ^^^^^^^^^^^^^^ #{message} + @value += 1 + end + RUBY + end + + it 'adds offense when the second positional argument is a float' do + expect_offense(<<~RUBY) + assert_changes @value, -1.0 do + ^^^^^^^^^^^^^^ #{message} + @value += 1 + end + RUBY + end + + it 'does not add offense when the second argument is a string' do + expect_no_offenses(<<~RUBY) + assert_changes @value, "Value should change" do + @value += 1 + end + RUBY + end + + it 'does not add offense when the second argument is an interpolated string' do + expect_no_offenses(<<~RUBY) + assert_changes @value, "\#{thing} should change" do + @value += 1 + end + RUBY + end + + it 'does not add offense when the second argument is a symbol' do + expect_no_offenses(<<~RUBY) + assert_changes @value, :should_change do + @value += 1 + end + RUBY + end + + it 'does not add offense when the second argument is an interpolated symbol' do + expect_no_offenses(<<~RUBY) + assert_changes @value, :"\#{thing}_should_change" do + @value += 1 + end + RUBY + end + + it 'does not add offense when the second argument is a variable' do + expect_no_offenses(<<~RUBY) + message = "Value should change" + assert_changes @value, message do + @value += 1 + end + RUBY + end + + it 'does not add offense when there is only one argument' do + expect_no_offenses(<<~RUBY) + assert_changes @value do + @value += 1 + end + RUBY + end + + it 'does not add offense when there is only one positional argument' do + expect_no_offenses(<<~RUBY) + assert_changes @value, from: 0 do + @value += 1 + end + RUBY + end + end + + describe('autocorrect') do + it 'autocorrects method from assert_changes to assert_difference' do + source = <<-RUBY + assert_changes @value, -1.0 do + @value += 1 + end + RUBY + + corrected_source = <<-RUBY + assert_difference @value, -1.0 do + @value += 1 + end + RUBY + + expect(autocorrect_source(source)).to(eq(corrected_source)) + end + end +end