WIP: Use index to detect test library #3031
Conversation
lib/ruby_lsp/requests/code_lens.rb
Outdated
| @response_builder = T.let( | ||
| ResponseBuilders::CollectionResponseBuilder[Interface::CodeLens].new, | ||
| ResponseBuilders::CollectionResponseBuilder[Interface::CodeLens], | ||
| ) | ||
| super() | ||
| Listeners::CodeLens.new(@response_builder, global_state, uri, dispatcher) | ||
| Listeners::CodeLens.new(@response_builder, test_library, uri, dispatcher) | ||
|
|
||
| Addon.addons.each do |addon| | ||
| addon.create_code_lens_listener(@response_builder, uri, dispatcher) |
There was a problem hiding this comment.
@st0012 @vinistock I'm wondering how we should update this since currently ruby-lsp-rails checks @global_state.test_library == "rails". What if we make test_library be an optional keyword argument? That would prevent breaking the API for other addons.
There was a problem hiding this comment.
Alternatively, we change it to pass document instead of uri (since uri is available via document).
There was a problem hiding this comment.
This should be considered a breaking change anyway so I think changing args is expected?
There was a problem hiding this comment.
True. I've updated it to pass document insead of test_library, which should allow for more flexibility in future. I'm still considering if uri could be dropped.
(also have to investigate some test failures).
| @@ -5,21 +5,19 @@ | |||
| require_relative "support/expectations_test_runner" | |||
|
|
|||
| class CodeLensExpectationsTest < ExpectationsTestRunner | |||
| expectations_tests RubyLsp::Requests::CodeLens, "code_lens" | |||
| # expectations_tests RubyLsp::Requests::CodeLens, "code_lens" | |||
| @@ -491,7 +491,7 @@ def run_combined_requests(message) | |||
| document.cache_set("textDocument/foldingRange", folding_range.perform) | |||
| document.cache_set("textDocument/documentSymbol", document_symbol.perform) | |||
| document.cache_set("textDocument/documentLink", document_link.perform) | |||
| document.cache_set("textDocument/codeLens", code_lens.perform) | |||
| document.cache_set("textDocument/codeLens", code_lens.perform) if code_lens | |||
| @@ -257,6 +257,37 @@ def locate_node(position, node_types: []) | |||
| ) | |||
| end | |||
|
|
|||
| sig { returns(String) } | |||
| def test_library | |||
There was a problem hiding this comment.
Part of the advantage of looking at the ancestors is that we become more accurate even in very uncommon setups, like mixing test frameworks - which is something that came up a few times.
Considering that test files can have multiple test classes and they can inherit from different parents, is this approach flexible enough to account for that?
What was the idea behind putting it in document versus doing it in the code lens implementation?
| if ancestors.include?("ActiveSupport::TestCase") | ||
| "rails" | ||
| elsif ancestors.include?("Minitest::Test") | ||
| "minitest" | ||
| elsif ancestors.include?("Test::Unit::TestCase") | ||
| "test-unit" | ||
| else | ||
| "unknown" |
There was a problem hiding this comment.
We should become a bit more specific now that we're checking for ancestors. For example, as Alex mentioned here, the test framework isn't really "rails" and is not tied to ActiveSupport::TestCase.
We can check for ActiveSupport::Testing::Declarative and cover a broader range of cases.
Same thing for Minitest and Test Unit. We can now do a better job at detecting the spec style syntax.
|
Corresponding ruby-lsp-rails branch: https://github.com/Shopify/ruby-lsp-rails/compare/andyw8/prepare-for-code-lens-api-change?expand=1 |
Closes #1334
Co-authored with @st0012
TODO:
unknownvsnone?