Skip to content

Fix assert_raises calls#2571

Merged
amomchilov merged 3 commits intomainfrom
Alex/fix-assert_raises-calls
Apr 1, 2026
Merged

Fix assert_raises calls#2571
amomchilov merged 3 commits intomainfrom
Alex/fix-assert_raises-calls

Conversation

@amomchilov
Copy link
Copy Markdown
Contributor

Motivation

These calls are trying to pass a String/Regex as a matcher, but it's actually just interpreted as an exception type to try to rescue. It's confusing the base Minitest version of assert_raises(*exp) for the augment Rails assert_raises(*exp, match:).

@amomchilov amomchilov requested a review from a team as a code owner April 1, 2026 17:23
end
e = assert_raises(NameError) { rbi_for(:Post) }

assert_match("uninitialized constant Post::User", e.message)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like assert_match will create a regexp anyway:

    pattern = case(pattern)
      when String
        Regexp.new(Regexp.escape(pattern))
      else
        pattern
    end

Wouldn't passing a literal regexp with /pattern/ be slightly faster and more memory efficient?

Copy link
Copy Markdown
Contributor Author

@amomchilov amomchilov Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AHaha i saw this change and had an inkling you might flag this. Won't someone think of the bytes?!

If it needs escaping, I'll keep the string format, because it's much easier to read without the backslashes (e.g. /sig \{ returns\(void\)\) \}/)

@amomchilov amomchilov force-pushed the Alex/fix-assert_raises-calls branch from 48e30d4 to 7c6c7d6 Compare April 1, 2026 19:59
@amomchilov amomchilov enabled auto-merge April 1, 2026 19:59
@amomchilov amomchilov merged commit 5e647eb into main Apr 1, 2026
18 of 19 checks passed
@amomchilov amomchilov deleted the Alex/fix-assert_raises-calls branch April 1, 2026 20:23
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.

4 participants