Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jan 3, 2024

Follow up to #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:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@koic
Copy link
Member

koic commented Jan 4, 2024

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

This comment was marked as outdated.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jan 4, 2024

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 :)

@G-Rath G-Rath force-pushed the fix-oldest-rubocop branch 2 times, most recently from 3645ace to e1b48d2 Compare January 6, 2024 17:59
@koic
Copy link
Member

koic commented Jan 7, 2024

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
Copy link
Contributor Author

G-Rath commented Jan 7, 2024

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
Copy link
Member

koic commented Mar 4, 2024

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.

@koic koic closed this Mar 4, 2024
@G-Rath G-Rath deleted the fix-oldest-rubocop branch March 4, 2024 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants