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

WIP: Use index to detect test library #3031

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
37c2363
Pass test library to code lens
andyw8 Jan 9, 2025
658ec55
Update tests (no longer need to stub)
andyw8 Jan 9, 2025
c51615c
Implement
andyw8 Jan 9, 2025
47bb0e1
Remove test_library from global_state
andyw8 Jan 9, 2025
13382b9
Remove global_state from code lens listener
andyw8 Jan 9, 2025
28476bb
Remove unnecessary global_state passing
andyw8 Jan 9, 2025
616ac36
Try fix test
andyw8 Jan 9, 2025
b5d286e
Limit entries_for to classes
andyw8 Jan 9, 2025
d6aabe4
Add note
andyw8 Jan 9, 2025
1279d16
Don't run Code Lens for ERB
andyw8 Jan 9, 2025
9ff365b
Use 'unknown' not 'none'
andyw8 Jan 9, 2025
6892885
Remove commented-out code
andyw8 Jan 10, 2025
99f96c1
Filer out top level
andyw8 Jan 10, 2025
42b0ab0
Fix assert_equal ordering
andyw8 Jan 10, 2025
28b3258
Check path for test
andyw8 Jan 10, 2025
7c77de2
Enable LiteralAsActualArgument cop
andyw8 Jan 10, 2025
d9ba140
Fix arg ordering
andyw8 Jan 10, 2025
0bcb134
WIP (failing) pass document instead of test_library
andyw8 Jan 10, 2025
90a26b9
Merge branch 'main' into andyw8/use-index-to-detect-test-library
andyw8 Jan 10, 2025
62f5e44
Skip unsaved files
andyw8 Jan 12, 2025
26c300c
Update config to index tests
andyw8 Jan 12, 2025
4b9f5c1
WIP
andyw8 Jan 16, 2025
b694ae9
Add note
andyw8 Jan 16, 2025
4d87b79
Pass document
andyw8 Jan 16, 2025
4846245
Update test
andyw8 Jan 16, 2025
e67f369
Fix types
andyw8 Jan 16, 2025
eaa7d71
WIP (pairing with vini)
andyw8 Jan 22, 2025
9c25240
Merge branch 'main' into andyw8/use-index-to-detect-test-library
andyw8 Feb 4, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,6 @@ Layout/ClassStructure:
- include
- prepend
- extend

Minitest/LiteralAsActualArgument:
Enabled: true
6 changes: 3 additions & 3 deletions lib/ruby_indexer/lib/ruby_indexer/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ def initialize

@excluded_patterns = T.let(
[
File.join("**", "*_test.rb"),
# File.join("**", "*_test.rb"),
File.join("node_modules", "**", "*"),
File.join("spec", "**", "*"),
File.join("test", "**", "*"),
# File.join("spec", "**", "*"),
# File.join("test", "**", "*"),
File.join("tmp", "**", "*"),
],
T::Array[String],
Expand Down
2 changes: 1 addition & 1 deletion lib/ruby_indexer/test/method_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def bar; end
entries = T.must(@index[keyword])
# should receive two entries because module_function creates a singleton method
# for the Test module and a private method for classes include the Test module
assert_equal(entries.size, 2)
assert_equal(2, entries.size)
first_entry, second_entry = *entries
# The first entry points to the location of the module_function call
assert_equal("Test", first_entry.owner.name)
Expand Down
4 changes: 2 additions & 2 deletions lib/ruby_lsp/addon.rb
Original file line number Diff line number Diff line change
Expand Up @@ -214,11 +214,11 @@ def handle_window_show_message_response(title); end
sig do
overridable.params(
response_builder: ResponseBuilders::CollectionResponseBuilder[Interface::CodeLens],
uri: URI::Generic,
document: RubyLsp::Document[T.untyped],
dispatcher: Prism::Dispatcher,
).void
end
def create_code_lens_listener(response_builder, uri, dispatcher); end
def create_code_lens_listener(response_builder, document, dispatcher); end

# Creates a new Hover listener. This method is invoked on every Hover request
sig do
Expand Down
10 changes: 10 additions & 0 deletions lib/ruby_lsp/document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,16 @@ def find_index_by_position(start_pos, end_pos = nil)
end
end

sig { returns(T::Boolean) }
def test_file?
false
end

sig { returns(String) }
def test_library
""
end

private

sig { returns(Scanner) }
Expand Down
43 changes: 16 additions & 27 deletions lib/ruby_lsp/global_state.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@ module RubyLsp
class GlobalState
extend T::Sig

sig { returns(String) }
attr_reader :test_library

sig { returns(String) }
attr_accessor :formatter

Expand Down Expand Up @@ -39,7 +36,6 @@ def initialize

@formatter = T.let("auto", String)
@linters = T.let([], T::Array[String])
@test_library = T.let("minitest", String)
@has_type_checker = T.let(true, T::Boolean)
@index = T.let(RubyIndexer::Index.new, RubyIndexer::Index)
@supported_formatters = T.let({}, T::Hash[String, Requests::Support::Formatter])
Expand Down Expand Up @@ -142,9 +138,6 @@ def apply_options(options)
Notification.window_log_message("Auto detected linters: #{@linters.join(", ")}")
end

@test_library = detect_test_library(direct_dependencies)
notifications << Notification.window_log_message("Detected test library: #{@test_library}")

@has_type_checker = detect_typechecker(all_dependencies)
if @has_type_checker
notifications << Notification.window_log_message(
Expand Down Expand Up @@ -205,6 +198,22 @@ def supports_watching_files
@client_capabilities.supports_watching_files
end

sig do
params(
node: T.any(
Prism::ConstantPathNode,
Prism::ConstantReadNode,
Prism::ConstantPathTargetNode,
),
).returns(T.nilable(String))
end
def constant_name(node)
node.full_name
rescue Prism::ConstantPathNode::DynamicPartsInConstantPathError,
Prism::ConstantPathNode::MissingNodesInConstantPathError
nil
end

private

sig { params(direct_dependencies: T::Array[String], all_dependencies: T::Array[String]).returns(String) }
Expand Down Expand Up @@ -234,26 +243,6 @@ def detect_linters(dependencies, all_dependencies)
linters
end

sig { params(dependencies: T::Array[String]).returns(String) }
def detect_test_library(dependencies)
if dependencies.any?(/^rspec/)
"rspec"
# A Rails app may have a dependency on minitest, but we would instead want to use the Rails test runner provided
# by ruby-lsp-rails. A Rails app doesn't need to depend on the rails gem itself, individual components like
# activestorage may be added to the gemfile so that other components aren't downloaded. Check for the presence
# of bin/rails to support these cases.
elsif bin_rails_present
"rails"
# NOTE: Intentionally ends with $ to avoid mis-matching minitest-reporters, etc. in a Rails app.
elsif dependencies.any?(/^minitest$/)
"minitest"
elsif dependencies.any?(/^test-unit/)
"test-unit"
else
"unknown"
end
end

sig { params(dependencies: T::Array[String]).returns(T::Boolean) }
def detect_typechecker(dependencies)
return false if ENV["RUBY_LSP_BYPASS_TYPECHECKER"]
Expand Down
86 changes: 77 additions & 9 deletions lib/ruby_lsp/listeners/code_lens.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,18 @@ class CodeLens

sig do
params(
response_builder: ResponseBuilders::CollectionResponseBuilder[Interface::CodeLens],
global_state: GlobalState,
uri: URI::Generic,
response_builder: ResponseBuilders::CollectionResponseBuilder[Interface::CodeLens],
document: RubyDocument,
dispatcher: Prism::Dispatcher,
).void
end
def initialize(response_builder, global_state, uri, dispatcher)
@response_builder = response_builder
def initialize(global_state, response_builder, document, dispatcher)
@global_state = global_state
@uri = T.let(uri, URI::Generic)
@path = T.let(uri.to_standardized_path, T.nilable(String))
@response_builder = response_builder
@uri = T.let(document.uri, URI::Generic)
# @test_library = T.let(document.test_library, String)
@path = T.let(@uri.to_standardized_path, T.nilable(String))
# visibility_stack is a stack of [current_visibility, previous_visibility]
@visibility_stack = T.let([[:public, :public]], T::Array[T::Array[T.nilable(Symbol)]])
@group_stack = T.let([], T::Array[String])
Expand Down Expand Up @@ -65,7 +66,20 @@ def on_class_node_enter(node)
class_name = node.constant_path.slice
@group_stack.push(class_name)

if @path && class_name.end_with?("Test")
return unless @path # if file not yet saved

constant_path_node = node.constant_path
return if constant_path_node.is_a?(Prism::CallNode)

name = constant_name(constant_path_node)

return unless name

fully_qualified_name = actual_nesting(name).join("::")

ancestors = @global_state.index.linearized_ancestors_of(fully_qualified_name)

if ancestors.include?("Minitest::Test")
add_test_code_lens(
node,
name: class_name,
Expand All @@ -79,6 +93,22 @@ def on_class_node_enter(node)
end
end

# TODO: move to index

sig { params(name: String).returns(T::Array[String]) }
def actual_nesting(name)
nesting = @stack + [name]
corrected_nesting = []

nesting.reverse_each do |name|
corrected_nesting.prepend(name.delete_prefix("::"))

break if name.start_with?("::")
end

corrected_nesting
end

sig { params(node: Prism::ClassNode).void }
def on_class_node_leave(node)
@visibility_stack.pop
Expand Down Expand Up @@ -178,7 +208,7 @@ def on_call_node_leave(node)
sig { params(node: Prism::Node, name: String, command: String, kind: Symbol, id: String).void }
def add_test_code_lens(node, name:, command:, kind:, id: name)
# don't add code lenses if the test library is not supported or unknown
return unless SUPPORTED_TEST_LIBRARIES.include?(@global_state.test_library) && @path
return unless SUPPORTED_TEST_LIBRARIES.include?(@test_library) && @path

arguments = [
@path,
Expand Down Expand Up @@ -235,7 +265,7 @@ def generate_test_command(group_stack: [], spec_name: nil, method_name: nil)
command += " -Ispec" if File.fnmatch?("**/spec/**/*", path, File::FNM_PATHNAME)
command += " #{path}"

case @global_state.test_library
case @test_library
when "minitest"
last_dynamic_reference_index = group_stack.rindex(DYNAMIC_REFERENCE_MARKER)
command += if last_dynamic_reference_index
Expand Down Expand Up @@ -324,6 +354,44 @@ def generate_fully_qualified_id(group_stack:, method_name: nil)
group_stack.join("::")
end
end

# TODO: consider moving this to common.rb

# Returns the detected test library for a group of tests
# sig { params(node: T.any(Prism::CallNode, Prism::ClassNode)).returns(String) }
# def test_library_for_group(node)
# if node.is_a?(Prism::CallNode)
# receiver = node.receiver
# if node.name == :describe && (!receiver.is_a?(Prism::ConstantReadNode) || receiver.name == :RSpec)
# return "rspec"
# end

# return "unknown"
# end

# constant_path_node = node.constant_path
# return "unknown" if constant_path_node.is_a?(Prism::CallNode)

# class_name = constant_name(constant_path_node)
# # binding.break
# # TODO: handle errors, e.g. if something not indexed
# return "unknown" unless class_name

# ancestors = @index.linearized_ancestors_of(class_name)

# # ancestors = class_entries
# # .map { @global_state.index.linearized_ancestors_of(_1.name) }.flatten

# if ancestors.include?("ActiveSupport::TestCase") # or ActiveSupport::Testing::Declarative ?
# "rails"
# elsif ancestors.include?("Minitest::Test")
# "minitest"
# elsif ancestors.include?("Test::Unit::TestCase")
# "test-unit"
# else
# "unknown"
# end
# end
end
end
end
7 changes: 4 additions & 3 deletions lib/ruby_lsp/requests/code_lens.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,20 @@ def provider
params(
global_state: GlobalState,
uri: URI::Generic,
document: RubyLsp::RubyDocument,
dispatcher: Prism::Dispatcher,
).void
end
def initialize(global_state, uri, dispatcher)
def initialize(global_state, uri, document, dispatcher)
@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(@global_state, @response_builder, document, dispatcher)

Addon.addons.each do |addon|
addon.create_code_lens_listener(@response_builder, uri, dispatcher)
addon.create_code_lens_listener(@response_builder, document, dispatcher)
end
end

Expand Down
35 changes: 35 additions & 0 deletions lib/ruby_lsp/ruby_document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,41 @@ def locate_node(position, node_types: [])
)
end

sig { returns(String) }
def test_library
Copy link
Member

Choose a reason for hiding this comment

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

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?

return "none" unless test_file?

class_entries = @global_state.index.entries_for(@uri.to_s, RubyIndexer::Entry::Class)
unless class_entries
return "none"
end

# TODO: consider performance hit
# A better approach might be check the classes entries one at a time.
ancestors = class_entries
.map { @global_state.index.linearized_ancestors_of(_1.name) }.flatten

# ActiveSupport::TestCase is a subclass of Minitest::Test so we must check for it first
if ancestors.include?("ActiveSupport::TestCase")
"rails"
elsif ancestors.include?("Minitest::Test")
"minitest"
elsif ancestors.include?("Test::Unit::TestCase")
"test-unit"
else
"unknown"
Comment on lines +275 to +282
Copy link
Member

Choose a reason for hiding this comment

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

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.

end
end

sig { returns(T::Boolean) }
def test_file?
# if the file is not saved, then we don't know its path, so can't know it's a test
return false unless @uri.path # file may not be saved

path = T.must(@uri.path)
path.include?("/test/") || path.include?("/spec/")
end

sig { returns(T::Boolean) }
def should_index?
# This method controls when we should index documents. If there's no recent edit and the document has just been
Expand Down
5 changes: 3 additions & 2 deletions lib/ruby_lsp/server.rb
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,8 @@ def run_combined_requests(message)
folding_range = Requests::FoldingRanges.new(parse_result.comments, dispatcher)
document_symbol = Requests::DocumentSymbol.new(uri, dispatcher)
document_link = Requests::DocumentLink.new(uri, parse_result.comments, dispatcher)
code_lens = Requests::CodeLens.new(@global_state, uri, dispatcher)
# TODO: seperate PR for global state
code_lens = Requests::CodeLens.new(global_state, uri, document, dispatcher) if document.is_a?(RubyDocument)
inlay_hint = Requests::InlayHints.new(document, T.must(@store.features_configuration.dig(:inlayHint)), dispatcher)

if document.is_a?(RubyDocument) && document.should_index?
Expand All @@ -494,7 +495,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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if is temporary

document.cache_set("textDocument/inlayHint", inlay_hint.perform)

send_message(Result.new(id: message[:id], response: document.cache_get(message[:method])))
Expand Down
Loading
Loading