Rearrange things to define HasSuccessfulStatus in terms of HasStatus

This is more direct.

We want to test `HasSuccessfulStatus` now, so jiggle the testsuite a bit
to make that less repetitive by introducing a `StatusTestSuite` struct.

policy/predicate/predicates.go

@@ -33,7 +33,7 @@ type Predicates struct {
 	// `has_successful_status` is a deprecated field that is kept for backwards
 	// compatibility.  `has_status` replaces it, and can accept any conclusion
 	// rather than just "success".
-	HasSuccessfulStatus *HasStatus `yaml:"has_successful_status"`
+	HasSuccessfulStatus *HasSuccessfulStatus `yaml:"has_successful_status"`
 
 	HasLabels *HasLabels `yaml:"has_labels"`
 

policy/predicate/status.go

@@ -40,26 +40,6 @@ func NewHasStatus(statuses []string, conclusions []string) *HasStatus {
 	}
 }
 
-// UnmarshalYAML implements the yaml.Unmarshaler interface for HasStatus.
-// This supports unmarshalling the predicate in two forms:
-//  1. A list of strings, which are the statuses to check for. This is the
-//     deprecated `has_successful_status` format.
-//  2. A full structure with `statuses` and `conclusions` fields as in
-//     `has_status`.
-func (pred *HasStatus) UnmarshalYAML(unmarshal func(interface{}) error) error {
-	// Try to unmarshal as a list of strings first
-	statuses := []string{}
-	if err := unmarshal(&statuses); err == nil {
-		pred.Statuses = statuses
-
-		return nil
-	}
-
-	// If that fails, try to unmarshal as the full structure
-	type rawHasSuccessfulStatus HasStatus
-	return unmarshal((*rawHasSuccessfulStatus)(pred))
-}
-
 var _ Predicate = HasStatus{}
 
 func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
@@ -112,3 +92,23 @@ func (pred HasStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common
 func (pred HasStatus) Trigger() common.Trigger {
 	return common.TriggerStatus
 }
+
+// HasSuccessfulStatus checks that the specified statuses have a successful
+// conclusion.
+//
+// Deprecated: use the more flexible `HasStatus` with `conclusions: ["success"]`
+// instead.
+type HasSuccessfulStatus []string
+
+var _ Predicate = HasSuccessfulStatus{}
+
+func (pred HasSuccessfulStatus) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
+	return HasStatus{
+		Statuses:    pred,
+		Conclusions: allowedConclusions{"success": {}},
+	}.Evaluate(ctx, prctx)
+}
+
+func (pred HasSuccessfulStatus) Trigger() common.Trigger {
+	return common.TriggerStatus
+}

policy/predicate/status_test.go

@@ -16,9 +16,12 @@ package predicate
 
 import (
 	"context"
+	"fmt"
 	"slices"
+	"strings"
 	"testing"
 
+	"github.com/google/go-github/v62/github"
 	"github.com/palantir/policy-bot/policy/common"
 	"github.com/palantir/policy-bot/pull"
 	"github.com/palantir/policy-bot/pull/pulltest"
@@ -37,7 +40,12 @@ func keysSorted[V any](m map[string]V) []string {
 }
 
 func TestHasSuccessfulStatus(t *testing.T) {
-	p := HasStatus{Statuses: []string{"status-name", "status-name-2"}}
+	hasStatus := HasStatus{Statuses: []string{"status-name", "status-name-2"}}
+	hasStatusSkippedOk := HasStatus{
+		Statuses:    []string{"status-name", "status-name-2"},
+		Conclusions: allowedConclusions{"success": {}, "skipped": {}},
+	}
+	hasSuccessfulStatus := HasSuccessfulStatus{"status-name", "status-name-2"}
 
 	commonTestCases := []StatusTestCase{
 		{
@@ -141,22 +149,34 @@ func TestHasSuccessfulStatus(t *testing.T) {
 		},
 	}
 
-	// Run tests with skipped statuses counting as failures
-	runStatusTestCase(t, p, commonTestCases)
-	runStatusTestCase(t, p, okOnlyIfSkippedAllowed)
-
-	// Run tests with skipped statuses counting as successes
-	p.Conclusions = allowedConclusions{"success": {}, "skipped": {}}
-
-	for i := 0; i < len(commonTestCases); i++ {
-		commonTestCases[i].name += ", but skipped statuses are allowed"
+	testSuites := []StatusTestSuite{
+		{predicate: hasStatus, testCases: commonTestCases},
+		{predicate: hasStatus, testCases: okOnlyIfSkippedAllowed},
+		{predicate: hasSuccessfulStatus, testCases: commonTestCases},
+		{predicate: hasSuccessfulStatus, testCases: okOnlyIfSkippedAllowed},
+		{
+			nameSuffix: "skipped allowed",
+			predicate:  hasStatusSkippedOk,
+			testCases:  commonTestCases,
+		},
+		{
+			nameSuffix:        "skipped allowed",
+			predicate:         hasStatusSkippedOk,
+			testCases:         okOnlyIfSkippedAllowed,
+			overrideSatisfied: github.Bool(true),
+		},
 	}
-	for i := 0; i < len(okOnlyIfSkippedAllowed); i++ {
-		okOnlyIfSkippedAllowed[i].name += ", but skipped statuses are allowed"
-		okOnlyIfSkippedAllowed[i].ExpectedPredicateResult.Satisfied = true
+
+	for _, suite := range testSuites {
+		runStatusTestCase(t, suite.predicate, suite)
 	}
-	runStatusTestCase(t, p, commonTestCases)
-	runStatusTestCase(t, p, okOnlyIfSkippedAllowed)
+}
+
+type StatusTestSuite struct {
+	nameSuffix        string
+	predicate         Predicate
+	testCases         []StatusTestCase
+	overrideSatisfied *bool
 }
 
 type StatusTestCase struct {
@@ -165,10 +185,14 @@ type StatusTestCase struct {
 	ExpectedPredicateResult *common.PredicateResult
 }
 
-func runStatusTestCase(t *testing.T, p Predicate, cases []StatusTestCase) {
+func runStatusTestCase(t *testing.T, p Predicate, suite StatusTestSuite) {
 	ctx := context.Background()
 
-	for _, tc := range cases {
+	for _, tc := range suite.testCases {
+		if suite.overrideSatisfied != nil {
+			tc.ExpectedPredicateResult.Satisfied = *suite.overrideSatisfied
+		}
+
 		// If the test case expects the predicate to be satisfied, we always
 		// expect the values to contain all the statuses. Doing this here lets
 		// us use the same testcases when we allow and don't allow skipped
@@ -178,7 +202,15 @@ func runStatusTestCase(t *testing.T, p Predicate, cases []StatusTestCase) {
 			tc.ExpectedPredicateResult.Values = keysSorted(statuses)
 		}
 
-		t.Run(tc.name, func(t *testing.T) {
+		// `predicate.HasStatus` -> `HasStatus`
+		_, predicateType, _ := strings.Cut(fmt.Sprintf("%T", p), ".")
+		testName := fmt.Sprintf("%s_%s", predicateType, tc.name)
+
+		if suite.nameSuffix != "" {
+			testName += "_" + suite.nameSuffix
+		}
+
+		t.Run(testName, func(t *testing.T) {
 			predicateResult, err := p.Evaluate(ctx, tc.context)
 			if assert.NoError(t, err, "evaluation failed") {
 				assertPredicateResult(t, tc.ExpectedPredicateResult, predicateResult)