raise exceptions when PR creation fails (#8013)

common/lib/dependabot/pull_request_creator.rb

@@ -34,6 +34,12 @@ class RepoDisabled < StandardError; end
 
     class NoHistoryInCommon < StandardError; end
 
+    class UnmergedPRExists < StandardError; end
+
+    class BaseCommitNotUpToDate < StandardError; end
+
+    class UnexpectedError < StandardError; end
+
     # AnnotationError is raised if a PR was created, but failed annotation
     class AnnotationError < StandardError
       attr_reader :cause, :pull_request

common/lib/dependabot/pull_request_creator/github.rb

@@ -43,8 +43,12 @@ def initialize(source:, branch_name:, base_commit:, credentials:,
       end
 
       def create
-        return if branch_exists?(branch_name) && unmerged_pull_request_exists?
-        return if require_up_to_date_base? && !base_commit_is_up_to_date?
+        if branch_exists?(branch_name) && unmerged_pull_request_exists?
+          raise UnmergedPRExists, "PR ##{unmerged_pull_requests.first.id} already exists"
+        end
+        if require_up_to_date_base? && !base_commit_is_up_to_date?
+          raise BaseCommitNotUpToDate, "HEAD #{head_commit} does not match base #{base_commit}"
+        end
 
         create_annotated_pull_request
       rescue AnnotationError, Octokit::Error => e
@@ -76,7 +80,11 @@ def branch_exists?(name)
       # rubocop:enable Metrics/PerceivedComplexity
 
       def unmerged_pull_request_exists?
-        pull_requests_for_branch.reject(&:merged).any?
+        unmerged_pull_requests.any?
+      end
+
+      def unmerged_pull_requests
+        pull_requests_for_branch.reject(&:merged)
       end
 
       def pull_requests_for_branch
@@ -106,16 +114,20 @@ def pull_requests_for_branch
       end
 
       def base_commit_is_up_to_date?
-        git_metadata_fetcher.head_commit_for_ref(target_branch) == base_commit
+        head_commit == base_commit
+      end
+
+      def head_commit
+        @head_commit ||= git_metadata_fetcher.head_commit_for_ref(target_branch)
       end
 
       def create_annotated_pull_request
         commit = create_commit
         branch = create_or_update_branch(commit)
-        return unless branch
+        raise UnexpectedError, "Branch not created" unless branch
 
         pull_request = create_pull_request
-        return unless pull_request
+        raise UnexpectedError, "PR not created" unless pull_request
 
         begin
           annotate_pull_request(pull_request)
@@ -220,10 +232,7 @@ def create_or_update_branch(commit)
         # A race condition may cause GitHub to fail here, in which case we retry
         retry_count ||= 0
         retry_count += 1
-        if retry_count > 10
-          raise "Repeatedly failed to create or update branch #{branch_name} " \
-                "with commit #{commit.sha}."
-        end
+        raise if retry_count > 10
 
         sleep(rand(1..1.99))
         retry
@@ -359,9 +368,7 @@ def create_pull_request
           pr_description,
           headers: custom_headers || {}
         )
-      rescue Octokit::UnprocessableEntity => e
-        return handle_pr_creation_error(e) if e.message.include? "Error summary"
-
+      rescue Octokit::UnprocessableEntity
         # Sometimes PR creation fails with no details (presumably because the
         # details are internal). It doesn't hurt to retry in these cases, in
         # case the cause is a race.
@@ -372,18 +379,6 @@ def create_pull_request
         retry
       end
 
-      def handle_pr_creation_error(error)
-        # Ignore races that we lose
-        return if error.message.include?("pull request already exists")
-
-        # Ignore cases where the target branch has been deleted
-        return if error.message.include?("field: base") &&
-                  source.branch &&
-                  !branch_exists?(source.branch)
-
-        raise
-      end
-
       def target_branch
         source.branch || default_branch
       end

common/spec/dependabot/pull_request_creator/github_spec.rb

@@ -509,8 +509,9 @@
             .to_return(status: 200, body: "[{}]", headers: json_header)
         end
 
-        it "returns nil" do
-          expect(creator.create).to be_nil
+        it "raises a helpful error" do
+          expect { creator.create }
+            .to raise_error(Dependabot::PullRequestCreator::UnmergedPRExists)
           expect(WebMock).to_not have_requested(:post, "#{repo_api_url}/pulls")
         end
 
@@ -536,9 +537,9 @@
               )
           end
 
-          it "returns nil" do
-            expect(creator.create).to be_nil
-            expect(WebMock).to have_requested(:post, "#{repo_api_url}/pulls")
+          it "raises the error" do
+            expect { creator.create }
+              .to raise_error(Octokit::UnprocessableEntity)
           end
         end
 
@@ -580,8 +581,9 @@
           context "when `require_up_to_date_base` is true" do
             let(:require_up_to_date_base) { true }
 
-            it "does not create a PR" do
-              expect(creator.create).to be_nil
+            it "raises a helpful error" do
+              expect { creator.create }
+                .to raise_error(Dependabot::PullRequestCreator::BaseCommitNotUpToDate)
               expect(WebMock)
                 .to_not have_requested(:post, "#{repo_api_url}/pulls")
             end
@@ -796,8 +798,8 @@
                        headers: json_header)
         end
 
-        it "quietly ignores the failure" do
-          expect { creator.create }.to_not raise_error
+        it "raises the error" do
+          expect { creator.create }.to raise_error(Octokit::UnprocessableEntity)
         end
       end
     end