diff --git a/assert_changes_second_argument.patch b/assert_changes_second_argument.patch new file mode 100644 index 0000000000..3b7af402ad --- /dev/null +++ b/assert_changes_second_argument.patch @@ -0,0 +1,207 @@ +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 000000000..179ffc586 +--- /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 0e7d18346..c9759fca8 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 000000000..2adc3c930 +--- /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? ++ end ++ end ++ end ++ end ++end +diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb +index b7eddc4fe..b862ffc9d 100644 +--- a/lib/rubocop/cop/rails_cops.rb ++++ b/lib/rubocop/cop/rails_cops.rb +@@ -25,6 +25,7 @@ require_relative 'rails/application_job' + 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 000000000..f832b8076 +--- /dev/null ++++ b/spec/rubocop/cop/rails/assert_changes_second_argument_spec.rb +@@ -0,0 +1,95 @@ ++# 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 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 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..2adc3c9301 --- /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? + 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..f832b80767 --- /dev/null +++ b/spec/rubocop/cop/rails/assert_changes_second_argument_spec.rb @@ -0,0 +1,95 @@ +# 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 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