Skip to content

Cleanup parser specs#15446

Merged
straight-shoota merged 10 commits intocrystal-lang:masterfrom
FnControlOption:parser_spec
May 26, 2025
Merged

Cleanup parser specs#15446
straight-shoota merged 10 commits intocrystal-lang:masterfrom
FnControlOption:parser_spec

Conversation

@FnControlOption
Copy link
Contributor

Note: Commit ea79701 may be controversial. I slightly prefer explicit test cases over loops, but I have no strong feelings either way!

@straight-shoota
Copy link
Member

I too prefer unrolled loops for explicitness and grepability.

The loops however do provide value in grouping: They express that all examples are functionally equivalent, just testing different values.
This notion gets lost when they're just unrolled into the long list of examples.

Some sense of togetherness could be established by grouping the examples via describe. This would also assign a description which further helps understanding the purpose. We're missing that far too much in these massive spec files.

The other aspect of functional equivalence has particular relevance for more complex examples: Generalizing the implementation helps keep the code clean and easy to read. For example, extract the shared plumbing into a spec helper method and let the unrolled loop items call that explicitly. Perhaps that could be a strategy to explore for further enhancements of the more complex loops not touched in thie PR.
Of course this mechanism reduces grepability again if the tested source code is constructed in a helper method. It's all about balance.

# loop
%[a b c].each do |value|
  it "#{value} foo" do
    assert_fooing(value)
  end
end

# unrolled with helper
private def it_foos(value)
  it "#{value}.foo" do
    assert_fooing(value)
  end
end

it_foos "a"
it_foos "b"
it_foos "c"

For this PR I'd like to have the former loops grouped into describe blocks.
Unrolling further loops could be left for follow-ups.

@FnControlOption FnControlOption marked this pull request as draft February 21, 2025 23:50
@FnControlOption FnControlOption marked this pull request as ready for review February 23, 2025 07:34
Copy link
Collaborator

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

To be honest, I fail to see the benefit of unrolling the loops 🤷

@straight-shoota
Copy link
Member

Benefits:

  • Better location reporting. With the loop variant, you only get a failure location that points to the generic loop code. No indication of which value it was.
  • Better readability. It's super obvious what it_parses ":foo!", "foo!".symbol does. Try to figure that out from
    value = symbol[1, symbol.size - 1]
    value = value[1, value.size - 2] if value.starts_with?('"')
    it_parses symbol, value.symbol
  • Better grepability. Wanna look at examples involving the equality operator? Grep for def ==. Only you wouldn't find the loop examples because they generate the code at runtime.

@straight-shoota straight-shoota added this to the 1.17.0 milestone May 22, 2025
@straight-shoota straight-shoota merged commit b33ad7f into crystal-lang:master May 26, 2025
34 checks passed
@straight-shoota straight-shoota changed the title Compiler parser spec cleanup Cleanup parser specs May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants