Migrate test discovery to use Rubydex

Author: vinistock

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

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
 

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

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

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 },
         })