Skip to content

Conversation

brasic
Copy link
Contributor

@brasic brasic commented Aug 30, 2022

The method_source gem was added to railties in #19216. It was used to determine the last line number of a given test method to support running tests by line number. But this is not something that requires an external dependency: Ripper can do this easily, and it has the added advantage of not using repeated calls to eval to do it.

Drop method_source and replace it with a simple handler for Ripper's on_def parser event.

I don't believe that there are any mainstream rubies at this point that can run Rails and don't support Ripper but correct me if I'm mistaken.

@rails-bot rails-bot bot added the railties label Aug 30, 2022
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

LGTM, however I don't think it's really worth giving it its own file and unit testing it.

Can you move that class inside test_unit/runner.rb and drop the tests? (assuming features relying on definition_for are already tested)

@brasic brasic force-pushed the drop-method-source branch from 964574d to 7273f53 Compare August 30, 2022 13:48
@brasic
Copy link
Contributor Author

brasic commented Aug 30, 2022

@byroot thanks, done!

assuming features relying on definition_for are already tested

For the most part this is true although I added one test for functionality that I didn't see exercised (using multiple line number test selection syntax with def test_ syntax)

The `method_source` gem was added in
rails#19216. It was used to determine the
last line number of a given test method to support running tests by line
number. But this is not something that requires an external dependency:
Ripper can do this easily, and it has the added advantage of not using
repeated calls to `eval` to do it.

Drop `method_source` and replace it with a simple handler for Ripper's
`on_def` parser event.

I don't believe that there are any mainstream rubies at this point that
can run Rails and don't support Ripper but correct me if I'm wrong.
@brasic brasic force-pushed the drop-method-source branch from 7273f53 to 7690290 Compare August 30, 2022 13:52
@byroot
Copy link
Member

byroot commented Aug 30, 2022

Looks great, thank you!

I'll merge on green.

@byroot byroot merged commit 964a06b into rails:main Aug 30, 2022
@rafaelfranca
Copy link
Member

This doesn't work with tests defined using test. on_def will not run on those since there isn't a def from the in the source file. Reverting.

@brasic
Copy link
Contributor Author

brasic commented Sep 9, 2022

Thanks @rafaelfranca, that makes sense. I guess there must have been missing test coverage for that and I didn't consider it. I'll retool and open a new PR that covers the test macro.

@rafaelfranca
Copy link
Member

I guess there must have been missing test coverage for that and I didn't consider it.

Yeah, existing test wasn't failing but I updated it in the revert PR to fail with those code changes.

I'll retool and open a new PR that covers the test macro.

Thank you! Thank would be great.

@brasic
Copy link
Contributor Author

brasic commented Dec 5, 2022

I spent a long time sitting on a half-finished version of this, but I've finally opened #46644.

@brasic brasic deleted the drop-method-source branch December 5, 2022 03:57
brasic pushed a commit to brasic/rails that referenced this pull request Dec 5, 2022
The `method_source` gem was added in
rails#19216. It was used to determine the
last line number of a given test method to support running tests by line
number.

But this is not something that requires an external dependency:
Ripper can do this easily, and it has the added advantage of not using
`eval` calls in a loop to do it as method_source does.

It gets a bit trickier when dealing with declarative `test "some test"`
style methods, but ripper can still handle those in a similar way.

This is a second try at a PR (rails#45904)
that got rolled back because the previous effort didn't handle the
declarative test style.
brasic pushed a commit to brasic/rails that referenced this pull request Dec 5, 2022
The `method_source` gem was added in
rails#19216. It was used to determine the
last line number of a given test method to support running tests by line
number.

But this is not something that requires an external dependency:
Ripper can do this easily, and it has the added advantage of not using
`eval` calls in a loop to do it as method_source does.

It gets a bit trickier when dealing with declarative `test "some test"`
style methods, but ripper can still handle those in a similar way.

This is a second try at a PR (rails#45904)
that got rolled back because the previous effort didn't handle the
declarative test style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants