diff --git a/policy/predicate/predicates.go b/policy/predicate/predicates.go index 459cf7b9..eeb9bb53 100644 --- a/policy/predicate/predicates.go +++ b/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"` diff --git a/policy/predicate/status.go b/policy/predicate/status.go index d15131d8..4bbdcbf8 100644 --- a/policy/predicate/status.go +++ b/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 +} diff --git a/policy/predicate/status_test.go b/policy/predicate/status_test.go index 7a4cf998..b9d1075a 100644 --- a/policy/predicate/status_test.go +++ b/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)