Repository: palantir/bulldozer
Pull request #307 (2022-05-02 22:41:31)

Support required statuses for Bulldozer to update PRs
### Summary

Closes https://github.com/palantir/bulldozer/issues/259

1) Add `required_statuses` to Bulldozer update config
    - Refactored `ShouldUpdatePR` to `evaluate.go` to be alongside `ShouldMergePR` and reuse some status checks code
    - No tests exist anymore in `update_test.go` since they all covered `ShouldUpdatePR` and were refactored to `evaluate_test.go`
2) Enable update processing when status or check run events are handled
3) Additional changes
    - Improve logging to differentiate between should merge and should update logs
    - Adjusted config tests to better test provided and default config values
    - Refactor `TestShouldUpdatePR` to table-based tests so it is easier to understand the test cases

wrslatz: So it looks like status changes *not* being fulfilled is working correctly as implemented 🎉 

However, status changes themselves are not triggering an update. Looks like I need to update the check run `check_run.go`) and status update (`status.go`) handling to consider updates as well as merges like we do for pushes or other pull events.
bluekeyes: > However, status changes themselves are not triggering an update, from the local testing I've done. Looks like I need to update the check run (`check_run.go`) and status update (`status.go`) handling to consider updates as well as merges like we do for pushes or other pull events.

Yes, this sounds correct. Because statuses are pretty frequent events, it's important to filter them if we're not already doing so. At minimum, we should exit the handler immediately for non-successful statuses. Offhand, I'm not sure if there are any other optimizations to make here.
wrslatz: > Yes, this sounds correct. Because statuses are pretty frequent events, it's important to filter them if we're not already doing so. At minimum, we should exit the handler immediately for non-successful statuses.

It looks like both already handle this logic
https://github.com/palantir/bulldozer/blob/c5b53400811456a5cdf4eea3c1af9b60f8ebf27a/server/handler/status.go#L52-L55
https://github.com/palantir/bulldozer/blob/c5b53400811456a5cdf4eea3c1af9b60f8ebf27a/server/handler/check_run.go#L47-L50

> Offhand, I'm not sure if there are any other optimizations to make here.

It looks like both of these handlers act on every open PR when processing
https://github.com/palantir/bulldozer/blob/c5b53400811456a5cdf4eea3c1af9b60f8ebf27a/server/handler/status.go#L72-L82
https://github.com/palantir/bulldozer/blob/c5b53400811456a5cdf4eea3c1af9b60f8ebf27a/server/handler/check_run.go#L63-L86

Whereas we'd want the update logic to only act on the PR to which the check or status update corresponds?
wrslatz: Actually, I think I'm misunderstanding how that looping to try to merge works. Perhaps we should try updates in the same places. I'll make an attempt at that now
bluekeyes: Yeah, both checks and statuses are attached to commits, which could be the HEAD of multiple open PRs (but in practice are usually only part of one.) The loops iterate over all of the open PRs associated with the updated commit, meaning you'll want to check if updates are necessary inside the loop, either before or after checking if merges are necessary.
wrslatz: I've finished most of the code changes here. I'm rewriting the `TestShouldUpdatePR` to use a table with named props instead of a matrix so it's easier to understand what exactly is being tested.

Do you think we need the full matrix of all combinations of test scenarios? It seems like a _lot_ of tests, and I'm not sure how valuable all of the permutations are vs having tests covering the major scenarios. I can push what I have so far to help with considering, if needed.

Patch:
diff --git a/README.md b/README.md
index ab40d3b..145fa10 100644
--- a/README.md
+++ b/README.md
@@ -210,6 +210,13 @@ update:
   # If true, bulldozer will ignore updating draft pull requests, unless they
   # explicitly match a configured trigger condition.
   ignore_drafts: false
+
+  # "required_statuses" is a list of additional status contexts that must pass
+  # before bulldozer will update a pull request, unless the pull request
+  # explicitly matches a configured trigger condition. This is useful if you want
+  # to require certain statuses to pass before automated updates are made.
+  required_statuses:
+    - "policy-bot: develop"
 ```
 
 #### Remote Configuration
diff --git a/bulldozer/config_v1.go b/bulldozer/config_v1.go
index 1560ec4..3864b9e 100644
--- a/bulldozer/config_v1.go
+++ b/bulldozer/config_v1.go
@@ -76,6 +76,10 @@ type UpdateConfig struct {
 
 	IgnoreDrafts *bool `yaml:"ignore_drafts"`
 
+	// Additional status checks that bulldozer should require
+	// (even if the branch protection settings doesn't require it)
+	RequiredStatuses []string `yaml:"required_statuses"`
+
 	// Blacklist and Whitelist are legacy options that will be disabled in a future v2 format
 	Blacklist Signals `yaml:"blacklist"`
 	Whitelist Signals `yaml:"whitelist"`
diff --git a/bulldozer/evaluate.go b/bulldozer/evaluate.go
index bb9fc92..ffc67c8 100644
--- a/bulldozer/evaluate.go
+++ b/bulldozer/evaluate.go
@@ -102,34 +102,34 @@ func ShouldMergePR(ctx context.Context, pullCtx pull.Context, mergeConfig MergeC
 	if mergeConfig.Ignore.Enabled() {
 		ignored, reason, err := IsPRIgnored(ctx, pullCtx, mergeConfig.Ignore)
 		if err != nil {
-			return false, errors.Wrap(err, "failed to determine if pull request is ignored")
+			return false, errors.Wrap(err, "failed to determine if pull request is ignored for merge")
 		}
 		if ignored {
 			logger.Debug().Msgf("%s is deemed not mergeable because ignoring is enabled and %s", pullCtx.Locator(), reason)
 			return false, nil
 		}
 	} else {
-		logger.Debug().Msg("ignoring is not enabled")
+		logger.Debug().Msg("ignoring for merge is not enabled")
 	}
 
 	if mergeConfig.Trigger.Enabled() {
 		triggered, reason, err := IsPRTriggered(ctx, pullCtx, mergeConfig.Trigger)
 		if err != nil {
-			return false, errors.Wrap(err, "failed to determine if pull request is triggered")
+			return false, errors.Wrap(err, "failed to determine if pull request is triggered for merge")
 		}
 		if !triggered {
 			logger.Debug().Msgf("%s is deemed not mergeable because triggering is enabled and no trigger signal detected", pullCtx.Locator())
 			return false, nil
 		}
 
-		logger.Debug().Msgf("%s is triggered because triggering is enabled and %s", pullCtx.Locator(), reason)
+		logger.Debug().Msgf("%s is triggered for merge because triggering is enabled and %s", pullCtx.Locator(), reason)
 	} else {
-		logger.Debug().Msg("triggering is not enabled")
+		logger.Debug().Msg("triggering for merge is not enabled")
 	}
 
 	requiredStatuses, err := pullCtx.RequiredStatuses(ctx)
 	if err != nil {
-		return false, errors.Wrap(err, "failed to determine required Github status checks")
+		return false, errors.Wrap(err, "failed to determine required Github status checks for merge")
 	}
 	requiredStatuses = append(requiredStatuses, mergeConfig.RequiredStatuses...)
 
@@ -140,7 +140,7 @@ func ShouldMergePR(ctx context.Context, pullCtx pull.Context, mergeConfig MergeC
 
 	successStatuses, err := pullCtx.CurrentSuccessStatuses(ctx)
 	if err != nil {
-		return false, errors.Wrap(err, "failed to determine currently successful status checks")
+		return false, errors.Wrap(err, "failed to determine currently successful status checks for merge")
 	}
 
 	unsatisfiedStatuses := statusSetDifference(requiredStatuses, successStatuses)
@@ -152,3 +152,58 @@ func ShouldMergePR(ctx context.Context, pullCtx pull.Context, mergeConfig MergeC
 	// Ignore required reviews and try a merge (which may fail with a 4XX).
 	return true, nil
 }
+
+func ShouldUpdatePR(ctx context.Context, pullCtx pull.Context, updateConfig UpdateConfig) (bool, error) {
+	logger := zerolog.Ctx(ctx)
+
+	if !updateConfig.Ignore.Enabled() && !updateConfig.Trigger.Enabled() && updateConfig.IgnoreDrafts == nil && len(updateConfig.RequiredStatuses) == 0 {
+		return false, nil
+	}
+
+	if updateConfig.Ignore.Enabled() {
+		ignored, reason, err := IsPRIgnored(ctx, pullCtx, updateConfig.Ignore)
+		if err != nil {
+			return false, errors.Wrap(err, "failed to determine if pull request is ignored for update")
+		}
+		if ignored {
+			logger.Debug().Msgf("%s is deemed not updateable because ignoring is enabled and %s", pullCtx.Locator(), reason)
+			return false, nil
+		}
+	}
+
+	if updateConfig.Trigger.Enabled() {
+		triggered, reason, err := IsPRTriggered(ctx, pullCtx, updateConfig.Trigger)
+		if err != nil {
+			return false, errors.Wrap(err, "failed to determine if pull request is triggered for update")
+		}
+		if !triggered {
+			logger.Debug().Msgf("%s is deemed not updateable because triggering is enabled and no trigger signal detected", pullCtx.Locator())
+			return false, nil
+		}
+
+		logger.Debug().Msgf("%s is triggered for update because triggering is enabled and %s", pullCtx.Locator(), reason)
+		return true, nil
+	}
+
+	if updateConfig.IgnoreDrafts != nil && *updateConfig.IgnoreDrafts && pullCtx.IsDraft(ctx) {
+		logger.Debug().Msgf("%s is deemed not updateable because PR is in a draft state", pullCtx.Locator())
+		return false, nil
+	}
+
+	requiredStatuses := updateConfig.RequiredStatuses
+
+	if len(requiredStatuses) > 0 {
+		successStatuses, err := pullCtx.CurrentSuccessStatuses(ctx)
+		if err != nil {
+			return false, errors.Wrap(err, "failed to determine currently successful status checks for update")
+		}
+
+		unsatisfiedStatuses := statusSetDifference(requiredStatuses, successStatuses)
+		if len(unsatisfiedStatuses) > 0 {
+			logger.Debug().Msgf("%s is deemed not updateable because of unfulfilled status checks: [%s]", pullCtx.Locator(), strings.Join(unsatisfiedStatuses, ","))
+			return false, nil
+		}
+	}
+
+	return true, nil
+}
diff --git a/bulldozer/update.go b/bulldozer/update.go
index 3d60e6a..7517b90 100644
--- a/bulldozer/update.go
+++ b/bulldozer/update.go
@@ -23,46 +23,6 @@ import (
 	"github.com/rs/zerolog"
 )
 
-func ShouldUpdatePR(ctx context.Context, pullCtx pull.Context, updateConfig UpdateConfig) (bool, error) {
-	logger := zerolog.Ctx(ctx)
-
-	if !updateConfig.Ignore.Enabled() && !updateConfig.Trigger.Enabled() && updateConfig.IgnoreDrafts == nil {
-		return false, nil
-	}
-
-	if updateConfig.Ignore.Enabled() {
-		ignored, reason, err := IsPRIgnored(ctx, pullCtx, updateConfig.Ignore)
-		if err != nil {
-			return false, errors.Wrap(err, "failed to determine if pull request is ignored")
-		}
-		if ignored {
-			logger.Debug().Msgf("%s is deemed not updateable because ignoring is enabled and %s", pullCtx.Locator(), reason)
-			return false, nil
-		}
-	}
-
-	if updateConfig.Trigger.Enabled() {
-		triggered, reason, err := IsPRTriggered(ctx, pullCtx, updateConfig.Trigger)
-		if err != nil {
-			return false, errors.Wrap(err, "failed to determine if pull request is triggered")
-		}
-		if !triggered {
-			logger.Debug().Msgf("%s is deemed not updateable because triggering is enabled and no trigger signal detected", pullCtx.Locator())
-			return false, nil
-		}
-
-		logger.Debug().Msgf("%s is triggered because triggering is enabled and %s", pullCtx.Locator(), reason)
-		return true, nil
-	}
-
-	if updateConfig.IgnoreDrafts != nil && *updateConfig.IgnoreDrafts && pullCtx.IsDraft(ctx) {
-		logger.Debug().Msgf("%s is deemed not updateable because PR is in a draft state", pullCtx.Locator())
-		return false, nil
-	}
-
-	return true, nil
-}
-
 func UpdatePR(ctx context.Context, pullCtx pull.Context, client *github.Client, updateConfig UpdateConfig, baseRef string) bool {
 	logger := zerolog.Ctx(ctx)
 
diff --git a/server/handler/check_run.go b/server/handler/check_run.go
index 98ad7f4..c9dc67a 100644
--- a/server/handler/check_run.go
+++ b/server/handler/check_run.go
@@ -77,6 +77,19 @@ func (h *CheckRun) Handle(ctx context.Context, eventType, deliveryID string, pay
 		if err != nil {
 			return err
 		}
+
+		if h.DisableUpdateFeature {
+			logger.Debug().Msgf("Skipping updates to pull request due to server configuration override")
+		} else {
+			base, _ := pullCtx.Branches()
+			didUpdatePR, err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, base)
+			if err != nil {
+				logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request")
+			}
+			if didUpdatePR {
+				continue
+			}
+		}
 		if err := h.ProcessPullRequest(ctx, pullCtx, client, config, fullPR); err != nil {
 			logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request")
 		}
diff --git a/server/handler/status.go b/server/handler/status.go
index d675ba6..e6b495b 100644
--- a/server/handler/status.go
+++ b/server/handler/status.go
@@ -76,6 +76,19 @@ func (h *Status) Handle(ctx context.Context, eventType, deliveryID string, paylo
 		if err != nil {
 			return err
 		}
+
+		if h.DisableUpdateFeature {
+			logger.Debug().Msgf("Skipping updates to pull request due to server configuration override")
+		} else {
+			base, _ := pullCtx.Branches()
+			didUpdatePR, err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, config, pr, base)
+			if err != nil {
+				logger.Error().Err(errors.WithStack(err)).Msg("Error updating pull request")
+			}
+			if didUpdatePR {
+				continue
+			}
+		}
 		if err := h.ProcessPullRequest(logger.WithContext(ctx), pullCtx, client, config, pr); err != nil {
 			logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request")
 		}


Test patch:
diff --git a/bulldozer/config_test.go b/bulldozer/config_test.go
index a9ef5f3..21be694 100644
--- a/bulldozer/config_test.go
+++ b/bulldozer/config_test.go
@@ -38,6 +38,7 @@ merge:
     squash:
       body: summarize_commits
   delete_after_merge: true
+  required_statuses: ["Test 1", "Test 2"]
 
 update:
   trigger:
@@ -45,6 +46,7 @@ update:
   ignore:
     labels: ["do not update"]
   ignore_drafts: true
+  required_statuses: ["Test 3", "Test 4"]
 `
 
 		actual, err := ParseConfig([]byte(config))
@@ -57,7 +59,34 @@ update:
 			Labels:            []string{"do not merge"},
 			CommentSubstrings: []string{"==DO_NOT_MERGE=="},
 		}, actual.Merge.Ignore)
+		assert.Equal(t, []string{"Test 1", "Test 2"}, actual.Merge.RequiredStatuses)
+
 		assert.Equal(t, *actual.Update.IgnoreDrafts, true)
+		assert.Equal(t, []string{"Test 3", "Test 4"}, actual.Update.RequiredStatuses)
+	})
+
+	t.Run("parseDefaults", func(t *testing.T) {
+		config := `
+version: 1
+
+merge:
+  method: squash
+  options:
+    squash:
+      body: summarize_commits
+`
+
+		actual, err := ParseConfig([]byte(config))
+		require.Nil(t, err)
+
+		assert.Empty(t, actual.Merge.Trigger)
+		assert.Empty(t, actual.Merge.Ignore)
+		assert.Empty(t, actual.Merge.RequiredStatuses)
+
+		assert.Empty(t, actual.Update.Trigger)
+		assert.Empty(t, actual.Update.Ignore)
+		assert.Nil(t, actual.Update.IgnoreDrafts)
+		assert.Empty(t, actual.Update.RequiredStatuses)
 	})
 
 	t.Run("parseExisting", func(t *testing.T) {
@@ -102,7 +131,6 @@ update:
 		assert.Equal(t, Signals{
 			Labels: []string{"do not update"},
 		}, actual.Update.Ignore)
-		assert.Nil(t, actual.Update.IgnoreDrafts)
 	})
 
 	t.Run("ignoresOldConfig", func(t *testing.T) {
diff --git a/bulldozer/evaluate_test.go b/bulldozer/evaluate_test.go
index 837c942..6475f8c 100644
--- a/bulldozer/evaluate_test.go
+++ b/bulldozer/evaluate_test.go
@@ -16,6 +16,7 @@ package bulldozer
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	"github.com/palantir/bulldozer/pull/pulltest"
@@ -326,3 +327,552 @@ func TestShouldMerge(t *testing.T) {
 		assert.True(t, actualShouldMerge)
 	})
 }
+
+func TestShouldUpdatePR(t *testing.T) {
+	ctx := context.Background()
+	tests := map[string]struct {
+		pullCtx         pulltest.MockPullContext
+		updateConfig    UpdateConfig
+		expectingUpdate bool
+	}{
+		// Test default cases and trigger / ignore handling (excluding drafts and required statuses)
+		"default": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+			},
+			updateConfig:    UpdateConfig{},
+			expectingUpdate: false,
+		},
+		"labelsOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore", "trigger"},
+			},
+			updateConfig:    UpdateConfig{},
+			expectingUpdate: false,
+		},
+		"ignoreLabelOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore"},
+			},
+			updateConfig:    UpdateConfig{},
+			expectingUpdate: false,
+		},
+		"triggerLabelOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"trigger"},
+			},
+			updateConfig:    UpdateConfig{},
+			expectingUpdate: false,
+		},
+		"configOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignoreConfigOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"triggerConfigOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+			},
+			updateConfig: UpdateConfig{
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignoreConfigTriggerLabel": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"trigger"},
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"triggerConfigIgnoreLabel": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore"},
+			},
+			updateConfig: UpdateConfig{
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignored": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore"},
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"triggered": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"trigger"},
+			},
+			updateConfig: UpdateConfig{
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoredAndTriggered": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore", "trigger"},
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"triggeredWithIgnoreConfig": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"trigger"},
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoredWithTriggerConfig": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore"},
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"triggeredWithIgnoreLabel": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"trigger", "ignore"},
+			},
+			updateConfig: UpdateConfig{
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoredWithTriggerLabel": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore", "trigger"},
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		// Test ignore draft handling
+		"defaultDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+			},
+			updateConfig:    UpdateConfig{},
+			expectingUpdate: false,
+		},
+		"ignoredDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+				LabelValue:   []string{"ignore"},
+			},
+			updateConfig: UpdateConfig{
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"triggeredDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+				LabelValue:   []string{"trigger"},
+			},
+			updateConfig: UpdateConfig{
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoreDraftsDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+			},
+			expectingUpdate: false,
+		},
+		"ignoreDraftsNonDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+			},
+			expectingUpdate: true,
+		},
+		"ignoreDraftsAndTriggeredDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+				LabelValue:   []string{"trigger"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoreDraftsAndTriggeredNonDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"trigger"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoreDraftsAndIgnoredDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+				LabelValue:   []string{"ignore"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignoreDraftsAndIgnoredNonDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignoreDraftsTriggeredAndIgnoredDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+				LabelValue:   []string{"ignore", "trigger"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignoreDraftsTriggeredAndIgnoredNonDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: false,
+				LabelValue:   []string{"ignore", "trigger"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignoreDraftsAndIgnoreConfigOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"ignoreDraftsAndTriggerConfigOnly": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue: true,
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts: boolVal(true),
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		// Test required statuses handling
+		"statusesOnly": {
+			pullCtx: pulltest.MockPullContext{
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig:    UpdateConfig{},
+			expectingUpdate: false,
+		},
+		"fulfilledStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "status2"},
+			},
+			expectingUpdate: true,
+		},
+		"missingStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				SuccessStatusesValue: []string{"status1"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "status2"},
+			},
+			expectingUpdate: false,
+		},
+		"fulfilledTravisCIStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				SuccessStatusesValue: []string{"status1", "continuous-integration/travis-ci"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "continuous-integration/travis-ci"},
+			},
+			expectingUpdate: true,
+		},
+		"missingTravisCIStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				SuccessStatusesValue: []string{"status1"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "continuous-integration/travis-ci"},
+			},
+			expectingUpdate: false,
+		},
+		"triggeredMissingStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				LabelValue:           []string{"trigger"},
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1"},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoredMissingStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				LabelValue:           []string{"ignore"},
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1"},
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"triggeredAndIgnoredMissingStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				LabelValue:           []string{"trigger", "ignore"},
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1"},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"triggeredFulfilledStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				LabelValue:           []string{"trigger"},
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "status2"},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+			},
+			expectingUpdate: true,
+		},
+		"ignoredFulfilledStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				LabelValue:           []string{"ignore"},
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "status2"},
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"triggeredAndIgnoredFulfilledStatuses": {
+			pullCtx: pulltest.MockPullContext{
+				LabelValue:           []string{"trigger", "ignore"},
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "status2"},
+				Trigger: Signals{
+					Labels: []string{"trigger"},
+				},
+				Ignore: Signals{
+					Labels: []string{"ignore"},
+				},
+			},
+			expectingUpdate: false,
+		},
+		"missingStatusesDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue:         true,
+				SuccessStatusesValue: []string{"status1"},
+			},
+			updateConfig: UpdateConfig{
+				RequiredStatuses: []string{"status1", "status2"},
+			},
+			expectingUpdate: false,
+		},
+		"missingStatusesIgnoreDraftDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue:         true,
+				SuccessStatusesValue: []string{"status1"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts:     boolVal(true),
+				RequiredStatuses: []string{"status1", "status2"},
+			},
+			expectingUpdate: false,
+		},
+		"fulfilledStatusesIgnoreDraftDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue:         true,
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts:     boolVal(true),
+				RequiredStatuses: []string{"status1", "status2"},
+			},
+			expectingUpdate: false,
+		},
+		"missingStatusesIgnoreDraftNonDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue:         false,
+				SuccessStatusesValue: []string{"status1"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts:     boolVal(true),
+				RequiredStatuses: []string{"status1", "status2"},
+			},
+			expectingUpdate: false,
+		},
+		"fulfilledStatusesIgnoreDraftNonDraft": {
+			pullCtx: pulltest.MockPullContext{
+				IsDraftValue:         false,
+				SuccessStatusesValue: []string{"status1", "status2"},
+			},
+			updateConfig: UpdateConfig{
+				IgnoreDrafts:     boolVal(true),
+				RequiredStatuses: []string{"status1", "status2"},
+			},
+			expectingUpdate: true,
+		},
+		// Test error handling
+	}
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			updating, err := ShouldUpdatePR(ctx, &test.pullCtx, test.updateConfig)
+			require.NoError(t, err)
+			msg := fmt.Sprintf("case %s - pullCtx %+v updateConfig %+v -> expectingUpdate=%t",
+				name, test.pullCtx, test.updateConfig, test.expectingUpdate)
+			require.Equal(t, test.expectingUpdate, updating, msg)
+		})
+	}
+}
+
+func boolVal(b bool) *bool {
+	return &b
+}
diff --git a/bulldozer/update_test.go b/bulldozer/update_test.go
index 71d0440..858e434 100644
--- a/bulldozer/update_test.go
+++ b/bulldozer/update_test.go
@@ -13,136 +13,3 @@
 // limitations under the License.
 
 package bulldozer
-
-import (
-	"context"
-	"fmt"
-	"strconv"
-	"testing"
-
-	"github.com/palantir/bulldozer/pull"
-	"github.com/palantir/bulldozer/pull/pulltest"
-	"github.com/stretchr/testify/require"
-)
-
-func TestShouldUpdatePR(t *testing.T) {
-	ctx := context.Background()
-	testMatrix := []struct {
-		ignoreEnabled   bool
-		ignored         bool
-		triggerEnabled  bool
-		triggered       bool
-		ignoreDrafts    *bool
-		isDraft         bool
-		expectingUpdate bool
-	}{
-		{false, false, false, false, nil, false, false},
-		{false, false, false, true, nil, false, false},
-		{false, false, true, false, nil, false, false},
-		{false, false, true, true, nil, false, true},
-		{false, true, false, false, nil, false, false},
-		{false, true, false, true, nil, false, false},
-		{false, true, true, false, nil, false, false},
-		{false, true, true, true, nil, false, true},
-		{true, false, false, false, nil, false, true},
-		{true, false, false, true, nil, false, true},
-		{true, false, true, false, nil, false, false},
-		{true, false, true, true, nil, false, true},
-		{true, true, false, false, nil, false, false},
-		{true, true, false, true, nil, false, false},
-		{true, true, true, false, nil, false, false},
-		{true, true, true, true, nil, false, false},
-		// Test Draft PRs are still handled correctly when ignoring them is not configured
-		{false, false, false, false, nil, true, false},
-		{false, false, false, true, nil, true, false},
-		{false, false, true, false, nil, true, false},
-		{false, false, true, true, nil, true, true},
-		{false, true, false, false, nil, true, false},
-		{false, true, false, true, nil, true, false},
-		{false, true, true, false, nil, true, false},
-		{false, true, true, true, nil, true, true},
-		{true, false, false, false, nil, true, true},
-		{true, false, false, true, nil, true, true},
-		{true, false, true, false, nil, true, false},
-		{true, false, true, true, nil, true, true},
-		{true, true, false, false, nil, true, false},
-		{true, true, false, true, nil, true, false},
-		{true, true, true, false, nil, true, false},
-		{true, true, true, true, nil, true, false},
-		// Test Draft PRs are handled correctly when ignoring them is not enabled
-		{false, false, false, false, boolVal(false), true, true}, // All updates enabled
-		{false, false, false, true, boolVal(false), true, true},  // All updates enabled
-		{false, false, true, false, boolVal(false), true, false},
-		{false, false, true, true, boolVal(false), true, true},
-		{false, true, false, false, boolVal(false), true, true}, // All updates enabled
-		{false, true, false, true, boolVal(false), true, true},  // All updates enabled
-		{false, true, true, false, boolVal(false), true, false},
-		{false, true, true, true, boolVal(false), true, true},
-		{true, false, false, false, boolVal(false), true, true},
-		{true, false, false, true, boolVal(false), true, true},
-		{true, false, true, false, boolVal(false), true, false},
-		{true, false, true, true, boolVal(false), true, true},
-		{true, true, false, false, boolVal(false), true, false},
-		{true, true, false, true, boolVal(false), true, false},
-		{true, true, true, false, boolVal(false), true, false},
-		{true, true, true, true, boolVal(false), true, false},
-		// Test Draft PRs are handled correctly when ignoring them is enabled
-		{false, false, false, false, boolVal(true), true, false},
-		{true, true, false, false, boolVal(true), true, false},
-		{true, false, false, false, boolVal(true), true, false},
-		{false, true, false, false, boolVal(true), true, false},
-		{false, false, true, true, boolVal(true), true, true},
-		{false, false, true, false, boolVal(true), true, false},
-		{false, false, false, true, boolVal(true), true, false},
-		{true, true, true, true, boolVal(true), true, false},
-		{true, false, true, true, boolVal(true), true, true},
-		{false, true, true, true, boolVal(true), true, true},
-		{true, true, false, true, boolVal(true), true, false},
-		{true, true, true, false, boolVal(true), true, false},
-		{true, false, true, false, boolVal(true), true, false},
-		{false, true, false, true, boolVal(true), true, false},
-	}
-
-	for ndx, testCase := range testMatrix {
-		pullCtx, updateConfig := generateUpdateTestCase(testCase.ignoreEnabled, testCase.ignored, testCase.triggerEnabled, testCase.triggered, testCase.ignoreDrafts, testCase.isDraft)
-		updating, err := ShouldUpdatePR(ctx, pullCtx, updateConfig)
-		require.NoError(t, err)
-		ignoreDraftsPrintVal := "nil"
-		if testCase.ignoreDrafts != nil {
-			ignoreDraftsPrintVal = strconv.FormatBool(*testCase.ignoreDrafts)
-		}
-		msg := fmt.Sprintf("case %d - ignoreEnabled=%t ignored=%t triggerEnabled=%t triggered=%t ignoreDrafts=%v isDraft=%t -> doUpdate=%t",
-			ndx, testCase.ignoreEnabled, testCase.ignored, testCase.triggerEnabled, testCase.triggered, ignoreDraftsPrintVal, testCase.isDraft, testCase.expectingUpdate)
-		require.Equal(t, testCase.expectingUpdate, updating, msg)
-	}
-}
-func generateUpdateTestCase(ignorable bool, ignored bool, triggerable bool, triggered bool, ignoreDrafts *bool, isDraft bool) (pull.Context, UpdateConfig) {
-	updateConfig := UpdateConfig{
-		IgnoreDrafts: ignoreDrafts,
-	}
-	pullCtx := pulltest.MockPullContext{
-		IsDraftValue: isDraft,
-	}
-
-	if ignorable {
-		updateConfig.Ignore.Labels = append(updateConfig.Ignore.Labels, "ignore")
-	}
-
-	if ignored {
-		pullCtx.LabelValue = append(pullCtx.LabelValue, "ignore")
-	}
-
-	if triggerable {
-		updateConfig.Trigger.Labels = append(updateConfig.Trigger.Labels, "trigger")
-	}
-
-	if triggered {
-		pullCtx.LabelValue = append(pullCtx.LabelValue, "trigger")
-	}
-
-	return &pullCtx, updateConfig
-}
-
-func boolVal(b bool) *bool {
-	return &b
-}