From 541b20fad88d39ed0bd6cf6651dcf54e7f974e7a Mon Sep 17 00:00:00 2001 From: Vinicius Stock Date: Wed, 12 Feb 2025 16:13:48 -0500 Subject: [PATCH] Ensure we index non test files under test directories --- .../lib/ruby_indexer/configuration.rb | 105 +++++++----------- lib/ruby_indexer/test/configuration_test.rb | 34 +++++- 2 files changed, 71 insertions(+), 68 deletions(-) diff --git a/lib/ruby_indexer/lib/ruby_indexer/configuration.rb b/lib/ruby_indexer/lib/ruby_indexer/configuration.rb index 434c58b76..4927d0bb3 100644 --- a/lib/ruby_indexer/lib/ruby_indexer/configuration.rb +++ b/lib/ruby_indexer/lib/ruby_indexer/configuration.rb @@ -31,11 +31,8 @@ def initialize @excluded_patterns = T.let( [ - File.join("**", "*_test.rb"), - File.join("node_modules", "**", "*"), - File.join("spec", "**", "*"), - File.join("test", "**", "*"), - File.join("tmp", "**", "*"), + "**/{test,spec,features}/**/{*_test.rb,test_*.rb,*_spec.rb,*.feature}", + "**/fixtures/**/*", ], T::Array[String], ) @@ -44,10 +41,21 @@ def initialize if path # Substitute Windows backslashes into forward slashes, which are used in glob patterns glob = path.gsub(/[\\]+/, "/") - @excluded_patterns << File.join(glob, "**", "*.rb") + glob.delete_suffix!("/") + @excluded_patterns << "#{glob}/**/*.rb" end - @included_patterns = T.let([File.join("**", "*.rb")], T::Array[String]) + excluded_directories = ["tmp", "node_modules", "sorbet"] + top_level_directories = Dir.glob("#{Dir.pwd}/*").filter_map do |path| + dir_name = File.basename(path) + next unless File.directory?(path) && !excluded_directories.include?(dir_name) + + dir_name + end + + # We start the included patterns with only the non excluded directories so that we can avoid paying the price of + # traversing large directories that don't include Ruby files like `node_modules` + @included_patterns = T.let(["{#{top_level_directories.join(",")}}/**/*.rb", "*.rb"], T::Array[String]) @excluded_magic_comments = T.let( [ "frozen_string_literal:", @@ -66,21 +74,6 @@ def initialize ) end - sig { returns(String) } - def merged_excluded_file_pattern - # This regex looks for @excluded_patterns that follow the format of "something/**/*", where - # "something" is one or more non-"/" - # - # Returns "/path/to/workspace/{tmp,node_modules}/**/*" - @excluded_patterns - .filter_map do |pattern| - next if File.absolute_path?(pattern) - - pattern.match(%r{\A([^/]+)/\*\*/\*\z})&.captures&.first - end - .then { |dirs| File.join(@workspace_path, "{#{dirs.join(",")}}/**/*") } - end - sig { returns(T::Array[URI::Generic]) } def indexable_uris excluded_gems = @excluded_gems - @included_gems @@ -91,51 +84,19 @@ def indexable_uris flags = File::FNM_PATHNAME | File::FNM_EXTGLOB - # In order to speed up indexing, only traverse into top-level directories that are not entirely excluded. - # For example, if "tmp/**/*" is excluded, we don't need to traverse into "tmp" at all. However, if - # "vendor/bundle/**/*" is excluded, we will traverse all of "vendor" and `reject!` out all "vendor/bundle" entries - # later. - excluded_pattern = merged_excluded_file_pattern - included_paths = Dir.glob(File.join(@workspace_path, "*/"), flags) - .filter_map do |included_path| - next if File.fnmatch?(excluded_pattern, included_path, flags) - - relative_path = included_path - .delete_prefix(@workspace_path) - .tap { |path| path.delete_prefix!("/") } - - [included_path, relative_path] - end - - uris = T.let([], T::Array[URI::Generic]) - - # Handle top level files separately. The path below is an optimization to prevent descending down directories that - # are going to be excluded anyway, so we need to handle top level scripts separately - Dir.glob(File.join(@workspace_path, "*.rb"), flags).each do |path| - uris << URI::Generic.from_path(path: path) - end - - # Add user specified patterns - @included_patterns.each do |pattern| + uris = @included_patterns.flat_map do |pattern| load_path_entry = T.let(nil, T.nilable(String)) - included_paths.each do |included_path, relative_path| - relative_pattern = pattern.delete_prefix(File.join(relative_path, "/")) - - next unless pattern.start_with?("**") || pattern.start_with?(relative_path) - - Dir.glob(File.join(included_path, relative_pattern), flags).each do |path| - path = File.expand_path(path) - # All entries for the same pattern match the same $LOAD_PATH entry. Since searching the $LOAD_PATH for every - # entry is expensive, we memoize it until we find a path that doesn't belong to that $LOAD_PATH. This - # happens on repositories that define multiple gems, like Rails. All frameworks are defined inside the - # current workspace directory, but each one of them belongs to a different $LOAD_PATH entry - if load_path_entry.nil? || !path.start_with?(load_path_entry) - load_path_entry = $LOAD_PATH.find { |load_path| path.start_with?(load_path) } - end - - uris << URI::Generic.from_path(path: path, load_path_entry: load_path_entry) + Dir.glob(File.join(@workspace_path, pattern), flags).map! do |path| + # All entries for the same pattern match the same $LOAD_PATH entry. Since searching the $LOAD_PATH for every + # entry is expensive, we memoize it until we find a path that doesn't belong to that $LOAD_PATH. This happens + # on repositories that define multiple gems, like Rails. All frameworks are defined inside the current + # workspace directory, but each one of them belongs to a different $LOAD_PATH entry + if load_path_entry.nil? || !path.start_with?(load_path_entry) + load_path_entry = $LOAD_PATH.find { |load_path| path.start_with?(load_path) } end + + URI::Generic.from_path(path: path, load_path_entry: load_path_entry) end end @@ -149,11 +110,23 @@ def indexable_uris end end + # These file names match our exclude regex, but often define important test parent classes that we need for + # ancestor linearization to provide test explorer functionality + excluded_from_ignore_test_files = ["test_case.rb", "test_helper.rb"] + # Remove user specified patterns + bundle_path = Bundler.settings["path"]&.gsub(/[\\]+/, "/") uris.reject! do |indexable| - excluded_patterns.any? do |pattern| - File.fnmatch?(pattern, T.must(indexable.full_path), File::FNM_PATHNAME | File::FNM_EXTGLOB) + path = T.must(indexable.full_path) + + # Don't exclude the special files, unless they are under fixtures or inside a gem installed inside of the + # workspace + if excluded_from_ignore_test_files.include?(File.basename(path)) && + !File.fnmatch?("**/fixtures/**/*", path, flags) && (!bundle_path || !path.start_with?(bundle_path)) + next false end + + excluded_patterns.any? { |pattern| File.fnmatch?(pattern, path, flags) } end # Add default gems to the list of files to be indexed diff --git a/lib/ruby_indexer/test/configuration_test.rb b/lib/ruby_indexer/test/configuration_test.rb index a4b648702..ff7d6f93c 100644 --- a/lib/ruby_indexer/test/configuration_test.rb +++ b/lib/ruby_indexer/test/configuration_test.rb @@ -12,7 +12,7 @@ def setup end def test_load_configuration_executes_configure_block - @config.apply_config({ "excluded_patterns" => ["**/fixtures/**/*.rb"] }) + @config.apply_config({ "excluded_patterns" => ["**/fixtures/**/*"] }) uris = @config.indexable_uris bundle_path = Bundler.bundle_path.join("gems") @@ -39,7 +39,11 @@ def test_indexable_uris_only_includes_gem_require_paths next if lazy_spec.name == "ruby-lsp" spec = Gem::Specification.find_by_name(lazy_spec.name) - assert(uris.none? { |uri| uri.full_path.start_with?("#{spec.full_gem_path}/test/") }) + + test_uris = uris.select do |uri| + File.fnmatch?(File.join(spec.full_gem_path, "test/**/*"), uri.full_path, File::Constants::FNM_PATHNAME) + end + assert_empty(test_uris) rescue Gem::MissingSpecError # Transitive dependencies might be missing when running tests on Windows end @@ -235,5 +239,31 @@ def test_does_not_fail_if_there_are_missing_specs_due_to_platform_constraints end end end + + def test_indexables_include_non_test_files_in_test_directories + # In order to linearize test parent classes and accurately detect the framework being used, then intermediate + # parent classes _must_ also be indexed. Otherwise, we have no way of linearizing the rest of the ancestors to + # determine what the test class ultimately inherits from. + # + # Therefore, we need to ensure that test files are excluded, but non test files inside test directories have to be + # indexed + FileUtils.touch("test/test_case.rb") + + uris = @config.indexable_uris + project_paths = uris.filter_map do |uri| + path = uri.full_path + next if path.start_with?(Bundler.bundle_path.to_s) || path.start_with?(RbConfig::CONFIG["rubylibdir"]) + + Pathname.new(path).relative_path_from(Dir.pwd).to_s + end + + begin + assert_includes(project_paths, "test/requests/support/expectations_test_runner.rb") + assert_includes(project_paths, "test/test_helper.rb") + assert_includes(project_paths, "test/test_case.rb") + ensure + FileUtils.rm("test/test_case.rb") + end + end end end