diff --git a/lib/ruby_lsp/listeners/spec_style.rb b/lib/ruby_lsp/listeners/spec_style.rb index 5f79a418a..aad665809 100644 --- a/lib/ruby_lsp/listeners/spec_style.rb +++ b/lib/ruby_lsp/listeners/spec_style.rb @@ -34,7 +34,7 @@ def initialize(response_builder, global_state, dispatcher, uri) #: (Prism::ClassNode) -> void def on_class_node_enter(node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod with_test_ancestor_tracking(node) do |name, ancestors| - @spec_group_id_stack << (ancestors.include?("Minitest::Spec") ? ClassGroup.new(name) : nil) + @spec_group_id_stack << (spec_group?(ancestors, name) ? ClassGroup.new(name) : nil) end end @@ -81,6 +81,11 @@ def on_call_node_leave(node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMet private + #: (Array[String], String) -> bool + def spec_group?(ancestors, fully_qualified_name) + fully_qualified_name != "Minitest::Spec" && ancestors.include?("Minitest::Spec") + end + #: (Prism::CallNode) -> void def handle_describe(node) # Describes will include the nesting of all classes and all outer describes as part of its ID, unlike classes diff --git a/lib/ruby_lsp/listeners/test_discovery.rb b/lib/ruby_lsp/listeners/test_discovery.rb index 18c4dc8ed..e36d6b62a 100644 --- a/lib/ruby_lsp/listeners/test_discovery.rb +++ b/lib/ruby_lsp/listeners/test_discovery.rb @@ -13,7 +13,7 @@ class TestDiscovery def initialize(response_builder, global_state, uri) @response_builder = response_builder @uri = uri - @index = global_state.index #: RubyIndexer::Index + @graph = global_state.graph #: Rubydex::Graph @visibility_stack = [:public] #: Array[Symbol] @nesting = [] #: Array[String] end @@ -64,22 +64,29 @@ def calc_attached_ancestors(node, fully_qualified_name) superclass = node.superclass begin - ancestors = @index.linearized_ancestors_of(fully_qualified_name) - # If the project has no bundle and a test class inherits from `Minitest::Test`, the linearized ancestors will - # not include the parent class because we never indexed it in the first place. Here we add the superclass - # directly, so that we can support running tests in projects without a bundle - return ancestors if ancestors.length > 1 - - # If all we found is the class itself, then manually include the parent class - if ancestors.first == fully_qualified_name && superclass - return [*ancestors, superclass.slice] + declaration = @graph[fully_qualified_name] + + unless declaration.is_a?(Rubydex::Namespace) + # When there are dynamic parts in the constant path, we will not have indexed the namespace. We can still + # provide test functionality if the class inherits directly from Test::Unit::TestCase or Minitest::Test + return [superclass&.slice].compact + end + + ancestors = declaration.ancestors.map(&:name) + superclass_ref = declaration.definitions + .filter_map { |d| d.superclass if d.is_a?(Rubydex::ClassDefinition) } + .find { |ref| !ref.is_a?(Rubydex::ResolvedConstantReference) || ref.declaration.name != "Object" } + + # If we couldn't resolve the parent class, then artificially inject it into the ancestors + if superclass_ref.is_a?(Rubydex::UnresolvedConstantReference) && superclass + insert_index = ancestors.index(fully_qualified_name) #: as !nil + insert_index += 1 + ancestors.insert(insert_index, superclass.slice) + return ancestors end + # If the parent class is properly resolved or if there isn't one, then just use the ancestors ancestors - rescue RubyIndexer::Index::NonExistingNamespaceError - # When there are dynamic parts in the constant path, we will not have indexed the namespace. We can still - # provide test functionality if the class inherits directly from Test::Unit::TestCase or Minitest::Test - [superclass&.slice].compact end end diff --git a/lib/ruby_lsp/listeners/test_style.rb b/lib/ruby_lsp/listeners/test_style.rb index b29aeb204..7d2881bc4 100644 --- a/lib/ruby_lsp/listeners/test_style.rb +++ b/lib/ruby_lsp/listeners/test_style.rb @@ -174,9 +174,10 @@ def initialize(response_builder, global_state, dispatcher, uri) #: (Prism::ClassNode node) -> void def on_class_node_enter(node) # rubocop:disable RubyLsp/UseRegisterWithHandlerMethod with_test_ancestor_tracking(node) do |name, ancestors| - @framework = :test_unit if ancestors.include?("Test::Unit::TestCase") + is_test_unit = test_unit_group?(ancestors, name) + @framework = :test_unit if is_test_unit - if @framework == :test_unit || non_declarative_minitest?(ancestors, name) + if is_test_unit || non_declarative_minitest?(ancestors, name) test_item = Requests::Support::TestItem.new( name, name, @@ -259,17 +260,28 @@ def last_test_group @parent_stack[index] #: as Requests::Support::TestItem | ResponseBuilders::TestCollection end - #: (Array[String] attached_ancestors, String fully_qualified_name) -> bool + #: (Array[String], String) -> bool + def test_unit_group?(ancestors, fully_qualified_name) + fully_qualified_name != "Test::Unit::TestCase" && ancestors.include?("Test::Unit::TestCase") + end + + #: (Array[String], String) -> bool def non_declarative_minitest?(attached_ancestors, fully_qualified_name) + return false if ["Minitest::Spec", "Minitest::Test", "ActiveSupport::TestCase"].include?(fully_qualified_name) return false unless attached_ancestors.include?("Minitest::Test") # We only support regular Minitest tests. The declarative syntax provided by ActiveSupport is handled by the # Rails add-on - name_parts = fully_qualified_name.split("::") - singleton_name = "#{name_parts.join("::")}::<#{name_parts.last}>" - !@index.linearized_ancestors_of(singleton_name).include?("ActiveSupport::Testing::Declarative") - rescue RubyIndexer::Index::NonExistingNamespaceError - true + + declaration = @graph[fully_qualified_name] + # If we don't find the fully qualified name in the graph, it means there's a dynamic portion in the test class + # definition. In that case, if the ancestors did include `Minitest::Test`, we always assume it's a test + return true unless declaration.is_a?(Rubydex::Namespace) + + singleton = declaration.singleton_class + return !singleton.ancestors.map(&:name).include?("ActiveSupport::Testing::Declarative") if singleton + + !attached_ancestors.include?("ActiveSupport::TestCase") end end end diff --git a/lib/ruby_lsp/requests/discover_tests.rb b/lib/ruby_lsp/requests/discover_tests.rb index 05f4e833d..527e34928 100644 --- a/lib/ruby_lsp/requests/discover_tests.rb +++ b/lib/ruby_lsp/requests/discover_tests.rb @@ -19,55 +19,19 @@ def initialize(global_state, document, dispatcher) @document = document @dispatcher = dispatcher @response_builder = ResponseBuilders::TestCollection.new #: ResponseBuilders::TestCollection - @index = global_state.index #: RubyIndexer::Index end # @override #: -> Array[Support::TestItem] def perform - uri = @document.uri + Listeners::TestStyle.new(@response_builder, @global_state, @dispatcher, @document.uri) + Listeners::SpecStyle.new(@response_builder, @global_state, @dispatcher, @document.uri) - # We normally only index test files once they are opened in the editor to save memory and avoid doing - # unnecessary work. If the file is already opened and we already indexed it, then we can just discover the tests - # straight away. - # - # However, if the user navigates to a specific test file from the explorer with nothing opened in the UI, then - # we will not have indexed the test file yet and trying to linearize the ancestor of the class will fail. In - # this case, we have to instantiate the indexer listener first, so that we insert classes, modules and methods - # in the index first and then discover the tests, all in the same traversal. - if @index.entries_for(uri.to_s) - Listeners::TestStyle.new(@response_builder, @global_state, @dispatcher, @document.uri) - Listeners::SpecStyle.new(@response_builder, @global_state, @dispatcher, @document.uri) - - Addon.addons.each do |addon| - addon.create_discover_tests_listener(@response_builder, @dispatcher, @document.uri) - end - - @dispatcher.visit(@document.ast) - else - @global_state.synchronize do - RubyIndexer::DeclarationListener.new( - @index, - @dispatcher, - @document.parse_result, - uri, - collect_comments: true, - ) - - Listeners::TestStyle.new(@response_builder, @global_state, @dispatcher, @document.uri) - Listeners::SpecStyle.new(@response_builder, @global_state, @dispatcher, @document.uri) - - Addon.addons.each do |addon| - addon.create_discover_tests_listener(@response_builder, @dispatcher, @document.uri) - end - - # Dispatch the events both for indexing the test file and discovering the tests. The order here is - # important because we need the index to be aware of the existing classes/modules/methods before the test - # listeners can do their work - @dispatcher.visit(@document.ast) - end + Addon.addons.each do |addon| + addon.create_discover_tests_listener(@response_builder, @dispatcher, @document.uri) end + @dispatcher.visit(@document.ast) @response_builder.response end end diff --git a/test/requests/discover_tests_test.rb b/test/requests/discover_tests_test.rb index d727dc0d7..1f2a04fab 100644 --- a/test/requests/discover_tests_test.rb +++ b/test/requests/discover_tests_test.rb @@ -359,43 +359,6 @@ def test_something; end end end - def test_files_are_indexed_lazily_if_needed - path = File.join(Dir.pwd, "lib", "foo.rb") - File.write(path, <<~RUBY) - class FooTest < Test::Unit::TestCase - def test_something; end - end - RUBY - - begin - with_server do |server, uri| - server.global_state.index.index_single(uri, <<~RUBY) - module Test - module Unit - class TestCase; end - end - end - RUBY - - server.process_message( - id: 1, - method: "rubyLsp/discoverTests", - params: { textDocument: { uri: URI::Generic.from_path(path: path) } }, - ) - - items = get_response(server) - assert_equal( - ["FooTest"], - items.map { |i| i[:label] }, - ) - assert_equal(["test_something"], items[0][:children].map { |i| i[:label] }) - assert_all_items_tagged_with(items, :test_unit) - end - ensure - FileUtils.rm(path) - end - end - def test_does_not_raise_on_duplicate_test_ids source = <<~RUBY module Foo @@ -898,13 +861,15 @@ def assert_all_items_tagged_with(items, tag) end def with_minitest_test(source, &block) - with_server(source) do |server, uri| - server.global_state.index.index_single(uri, <<~RUBY) - module Minitest - class Test; end - end - RUBY + source_with_minitest = <<~RUBY + #{source} + module Minitest + class Test; end + end + RUBY + + with_server(source_with_minitest) do |server, uri| server.process_message(id: 1, method: "rubyLsp/discoverTests", params: { textDocument: { uri: uri }, }) @@ -916,15 +881,17 @@ class Test; end end def with_test_unit(source, &block) - with_server(source) do |server, uri| - server.global_state.index.index_single(uri, <<~RUBY) - module Test - module Unit - class TestCase; end - end + source_with_test_unit = <<~RUBY + #{source} + + module Test + module Unit + class TestCase; end end - RUBY + end + RUBY + with_server(source_with_test_unit) do |server, uri| server.process_message(id: 1, method: "rubyLsp/discoverTests", params: { textDocument: { uri: uri }, }) @@ -936,24 +903,26 @@ class TestCase; end end def with_active_support_declarative_tests(source, &block) - with_server(source) do |server, uri| - server.global_state.index.index_single(uri, <<~RUBY) - module Minitest - class Test; end - end + source_with_test_case = <<~RUBY + #{source} - module ActiveSupport - module Testing - module Declarative - end - end + module Minitest + class Test; end + end - class TestCase < Minitest::Test - extend Testing::Declarative + module ActiveSupport + module Testing + module Declarative end end - RUBY + class TestCase < Minitest::Test + extend Testing::Declarative + end + end + RUBY + + with_server(source_with_test_case) do |server, uri| server.process_message(id: 1, method: "rubyLsp/discoverTests", params: { textDocument: { uri: uri }, }) @@ -965,14 +934,16 @@ class TestCase < Minitest::Test end def with_minitest_spec_configured(source, &block) - with_server(source) do |server, uri| - server.global_state.index.index_single(uri, <<~RUBY) - module Minitest - class Test; end - class Spec < Test; end - end - RUBY + source_with_spec = <<~RUBY + #{source} + + module Minitest + class Test; end + class Spec < Test; end + end + RUBY + with_server(source_with_spec) do |server, uri| server.process_message(id: 1, method: "rubyLsp/discoverTests", params: { textDocument: { uri: uri }, })