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

Improve detection for rails as the testing framework #2161

Closed
wants to merge 1 commit into from

Conversation

Earlopain
Copy link
Contributor

Motivation

In some preparation of #2096

Note that this is only for detection of the testing framework, the issue is about installing ruby-lsp-rails. It may be relatively straight-forward to do this as well but I'm more familiar with this part of the code.

So, just do it here to talk roughtly about the implementation direction. I do plan to look properly at the issue after this and that should hopefully only involve moving a few things around.

Perhaps the GlobalState can even be created earlier so that SetupBundler has access to this method. Though the location of config/application.rb depend on workspace_uri which does get modified in apply_options .

Implementation

I created a listener for this which checks for Rails::Application inheritance.

This does parse the file twice, once on startup and once indexing actually starts. I don't see an easy way around this since proper indexing only starts once the server received initialized.

Automated Tests

One for the happy path, one for the other.

Manual Tests

I tested this out on my app which doesn't depend on rails but some subcompontents and that works fine.

The bundler setup isn't impacted by this
@Earlopain Earlopain requested a review from a team as a code owner June 11, 2024 18:14
@Earlopain Earlopain requested review from andyw8 and st0012 June 11, 2024 18:14
Copy link
Contributor

@louim louim left a comment

Choose a reason for hiding this comment

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

This is nice! It will only require a small addition to implement #2096. I can do so in a subsequent PR to keep things separate.


sig { returns(T.nilable(String)) }
def rails_app_config
path = File.join(workspace_path, "config/application.rb")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it's possible that the file is not right there, but would be nested deeper? I'm not too familiar with the different way people configure their project in relation with the workspace. At the same time, it might be best to stay with the simple solution.

Copy link
Contributor

@andyw8 andyw8 Jun 12, 2024

Choose a reason for hiding this comment

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

It's possible in theory for it to be in other locations, but would be very unusual, so I don't think we should worry about that.

Copy link
Contributor

Choose a reason for hiding this comment

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

(and we already require that the Rails app is at the root of a workspace, so that's not a concern).

Comment on lines +178 to +182
dispatcher = Prism::Dispatcher.new
listener = Listeners::RailsApp.new(dispatcher)
dispatcher.dispatch(result.value)

listener.rails_app
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably have done it with a simple regex, but your solution looks much cleaner 👌🏼

@andyw8 andyw8 added enhancement New feature or request server This pull request should be included in the server gem's release notes labels Jun 12, 2024
@vinistock
Copy link
Member

Sorry to jump in late, but if this is only related to the test framework detection then I believe we should first explore #1334.

Now that we can linearize ancestors for a class, we should use that information to determine which test framework is used for a given test.

Essentially something like this

ancestors = @index.linearized_ancestors_of("NameOfTestParentClass")

if ancestors.include?("ActiveSupport::TestCase")
  # rails
elsif ancestors.include?("Minitest::Test")
  # minitest
elsif ancestors.include?("Test::Unit::TestCase")
  # test-unit
else
  # rspec
end

Notice that linearizing ancestors is an expensive operation and using two test frameworks in the same project is quite uncommon, so we may want to use this as a detection mechanism only when someone opens a test file the first time and then memoize the framework we identified.

@Earlopain
Copy link
Contributor Author

Right, if the testing framework detection is going to use some other logic for this, how about I just switch this over to SetupBundler to really fix the issue instead?

My main reason for not doing that in the first place was that I wasn't sure how to share the implementation between these two but if that isn't a concern then I can just move it over.

@Earlopain
Copy link
Contributor Author

Continuing in #2218

@Earlopain Earlopain closed this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants