-
Notifications
You must be signed in to change notification settings - Fork 154
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
Conversation
The bundler setup isn't impacted by this
There was a problem hiding this 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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
dispatcher = Prism::Dispatcher.new | ||
listener = Listeners::RailsApp.new(dispatcher) | ||
dispatcher.dispatch(result.value) | ||
|
||
listener.rails_app |
There was a problem hiding this comment.
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 👌🏼
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. |
Right, if the testing framework detection is going to use some other logic for this, how about I just switch this over to 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. |
Continuing in #2218 |
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 thatSetupBundler
has access to this method. Though the location ofconfig/application.rb
depend onworkspace_uri
which does get modified inapply_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.