From 916ffbc103286ed2bffd3e8b12aa2b085cad4450 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Wed, 3 Jan 2024 08:19:36 +1300 Subject: [PATCH] [Fix #292] ensure all kinds of assignments are correctly handled when counting assertions --- ...fix_ensure_all_kinds_of_assignments_are.md | 1 + .../cop/minitest/multiple_assertions.rb | 18 +- .../cop/minitest/multiple_assertions_test.rb | 201 ++++++++++++++++++ 3 files changed, 216 insertions(+), 4 deletions(-) create mode 100644 changelog/fix_ensure_all_kinds_of_assignments_are.md diff --git a/changelog/fix_ensure_all_kinds_of_assignments_are.md b/changelog/fix_ensure_all_kinds_of_assignments_are.md new file mode 100644 index 00000000..a98a6932 --- /dev/null +++ b/changelog/fix_ensure_all_kinds_of_assignments_are.md @@ -0,0 +1 @@ +* [#292](https://github.com/rubocop/rubocop-minitest/issues/292): Ensure all kinds of assignments are correctly handled when counting assertions. ([@G-Rath][]) diff --git a/lib/rubocop/cop/minitest/multiple_assertions.rb b/lib/rubocop/cop/minitest/multiple_assertions.rb index 7c0c3871..74b603a8 100644 --- a/lib/rubocop/cop/minitest/multiple_assertions.rb +++ b/lib/rubocop/cop/minitest/multiple_assertions.rb @@ -74,11 +74,21 @@ def assertions_count_based_on_type(node) end def assertions_count_in_assignment(node) - # checking the direct expression is handled by assertion_method? - return 0 unless node.expression.block_type? || node.expression.numblock_type? + return assertions_count_based_on_type(node.expression) unless node.masgn_type? - # this will only trigger the branches for :block and :numblock type nodes - assertions_count_based_on_type(node.expression) + rhs = node.children.last + + case rhs.type + when :array + rhs.children.sum { |child| assertions_count_based_on_type(child) } + when :send + assertion_method?(rhs) ? 1 : 0 + else + # Play it safe and bail if we don't have any explicit handling for whatever + # the RHS type is, since at this point we're already probably dealing with + # a pretty exotic situation that's unlikely in the real world. + 0 + end end def assertions_count_in_branches(branches) diff --git a/test/rubocop/cop/minitest/multiple_assertions_test.rb b/test/rubocop/cop/minitest/multiple_assertions_test.rb index d48df5af..232bba9e 100644 --- a/test/rubocop/cop/minitest/multiple_assertions_test.rb +++ b/test/rubocop/cop/minitest/multiple_assertions_test.rb @@ -113,6 +113,49 @@ def test_asserts_once RUBY end + def test_does_not_crash_with_mass_assignments + assert_no_offenses(<<~RUBY) + class FooTest < Minitest::Test + def test_does_not_crash + _, _ = foo + _, _ = [] + _, _ = {} + _, _ = '' + _, _ = 1 + end + end + RUBY + end + + def test_all_types_of_assignments_are_understood + assert_offense(<<~RUBY) + class FooTest < Minitest::Test + def test_all_types_of_assignment + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [8/1]. + # lvasgn + foo = assert_equal(1, 1) + # ivasgn + @instance_variable = assert_equal(1, 1) + # cvasgn + @@class_variable = assert_equal(1, 1) + # gvasgn + $global_variable = assert_equal(1, 1) + # casgn + # MyClass::CONSTANT_VALUE = assert_equal(1, 1) + # masgn + a, b, c = assert_equal(1, 1) + a, b, c = [assert_equal(1, 1), assert_equal(1, 1), assert_equal(1, 1)] + # op_asgn + counter += assert_equal(1, 1) + # or_asgn + result ||= assert_equal(1, 1) + # and_asgn + flag &&= assert_equal(1, 1) + end + end + RUBY + end + def test_assignments_are_not_counted_twice assert_no_offenses(<<~RUBY) class FooTest < Minitest::Test @@ -295,6 +338,22 @@ class FooTest < ActiveSupport::TestCase RUBY end + def test_registers_offense_when_multiple_assertions_inside_assigned_conditional + assert_offense(<<~RUBY) + class FooTest < ActiveSupport::TestCase + test 'something' do + ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [2/1]. + _ = if condition + assert_equal(foo, bar) # 1 + assert_empty(array) # 2 + else + assert_equal(foo, baz) + end + end + end + RUBY + end + def test_does_not_register_offense_when_single_assertion_inside_conditional assert_no_offenses(<<~RUBY) class FooTest < ActiveSupport::TestCase @@ -330,6 +389,27 @@ class FooTest < ActiveSupport::TestCase RUBY end + def test_registers_offense_when_multiple_expectations_inside_assigned_case + assert_offense(<<~RUBY) + class FooTest < ActiveSupport::TestCase + test 'something' do + ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [3/1]. + _ = case + when condition1 + assert_equal(foo, bar) + assert_empty(array) + when condition2 + assert_equal(foo, bar) # 1 + assert_empty(array) # 2 + assert_equal(foo, baz) # 3 + else + assert_equal(foo, zoo) + end + end + end + RUBY + end + def test_does_not_register_offense_when_single_assertion_inside_case assert_no_offenses(<<~RUBY) class FooTest < ActiveSupport::TestCase @@ -366,6 +446,27 @@ class FooTest < ActiveSupport::TestCase RUBY end + def test_registers_offense_when_multiple_expectations_inside_assigned_pattern_matching + assert_offense(<<~RUBY) + class FooTest < ActiveSupport::TestCase + test 'something' do + ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [3/1]. + _ = case variable + in pattern1 + assert_equal(foo, bar) + assert_empty(array) + in pattern2 + assert_equal(foo, bar) # 1 + assert_empty(array) # 2 + assert_equal(foo, baz) # 3 + else + assert_equal(foo, zoo) + end + end + end + RUBY + end + def test_does_not_register_offense_when_single_assertion_inside_pattern_matching assert_no_offenses(<<~RUBY) class FooTest < ActiveSupport::TestCase @@ -399,6 +500,26 @@ class FooTest < ActiveSupport::TestCase RUBY end + def test_registers_offense_when_multiple_expectations_inside_assigned_rescue + assert_offense(<<~RUBY) + class FooTest < ActiveSupport::TestCase + test 'something' do + ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [3/1]. + _ = begin + do_something + rescue Foo + assert_equal(foo, bar) + assert_empty(array) + rescue Bar + assert_equal(foo, bar) # 1 + assert_empty(array) # 2 + assert_equal(foo, baz) # 3 + end + end + end + RUBY + end + def test_does_not_register_offense_when_single_assertion_inside_rescue assert_no_offenses(<<~RUBY) class FooTest < ActiveSupport::TestCase @@ -450,6 +571,86 @@ class FooTest < ActiveSupport::TestCase RUBY end + def test_registers_offense_when_complex_structure_with_assignments_and_multiple_assertions + configure_max_assertions(2) + + assert_offense(<<~RUBY) + class FooTest < ActiveSupport::TestCase + test 'something' do + ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [4/2]. + _= if condition1 + assert foo + elsif condition2 + assert foo + else + begin + do_something + assert foo # 1 + rescue Foo + assert foo + assert foo + rescue Bar + # noop + rescue Zoo + _ = case + when condition + assert foo # 2 + assert foo # 3 + assert foo # 4 + else + assert foo + assert foo + end + else + assert foo + end + end + end + end + RUBY + end + + def test_registers_offense_when_complex_multiple_assignment_structure_and_multiple_assertions + configure_max_assertions(2) + + assert_offense(<<~RUBY) + class FooTest < ActiveSupport::TestCase + test 'something' do + ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [5/2]. + _, _, _ = [ + if condition1 + assert foo + else + assert foo + end, + begin + do_something + assert foo # 1 + rescue Foo + assert foo + assert foo + rescue Bar + # noop + rescue Zoo + _ = case + when condition + assert foo # 2 + assert foo # 3 + assert foo # 4 + else + assert foo + assert foo + end + else + assert foo + end, + assert(foo) + ] + end + end + RUBY + end + def test_does_not_register_offense_when_complex_structure_with_single_assertion assert_no_offenses(<<~RUBY) class FooTest < ActiveSupport::TestCase