Skip to content

Extract #assert_rewrites_rbs helper#939

Merged
amomchilov merged 1 commit into
mainfrom
Alex/assert_rewrites_rbs
Jun 5, 2026
Merged

Extract #assert_rewrites_rbs helper#939
amomchilov merged 1 commit into
mainfrom
Alex/assert_rewrites_rbs

Conversation

@amomchilov
Copy link
Copy Markdown
Contributor

@amomchilov amomchilov commented Jun 5, 2026

Background

As part of #787, I'm building a more comprehensive test suite that will ensure that the RBS-to-Sorbet translator preserves line numbering.

Achieving correct line numbers while still keeping the same meaning will require the translator to produce some pretty funky code, which is great for machines, but sucks for humans. So we'll want to support both:

  1. a more human-readable format that limits line lengths, has reasonable new lines for clarity, etc.
    • this is what we already have
  2. a line-matching format that "works" for machines, and preserves the correctness of line numbers in back traces, __LINE__, etc.
    • this is what I'm working on building

This PR

Refactors our existing tests into a new #assert_rewrites_rbs helper. Currently it just has one input and one expected output.

In a future PR, I'll be adding a second expected output for each test case. WIP, but you can get an idea here: 658304d

Very easy to review if you hide whitespace changes.

Alternatives considered

Make a dedicated test class for the line-matching rewriter. That way this test class' test cases have one input and one (human-readable) expected output, and the other class' test cases have one input and one (machine-readable) expected output.

I tried that out, but it became obvious that the two suites would be hard to keep in sync, and we would surely have new additions in the future that are only tested in one mode or the other, rather than both.

@amomchilov amomchilov marked this pull request as ready for review June 5, 2026 19:49
@amomchilov amomchilov requested a review from a team as a code owner June 5, 2026 19:49
@amomchilov amomchilov marked this pull request as draft June 5, 2026 19:50
@amomchilov amomchilov force-pushed the Alex/assert_rewrites_rbs branch from a435d91 to 68dc474 Compare June 5, 2026 19:53
@amomchilov amomchilov marked this pull request as ready for review June 5, 2026 19:54
#| ?max_line_length: Integer?,
#| ?overloads_strategy: Symbol
#| ) -> String
def assert_rewrites_rbs(
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 there are 7 occurrences of tests where the from and to_pretty_format_for_humans is equivalent. Wdyt about adding a default value so that if they're equal the callsites won't have to specify the kwargs:

def assert_rewrites_rbs(
  from = nil,
  to: from,
  max_line_length: nil,
  overloads_strategy: :translate_all
)

Copy link
Copy Markdown
Contributor Author

@amomchilov amomchilov Jun 5, 2026

Choose a reason for hiding this comment

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

Spoke offline. Will following up with something similar, but as a separate method like assert_rbs_rewrite_is_no_op.

That way you don't have a call with a from: but not to:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh also, some things that are no-op today, might not be no-op in the new rewriter mode. I'll explain it in the PR when I open it.

Comment thread test/spoom/sorbet/translate/rbs_comments_to_sorbet_sigs_test.rb Outdated
source_with_rbs = from
expected_pretty_format = to_pretty_format_for_humans

begin # Validate the human-readable rewrite
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.

Why is this wrapped in a begin?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just for visual grouping

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.

Felt weird at first sight, but it quickly grew up on me 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For context, up the stack there will be two of these, running different sets of assertions/validations against the two different outputs:

begin # Validate the human-readable rewrite
rewritten_output = rbs_comments_to_sorbet_sigs(
source_with_rbs,
max_line_length:,
overloads_strategy:,
)
assert_equal(expected_pretty_format, rewritten_output)
end
begin # Validate the line-matched format
case expected_line_matched_format
when :same_as_pretty_output
expected_line_matched_format = expected_pretty_format
when Symbol
raise ArgumentError, "Invalid symbol for expected_line_matched_format: #{expected_line_matched_format}"
end
# assert_equal(source_with_rbs.lines.count, expected_line_matched_format.lines.count, <<~MSG)
# Precondition: the expected rewritten code should have the same line count as the RBS-containing input.
# This is a mistake in the test case, not the rewriter.
# MSG
rewritten_output = rbs_comments_to_sorbet_sigs(
source_with_rbs,
max_line_length:,
overloads_strategy:,
)
# TODO: run the validator to compare the two results
assert_equal(expected_line_matched_format, rewritten_output)
end

@amomchilov amomchilov force-pushed the Alex/assert_rewrites_rbs branch from 68dc474 to 9a3d00b Compare June 5, 2026 22:02
Comment thread test/spoom/sorbet/translate/rbs_comments_to_sorbet_sigs_test.rb Outdated
@amomchilov amomchilov force-pushed the Alex/assert_rewrites_rbs branch from 9a3d00b to b35b615 Compare June 5, 2026 22:31
@amomchilov amomchilov enabled auto-merge June 5, 2026 22:32
@amomchilov amomchilov merged commit e647dbb into main Jun 5, 2026
13 checks passed
@amomchilov amomchilov deleted the Alex/assert_rewrites_rbs branch June 5, 2026 22:41
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.

3 participants