Moving back to generic error

Author: thavaahariharangit

updater/lib/dependabot/dependency_change_builder.rb

@@ -26,71 +26,6 @@ class DependencyChangeBuilder
     extend T::Sig
 
     SUPPORT_FILE_WARNING_NAME_LIMIT = 10
-    SUPPORT_FILES_ONLY_ERROR_MESSAGE = "FileUpdater returned only support files"
-
-    class SupportFilesOnly < Dependabot::DependabotError
-      extend T::Sig
-
-      sig { returns(String) }
-      attr_reader :dependency_info
-
-      sig { returns(T::Array[String]) }
-      attr_reader :support_file_names
-
-      sig { returns(Integer) }
-      attr_reader :omitted_support_file_count
-
-      sig do
-        params(
-          dependency_info: String,
-          support_file_names: T::Array[String],
-          omitted_support_file_count: Integer
-        ).void
-      end
-      def initialize(dependency_info:, support_file_names:, omitted_support_file_count:)
-        @dependency_info = T.let(dependency_info.dup.freeze, String)
-        @support_file_names = T.let(support_file_names.map { |name| name.dup.freeze }.freeze, T::Array[String])
-        @omitted_support_file_count = T.let(omitted_support_file_count, Integer)
-
-        super(
-          message_for(
-            dependency_info: @dependency_info,
-            support_file_names: @support_file_names,
-            omitted_support_file_count: @omitted_support_file_count
-          )
-        )
-      end
-
-      private
-
-      sig do
-        params(
-          dependency_info: String,
-          support_file_names: T::Array[String],
-          omitted_support_file_count: Integer
-        ).returns(String)
-      end
-      def message_for(dependency_info:, support_file_names:, omitted_support_file_count:)
-        support_files = formatted_support_files(
-          support_file_names: support_file_names,
-          omitted_support_file_count: omitted_support_file_count
-        )
-
-        "#{SUPPORT_FILES_ONLY_ERROR_MESSAGE} for: #{dependency_info}; excluded support files: #{support_files}"
-      end
-
-      sig do
-        params(
-          support_file_names: T::Array[String],
-          omitted_support_file_count: Integer
-        ).returns(String)
-      end
-      def formatted_support_files(support_file_names:, omitted_support_file_count:)
-        support_files = support_file_names.join(", ")
-        support_files += " (and #{omitted_support_file_count} more)" if omitted_support_file_count.positive?
-        support_files
-      end
-    end
 
     sig do
       params(
@@ -134,14 +69,17 @@ def initialize(job:, dependency_files:, updated_dependencies:, change_source:, n
       @updated_dependencies = updated_dependencies
       @change_source = change_source
       @notices = notices
+      @support_files_only_diagnostics = T.let(nil, T.nilable(String))
     end
 
     sig { returns(Dependabot::DependencyChange) }
     def run
       updated_files = generate_dependency_files
 
       unless updated_files.any?
-        raise DependabotError, "FileUpdater failed to update any files for: #{dependency_info_for_error}"
+        error_message = "FileUpdater failed to update any files for: #{dependency_info_for_error}"
+        error_message += "; #{support_files_only_diagnostics}" if support_files_only_diagnostics
+        raise DependabotError, error_message
       end
 
       # Remove any unchanged dependencies from the updated list
@@ -182,6 +120,9 @@ def run
     sig { returns(T::Array[Dependabot::Notice]) }
     attr_reader :notices
 
+    sig { returns(T.nilable(String)) }
+    attr_reader :support_files_only_diagnostics
+
     sig { returns(T.nilable(String)) }
     def source_dependency_name
       return nil unless change_source.is_a? Dependabot::Dependency
@@ -206,7 +147,8 @@ def generate_dependency_files
           "#{updated_dependency.version}"
         )
       else
-        Dependabot.logger.info("Updating #{dependency_names}")
+        dependency_names = updated_dependencies.map(&:name)
+        Dependabot.logger.info("Updating #{dependency_names.join(', ')}")
       end
 
       # Ignore dependencies that are tagged as information_only. These will be
@@ -225,7 +167,7 @@ def generate_dependency_files
       @notices.concat(updater_notices)
 
       updated_files = all_files.reject(&:support_file?)
-      handle_support_files_only!(all_files: all_files, updated_files: updated_files)
+      add_support_files_only_diagnostics(all_files: all_files, updated_files: updated_files)
 
       updated_files
     end
@@ -236,20 +178,18 @@ def generate_dependency_files
         updated_files: T::Array[Dependabot::DependencyFile]
       ).void
     end
-    def handle_support_files_only!(all_files:, updated_files:)
+    def add_support_files_only_diagnostics(all_files:, updated_files:)
       return unless all_files.any? && updated_files.empty?
 
       support_file_names = naturally_sorted_names(all_files.select(&:support_file?).map(&:name).uniq)
       listed_support_file_names = support_file_names.first(SUPPORT_FILE_WARNING_NAME_LIMIT)
       omitted_support_file_count = support_file_names.length - listed_support_file_names.length
+      support_file_list = listed_support_file_names.join(", ")
+      support_file_list += " (and #{omitted_support_file_count} more)" if omitted_support_file_count.positive?
 
-      error = SupportFilesOnly.new(
-        dependency_info: dependency_info_for_error,
-        support_file_names: listed_support_file_names,
-        omitted_support_file_count: omitted_support_file_count
-      )
-      Dependabot.logger.warn(error.message)
-      raise error
+      diagnostics = "FileUpdater returned only support files: #{support_file_list}"
+      @support_files_only_diagnostics = diagnostics
+      Dependabot.logger.warn("#{diagnostics} for: #{dependency_info_for_error}")
     end
 
     sig { params(names: T::Array[String]).returns(T::Array[String]) }
@@ -266,7 +206,7 @@ def natural_sort_segments(name)
     end
 
     sig { returns(String) }
-    def dependency_names
+    def dependency_names_for_error
       format_names(updated_dependencies.map(&:name))
     end
 
@@ -277,7 +217,7 @@ def format_names(names)
 
     sig { returns(String) }
     def dependency_info_for_error
-      return dependency_names unless updated_dependencies.one?
+      return dependency_names_for_error unless updated_dependencies.one?
 
       dependency = T.must(updated_dependencies.first)
       previous_version = dependency.previous_version || "unknown"

updater/spec/dependabot/dependency_change_builder_spec.rb

@@ -73,7 +73,6 @@
       )
     end
 
-    let(:support_files_only_error_message) { described_class::SUPPORT_FILES_ONLY_ERROR_MESSAGE }
     let(:notices) { [] }
     let(:lead_dependency_change_source) { build_dependency(name: "dummy-pkg-b", version: "1.1.0") }
     let(:single_dependency_info) { "dummy-pkg-b (1.1.0 → 1.2.0)" }
@@ -137,21 +136,6 @@ def build_support_files(names)
       end
     end
 
-    def expect_support_files_only_error(
-      dependency_info:, support_file_names:, omitted_support_file_count: 0, expected_message: nil
-    )
-      expect { create_change }.to raise_error(described_class::SupportFilesOnly) { |error|
-        expect(error.dependency_info).to eq(dependency_info)
-        expect(error.support_file_names).to eq(support_file_names)
-        expect(error.omitted_support_file_count).to eq(omitted_support_file_count)
-        if expected_message
-          expect(error.message).to eq(expected_message)
-        else
-          expect(error.message).to include(support_files_only_error_message)
-        end
-      }
-    end
-
     context "when the source is a lead dependency" do
       let(:change_source) { lead_dependency_change_source }
 
@@ -272,64 +256,31 @@ def expect_support_files_only_error(
         expect(Dependabot.logger)
           .to receive(:warn)
           .with(satisfy { |message|
-            message.include?(support_files_only_error_message) &&
+            message.include?("FileUpdater returned only support files") &&
               message.include?("for: #{single_dependency_info}") &&
-              message.include?("excluded support files:") &&
               message.include?("sub_dep") &&
               message.include?("sub_dep.lock") &&
               !message.include?("(and")
           })
 
-        expect_support_files_only_error(
-          dependency_info: single_dependency_info,
-          support_file_names: %w(sub_dep sub_dep.lock)
-        )
+        expect { create_change }
+          .to raise_error(
+            Dependabot::DependabotError,
+            "FileUpdater failed to update any files for: dummy-pkg-b (1.1.0 → 1.2.0); " \
+            "FileUpdater returned only support files: sub_dep, sub_dep.lock"
+          )
       end
 
       it "collects notices before raising" do
-        expect_support_files_only_error(
-          dependency_info: single_dependency_info,
-          support_file_names: %w(sub_dep sub_dep.lock)
-        )
+        expect { create_change }
+          .to raise_error(
+            Dependabot::DependabotError,
+            "FileUpdater failed to update any files for: dummy-pkg-b (1.1.0 → 1.2.0); " \
+            "FileUpdater returned only support files: sub_dep, sub_dep.lock"
+          )
 
         expect(notices).to eq(updater_notices)
       end
-
-      it "exposes immutable support file names on the raised error" do
-        expect { create_change }.to raise_error(described_class::SupportFilesOnly) { |error|
-          expect(error.support_file_names).to be_frozen
-          expect(error.support_file_names).to all(be_frozen)
-          expect { error.support_file_names << "new_support_file" }.to raise_error(FrozenError)
-        }
-      end
-
-      it "exposes immutable dependency info on the raised error" do
-        expect { create_change }.to raise_error(described_class::SupportFilesOnly) { |error|
-          expect(error.dependency_info).to be_frozen
-        }
-      end
-
-      it "defensively copies support file names and dependency info" do
-        dependency_info = +"dummy-pkg-b (1.1.0 → 1.2.0)"
-        support_file_names = ["sub_dep", "sub_dep.lock"]
-
-        error = described_class::SupportFilesOnly.new(
-          dependency_info: dependency_info,
-          support_file_names: support_file_names,
-          omitted_support_file_count: 0
-        )
-
-        dependency_info << " (mutated)"
-        support_file_names << "new_support_file"
-
-        expect(error.dependency_info).to eq("dummy-pkg-b (1.1.0 → 1.2.0)")
-        expect(error.support_file_names).to eq(%w(sub_dep sub_dep.lock))
-        expect(error.message).to start_with(
-          "#{support_files_only_error_message} for: dummy-pkg-b (1.1.0 → 1.2.0);"
-        )
-        expect(error.message).to include("excluded support files: sub_dep, sub_dep.lock")
-        expect(error.message).not_to include("new_support_file")
-      end
     end
 
     context "when grouped updates return only support files" do
@@ -347,16 +298,13 @@ def expect_support_files_only_error(
         stub_file_updater(updated_dependency_files: support_files)
       end
 
-      it "raises an exception with sorted and unique dependency names" do
-        expected_message =
-          "FileUpdater returned only support files for: dummy-pkg-a, dummy-pkg-b; " \
-          "excluded support files: sub_dep, sub_dep.lock"
-
-        expect_support_files_only_error(
-          dependency_info: "dummy-pkg-a, dummy-pkg-b",
-          support_file_names: %w(sub_dep sub_dep.lock),
-          expected_message: expected_message
-        )
+      it "raises a diagnostics error with sorted and unique dependency names" do
+        expect { create_change }
+          .to raise_error(
+            Dependabot::DependabotError,
+            "FileUpdater failed to update any files for: dummy-pkg-a, dummy-pkg-b; " \
+            "FileUpdater returned only support files: sub_dep, sub_dep.lock"
+          )
       end
     end
 
@@ -373,22 +321,17 @@ def expect_support_files_only_error(
         stub_file_updater(updated_dependency_files: support_files)
       end
 
-      it "warns with the listed limit and omitted count" do
-        expect(Dependabot.logger)
-          .to receive(:warn)
-          .with(satisfy { |message|
-            message.include?(support_files_only_error_message) &&
-              message.include?("excluded support files:") &&
-              message.include?("(and 1 more)")
-          })
+      it "adds omitted count to diagnostics" do
+        expected_support_files = Array.new(described_class::SUPPORT_FILE_WARNING_NAME_LIMIT) do |index|
+          "support_#{index}.txt"
+        end.join(", ")
 
-        expect_support_files_only_error(
-          dependency_info: single_dependency_info,
-          support_file_names: Array.new(described_class::SUPPORT_FILE_WARNING_NAME_LIMIT) do |index|
-            "support_#{index}.txt"
-          end,
-          omitted_support_file_count: 1
-        )
+        expect { create_change }
+          .to raise_error(
+            Dependabot::DependabotError,
+            "FileUpdater failed to update any files for: dummy-pkg-b (1.1.0 → 1.2.0); " \
+            "FileUpdater returned only support files: #{expected_support_files} (and 1 more)"
+          )
       end
     end
 
@@ -402,11 +345,13 @@ def expect_support_files_only_error(
         stub_file_updater(updated_dependency_files: support_files)
       end
 
-      it "orders support file names naturally in the raised error" do
-        expect_support_files_only_error(
-          dependency_info: single_dependency_info,
-          support_file_names: %w(support_1.txt support_2.txt support_10.txt)
-        )
+      it "orders support file names naturally in diagnostics" do
+        expect { create_change }
+          .to raise_error(
+            Dependabot::DependabotError,
+            "FileUpdater failed to update any files for: dummy-pkg-b (1.1.0 → 1.2.0); " \
+            "FileUpdater returned only support files: support_1.txt, support_2.txt, support_10.txt"
+          )
       end
     end
 
@@ -421,10 +366,12 @@ def expect_support_files_only_error(
       end
 
       it "orders support file names naturally regardless of case" do
-        expect_support_files_only_error(
-          dependency_info: single_dependency_info,
-          support_file_names: ["support_1.txt", "Support_2.txt", "support_10.txt"]
-        )
+        expect { create_change }
+          .to raise_error(
+            Dependabot::DependabotError,
+            "FileUpdater failed to update any files for: dummy-pkg-b (1.1.0 → 1.2.0); " \
+            "FileUpdater returned only support files: support_1.txt, Support_2.txt, support_10.txt"
+          )
       end
     end
   end