test: harden gh push-cancel assert against races

The TestGithubSecondPushRequestGitOpsCommentCancel flow was flaky under
timing variance because its primary cancel detection path depended on
non-deterministic status propagation and compared against the wrong
Tekton reason constant.

Use PipelineRunReasonCancelled instead of TaskRunReasonCancelled, add
UntilRepositoryHasStatusReason(...) scoped by target SHA, make
cancellation waits explicit, keep controller-log matching as diagnostic
fallback, and add unit coverage for the new wait helper.

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

Author: chmouel

test/github_push_retest_test.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"regexp"
 	"testing"
+	"time"
 
 	"github.com/google/go-github/v81/github"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
@@ -183,22 +184,21 @@ func TestGithubSecondPushRequestGitOpsCommentCancel(t *testing.T) {
 		&github.RepositoryComment{Body: github.Ptr(comment)})
 	assert.NilError(t, err)
 
-	g.Cnx.Clients.Log.Infof("Waiting for Repository to be updated still to %d since it has been cancelled", numberOfStatus)
-	repo, _ := twait.UntilRepositoryUpdated(ctx, g.Cnx.Clients, waitOpts) // don't check for error, because cancelled is not success and this will fail
-	cancelled := false
-	for _, c := range repo.Status {
-		if c.Conditions[0].Reason == tektonv1.TaskRunReasonCancelled.String() {
-			cancelled = true
-		}
-	}
+	cancelWaitOpts := waitOpts
+	cancelWaitOpts.MinNumberStatus = 1
+	cancelWaitOpts.PollTimeout = 90 * time.Second
 
-	// this went too fast so at least we check it was requested for it
-	if !cancelled {
+	// wait for the cancellation to propagate through PipelineRun status and repository status
+	err = twait.UntilPipelineRunHasReason(ctx, g.Cnx.Clients, tektonv1.PipelineRunReasonCancelled, cancelWaitOpts)
+	if err == nil {
+		_, err = twait.UntilRepositoryHasStatusReason(ctx, g.Cnx.Clients, cancelWaitOpts, tektonv1.PipelineRunReasonCancelled.String())
+	}
+	if err != nil {
 		numLines := int64(1000)
-		reg := regexp.MustCompile(".*pipelinerun.*skipping cancelling pipelinerun.*on-push.*already done.*")
-		err = twait.RegexpMatchingInControllerLog(ctx, g.Cnx, *reg, 10, "ghe-controller", &numLines)
-		if err != nil {
-			t.Errorf("neither a cancelled pipelinerun in repo status or a request to skip the cancellation in the controller log was found: %s", err.Error())
+		reg := regexp.MustCompile(".*cancel-in-progress:.*pipelinerun.*on-push.*")
+		logErr := twait.RegexpMatchingInControllerLog(ctx, g.Cnx, *reg, 10, "ghe-controller", &numLines)
+		if logErr != nil {
+			t.Errorf("neither a cancelled pipelinerun in repo status or a cancellation request in the controller log was found: status wait error: %s, log wait error: %s", err.Error(), logErr.Error())
 		}
 		return
 	}
@@ -209,9 +209,9 @@ func TestGithubSecondPushRequestGitOpsCommentCancel(t *testing.T) {
 	})
 	assert.NilError(t, err)
 	assert.Equal(t, len(pruns.Items), numberOfStatus)
-	cancelled = false
+	cancelled := false
 	for _, pr := range pruns.Items {
-		if pr.Status.Conditions[0].Reason == tektonv1.TaskRunReasonCancelled.String() {
+		if len(pr.Status.Conditions) > 0 && pr.Status.Conditions[0].Reason == tektonv1.PipelineRunReasonCancelled.String() {
 			cancelled = true
 		}
 	}

test/pkg/wait/wait.go

@@ -87,6 +87,42 @@ func UntilRepositoryUpdated(ctx context.Context, clients clients.Clients, opts O
 	})
 }
 
+func UntilRepositoryHasStatusReason(ctx context.Context, clients clients.Clients, opts Opts, reason string) (*pacv1alpha1.Repository, error) {
+	ctx, cancel := context.WithTimeout(ctx, opts.PollTimeout)
+	defer cancel()
+	var repo *pacv1alpha1.Repository
+	return repo, kubeinteraction.PollImmediateWithContext(ctx, opts.PollTimeout, func() (bool, error) {
+		var err error
+		if repo, err = clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(opts.Namespace).Get(ctx, opts.RepoName, metav1.GetOptions{}); err != nil {
+			return true, err
+		}
+
+		matchingStatuses := 0
+		reasons := []string{}
+		for _, status := range repo.Status {
+			if opts.TargetSHA != "" {
+				if status.SHA == nil || *status.SHA != opts.TargetSHA {
+					continue
+				}
+			}
+			matchingStatuses++
+			if len(status.Conditions) == 0 {
+				continue
+			}
+			reasons = append(reasons, status.Conditions[0].Reason)
+			if status.Conditions[0].Reason == reason {
+				return true, nil
+			}
+		}
+
+		clients.Log.Infof(
+			"Still waiting for repository status reason to be updated: wanted=%q matching statuses=%d reasons=%v",
+			reason, matchingStatuses, reasons,
+		)
+		return false, nil
+	})
+}
+
 func UntilPipelineRunCreated(ctx context.Context, clients clients.Clients, opts Opts) error {
 	ctx, cancel := context.WithTimeout(ctx, opts.PollTimeout)
 	defer cancel()

test/pkg/wait/wait_test.go

@@ -0,0 +1,138 @@
+package wait
+
+import (
+	"testing"
+	"time"
+
+	pacv1alpha1 "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	paramsclients "github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
+	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
+	"go.uber.org/zap"
+	"gotest.tools/v3/assert"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	knativeapis "knative.dev/pkg/apis"
+	duckv1 "knative.dev/pkg/apis/duck/v1"
+	rtesting "knative.dev/pkg/reconciler/testing"
+)
+
+func makeRepositoryRunStatus(sha *string, reason string) pacv1alpha1.RepositoryRunStatus {
+	status := pacv1alpha1.RepositoryRunStatus{
+		SHA: sha,
+	}
+	if reason != "" {
+		status.Status = duckv1.Status{
+			Conditions: []knativeapis.Condition{
+				{Reason: reason},
+			},
+		}
+	}
+	return status
+}
+
+func strPtr(v string) *string {
+	return &v
+}
+
+func TestUntilRepositoryHasStatusReason(t *testing.T) {
+	tests := []struct {
+		name      string
+		statuses  []pacv1alpha1.RepositoryRunStatus
+		targetSHA string
+		reason    string
+		wantErr   bool
+	}{
+		{
+			name: "match by target sha and reason",
+			statuses: []pacv1alpha1.RepositoryRunStatus{
+				makeRepositoryRunStatus(strPtr("sha-1"), "Succeeded"),
+				makeRepositoryRunStatus(strPtr("sha-2"), "Cancelled"),
+			},
+			targetSHA: "sha-2",
+			reason:    "Cancelled",
+		},
+		{
+			name: "wrong reason for matching sha",
+			statuses: []pacv1alpha1.RepositoryRunStatus{
+				makeRepositoryRunStatus(strPtr("sha-2"), "Succeeded"),
+			},
+			targetSHA: "sha-2",
+			reason:    "Cancelled",
+			wantErr:   true,
+		},
+		{
+			name: "reason exists on a different sha only",
+			statuses: []pacv1alpha1.RepositoryRunStatus{
+				makeRepositoryRunStatus(strPtr("sha-1"), "Cancelled"),
+			},
+			targetSHA: "sha-2",
+			reason:    "Cancelled",
+			wantErr:   true,
+		},
+		{
+			name: "match without target sha filter",
+			statuses: []pacv1alpha1.RepositoryRunStatus{
+				makeRepositoryRunStatus(strPtr("sha-1"), "Cancelled"),
+			},
+			reason: "Cancelled",
+		},
+		{
+			name: "status without conditions",
+			statuses: []pacv1alpha1.RepositoryRunStatus{
+				makeRepositoryRunStatus(strPtr("sha-2"), ""),
+			},
+			targetSHA: "sha-2",
+			reason:    "Cancelled",
+			wantErr:   true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, _ := rtesting.SetupFakeContext(t)
+			repositoryNS := "test-repository-ns"
+			repositoryName := "test-repository"
+			seeded, _ := testclient.SeedTestData(t, ctx, testclient.Data{
+				Namespaces: []*corev1.Namespace{
+					{
+						ObjectMeta: metav1.ObjectMeta{Name: repositoryNS},
+					},
+				},
+				Repositories: []*pacv1alpha1.Repository{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      repositoryName,
+							Namespace: repositoryNS,
+						},
+						Spec: pacv1alpha1.RepositorySpec{
+							URL: "https://ghe.pipelinesascode.com/org/repo",
+						},
+						Status: tt.statuses,
+					},
+				},
+			})
+
+			clients := paramsclients.Clients{
+				PipelineAsCode: seeded.PipelineAsCode,
+				Log:            zap.NewNop().Sugar(),
+			}
+			opts := Opts{
+				RepoName:    repositoryName,
+				Namespace:   repositoryNS,
+				TargetSHA:   tt.targetSHA,
+				PollTimeout: 10 * time.Millisecond,
+			}
+
+			repository, err := UntilRepositoryHasStatusReason(ctx, clients, opts, tt.reason)
+			if tt.wantErr {
+				assert.Assert(t, err != nil)
+				assert.ErrorContains(t, err, "timed out")
+				return
+			}
+
+			assert.NilError(t, err)
+			assert.Assert(t, repository != nil)
+			assert.Equal(t, repository.GetName(), repositoryName)
+		})
+	}
+}