rubocop / rubocop-minitest

Code style checking for Minitest files.
https://docs.rubocop.org/rubocop-minitest
MIT License
144 stars 44 forks source link

Skip pattern matching tests when running on older versions of Rubocop #297

Closed G-Rath closed 9 months ago

G-Rath commented 11 months ago

Follow up to https://github.com/rubocop/rubocop-minitest/pull/294

I don't know why but these two pattern matching tests throw parsing errors on Rubocop <1.52 - I can't find anything in the changelog to indicate what actually changed in v1.52, nor can I reproduce this locally outside of the test suite; I've also checked the versions of other gems (notably rubocop-ast) and confirmed the issues seems to be within rubocop itself.

Since there are only two tests impacted I think this is fine - even if no one spends the time figuring out the specifics to improve this, eventually it'll go away when support for Ruby 2.7 and older versions of Rubocop are dropped 🤷


Before submitting the PR make sure the following are checked:

koic commented 11 months ago

Actually, the parsing of Ruby source code depends on the Ruby version used for analysis, not the runtime Ruby version. Can you update it to refer value of target_ruby_version instead of RuboCop's version?

G-Rath commented 11 months ago

I'm not at my laptop right now so can't double check, but I did my testing using Ruby 2.7 and went through every version of RoboCop - so unless the bug is with RoboCops detention of Ruby features I'm not sure that's it...

(Also, all the other CIs are passing with latest RuboCop which includes Ruby 2.7)

G-Rath commented 11 months ago

Ok right I see what you mean - it's the (default) version of Ruby being targeted by Rubocop, which got changed to 2.7 in v1.50 which is why those versions pass.

I've updated my check :)

koic commented 11 months ago

Ah, I'm sorry, it seems I misunderstood this. Can you revert to the previous implementation? https://github.com/rubocop/rubocop-minitest/compare/91950bb2a23dcdb5f97f9ccc6f014cb62898b296..8847638552bd2faf9512572a774f19a18c6bdab9

However, it appears the tests fail with RuboCop 1.50, not 1.51. And,

    segments = Gem::Version.new(RuboCop::Version.version).segments

-   # pattern matching cannot be parsed until v1.52 for some reason
-   skip if segments[0] <= 1 && segments[2] <= 51
+   skip 'Pattern matching tests fail for some reason until version 1.51.' if segments[0] <= 1 && segments[2] <= 50
    segments = Gem::Version.new(RuboCop::Version.version).segments

Note: The following is repro steps with RuboCop 1.50:

% bundle exec ruby -Itest test/rubocop/cop/minitest/multiple_assertions_test.rb
(snip)

  1) Error:
MultipleAssertionsTest#test_does_not_register_offense_when_single_assertion_inside_pattern_matching:
RuntimeError: Error parsing example code
    /Users/koic/src/github.com/rubocop/rubocop-minitest/lib/rubocop/minitest/assert_offense.rb:165:in `inspect_source'
    /Users/koic/src/github.com/rubocop/rubocop-minitest/lib/rubocop/minitest/assert_offense.rb:98:in `assert_no_offenses'
    test/rubocop/cop/minitest/multiple_assertions_test.rb:471:in `test_does_not_register_offense_when_single_assertion_inside_pattern_matching'

  2) Failure:
MultipleAssertionsTest#test_registers_offense_when_multiple_expectations_inside_pattern_matching [test/rubocop/cop/minitest/multiple_assertions_test.rb:429]:
--- expected
+++ actual
@@ -1,6 +1,5 @@
 "class FooTest < ActiveSupport::TestCase
   test 'something' do
-  ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [3/1].
     case variable
     in pattern1
       assert_equal(foo, bar)

  3) Failure:
MultipleAssertionsTest#test_registers_offense_when_multiple_expectations_inside_assigned_pattern_matching [test/rubocop/cop/minitest/multiple_assertions_test.rb:450]:
--- expected
+++ actual
@@ -1,6 +1,5 @@
 "class FooTest < ActiveSupport::TestCase
   test 'something' do
-  ^^^^^^^^^^^^^^^^^^^ Test case has too many assertions [3/1].
     _ = case variable
         in pattern1
           assert_equal(foo, bar)

37 runs, 36 assertions, 2 failures, 1 errors, 0 skips
G-Rath commented 11 months ago

Ah, I'm sorry, it seems I misunderstood this

I don't think you did? I agree with the new change - the issue is with the version of Ruby that Rubocop is targeting, and it happens the default changed in v1.50 (which yes I realised I mixed up with v1.51); both versions of my fix would work but checking the RuboCop target version is the better one because technically just because the default target Ruby version is now 2.7, you could change it back to 2.6.

koic commented 9 months ago

As indicated by the failing test, it was not the Ruby version or pattern matching that caused the flaky tests: https://github.com/rubocop/rubocop-minitest/actions/runs/8134207387/job/22226821848?pr=304

The issue is a race condition caused by the implementation of the AssertOffense testing module. As a workaround, I have opened #305. Thank you for investigating.