fix: Make error handling and tests more robust

hack/update-golden.sh

@@ -2,4 +2,5 @@
 # This will run `go test a/package -test.update-golden=true` on all packages that are importing `gotest.tools/v3/golden`
 # This will update the golden files with the current output.
 # Run this only when you are sure the output is meant to change.
-go test $(go list -f '{{ .ImportPath }} {{ .TestImports }}' ./... | grep gotest.tools/v3/golden | awk '{print $1}' | tr '\n' ' ') -test.update-golden=true
+go test $(go list -f '{{ .ImportPath }} {{ .TestImports }}' ./pkg/... | grep gotest.tools/v3/golden | awk '{print $1}' | tr '
+' ' ') -test.update-golden=true

pkg/adapter/adapter.go

@@ -182,10 +182,14 @@ func (l listener) handleEvent(ctx context.Context) http.HandlerFunc {
 		}
 
 		// figure out which provider request coming from
-		if err != nil || gitProvider == nil {
+		if err != nil {
 			l.writeResponse(response, http.StatusOK, err.Error())
 			return
 		}
+		if gitProvider == nil {
+			l.writeResponse(response, http.StatusOK, "gitProvider is nil")
+			return
+		}
 		gitProvider.SetPacInfo(&pacInfo)
 
 		s := sinker{

pkg/cli/webhook/bitbucket_cloud_test.go

@@ -78,7 +78,10 @@ func TestAskBBWebhookConfig(t *testing.T) {
 			bb := bitbucketCloudConfig{IOStream: io}
 			err := bb.askBBWebhookConfig(tt.repoURL, tt.controllerURL, "", tt.personalaccesstoken)
 			if tt.wantErrStr != "" {
-				assert.Equal(t, err.Error(), tt.wantErrStr)
+				assert.Assert(t, err != nil, "expected an error but got nil")
+				if err != nil {
+					assert.Equal(t, err.Error(), tt.wantErrStr)
+				}
 				return
 			}
 			assert.NilError(t, err)

pkg/cli/webhook/github_test.go

@@ -86,7 +86,10 @@ func TestAskGHWebhookConfig(t *testing.T) {
 			gh := gitHubConfig{IOStream: io}
 			err := gh.askGHWebhookConfig(tt.repoURL, tt.controllerURL, "", tt.personalaccesstoken)
 			if tt.wantErrStr != "" {
-				assert.Equal(t, err.Error(), tt.wantErrStr)
+				assert.Assert(t, err != nil, "expected an error but got nil")
+				if err != nil {
+					assert.Equal(t, err.Error(), tt.wantErrStr)
+				}
 				return
 			}
 			assert.NilError(t, err)

pkg/cli/webhook/gitlab_test.go

@@ -75,7 +75,10 @@ func TestAskGLWebhookConfig(t *testing.T) {
 			gl := gitLabConfig{IOStream: io}
 			err := gl.askGLWebhookConfig(tt.repoURL, tt.controllerURL, tt.providerURL, tt.personalaccesstoken)
 			if tt.wantErrStr != "" {
-				assert.Equal(t, err.Error(), tt.wantErrStr)
+				assert.Assert(t, err != nil, "expected an error but got nil")
+				if err != nil {
+					assert.Equal(t, err.Error(), tt.wantErrStr)
+				}
 				return
 			}
 			assert.NilError(t, err)

pkg/cmd/tknpac/bootstrap/bootstrap_test.go

@@ -320,7 +320,9 @@ func TestGetDashboardURL(t *testing.T) {
 			err := getDashboardURL(ctx, opts, run)
 			if tt.wantError {
 				assert.Assert(t, err != nil)
-				assert.Assert(t, strings.Contains(err.Error(), tt.errorMsg))
+				if err != nil {
+					assert.Assert(t, strings.Contains(err.Error(), tt.errorMsg))
+				}
 			} else {
 				assert.NilError(t, err)
 				assert.Equal(t, tt.wantURL, opts.dashboardURL)

pkg/matcher/annotation_matcher_test.go

@@ -2546,7 +2546,7 @@ func TestGetTargetBranch(t *testing.T) {
 			matched, targetEvent, targetBranch, err := getTargetBranch(tt.prun, tt.event)
 			if tt.expectedError != "" {
 				assert.Assert(t, err != nil)
-				assert.Error(t, err, tt.expectedError, err.Error())
+				assert.Error(t, err, tt.expectedError)
 				return
 			}
 			assert.NilError(t, err)

pkg/matcher/repo_runinfo_matcher.go

@@ -14,10 +14,10 @@ import (
 func MatchEventURLRepo(ctx context.Context, cs *params.Run, event *info.Event, ns string) (*apipac.Repository, error) {
 	repositories, err := cs.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(ns).List(
 		ctx, metav1.ListOptions{})
-	sort.RepositorySortByCreationOldestTime(repositories.Items)
 	if err != nil {
 		return nil, err
 	}
+	sort.RepositorySortByCreationOldestTime(repositories.Items)
 	for _, repo := range repositories.Items {
 		repo.Spec.URL = strings.TrimSuffix(repo.Spec.URL, "/")
 		if repo.Spec.URL == event.URL {

pkg/matcher/repo_runinfo_matcher_test.go

@@ -13,7 +13,6 @@ import (
 	testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository"
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
-	"gotest.tools/v3/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
@@ -181,18 +180,25 @@ func Test_getRepoByCR(t *testing.T) {
 			}
 			got, err := MatchEventURLRepo(ctx, client, &tt.args.runevent, "")
 
-			if err == nil && tt.wantErr {
-				assert.NilError(t, err, "GetRepoByCR() error = %v, wantErr %v", err, tt.wantErr)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("MatchEventURLRepo() error = %v, wantErr %v", err, tt.wantErr)
+				return
 			}
-			if tt.wantTargetNS == "" && got != nil {
-				t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.GetNamespace(), tt.wantTargetNS)
-			}
-			if tt.wantTargetNS != "" && got == nil {
-				t.Errorf("GetRepoByCR() want nil got '%v'", tt.wantTargetNS)
+
+			if tt.wantErr {
+				return
 			}
 
-			if tt.wantTargetNS != "" && tt.wantTargetNS != got.GetNamespace() {
-				t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.GetNamespace(), tt.wantTargetNS)
+			if tt.wantTargetNS == "" {
+				if got != nil {
+					t.Errorf("GetRepoByCR() got = '%v', want nil", got.GetNamespace())
+				}
+			} else {
+				if got == nil {
+					t.Errorf("GetRepoByCR() want non-nil got nil")
+				} else if tt.wantTargetNS != got.GetNamespace() {
+					t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.GetNamespace(), tt.wantTargetNS)
+				}
 			}
 		})
 	}

pkg/pipelineascode/pipelineascode.go

@@ -280,7 +280,11 @@ func (p *PacRun) startPR(ctx context.Context, match matcher.Match) (*tektonv1.Pi
 		if err != nil {
 			// we still return the created PR with error, and allow caller to decide what to do with the PR, and avoid
 			// unneeded SIGSEGV's
-			return pr, fmt.Errorf("cannot patch pipelinerun %s: %w", pr.GetGenerateName(), err)
+			prName := "unknown"
+			if pr != nil {
+				prName = pr.GetGenerateName()
+			}
+			return pr, fmt.Errorf("cannot patch pipelinerun %s: %w", prName, err)
 		}
 		currentReason := ""
 		if len(pr.Status.GetConditions()) > 0 {

pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go

@@ -273,7 +273,10 @@ func TestGetFileInsideRepo(t *testing.T) {
 			bbtest.MuxFiles(t, mux, tt.event, tt.targetbranch, filepath.Dir(tt.path), tt.filescontents, tt.wantErr != "")
 			fc, err := v.GetFileInsideRepo(ctx, tt.event, tt.path, tt.targetbranch)
 			if tt.wantErr != "" {
-				assert.Equal(t, err.Error(), tt.wantErr)
+				assert.Assert(t, err != nil, "expected an error but got nil")
+				if err != nil {
+					assert.Equal(t, err.Error(), tt.wantErr)
+				}
 				return
 			}
 			assert.NilError(t, err)
@@ -721,7 +724,10 @@ func TestGetFiles(t *testing.T) {
 			v := &Provider{client: client, baseURL: tURL}
 			changedFiles, err := v.GetFiles(ctx, tt.event)
 			if tt.wantError {
-				assert.Equal(t, err.Error(), tt.errMsg)
+				assert.Assert(t, err != nil, "expected an error but got nil")
+				if err != nil {
+					assert.Equal(t, err.Error(), tt.errMsg)
+				}
 				return
 			}
 			assert.NilError(t, err, nil)

pkg/provider/gitea/acl.go

@@ -19,14 +19,14 @@ func (v *Provider) CheckPolicyAllowing(_ context.Context, event *info.Event, all
 	}
 	// TODO: caching
 	orgTeams, resp, err := v.Client().ListOrgTeams(event.Organization, gitea.ListTeamsOptions{})
-	if resp.StatusCode == http.StatusNotFound {
-		// we explicitly disallow the policy when there is no team on org
-		return false, fmt.Sprintf("no teams on org %s", event.Organization)
-	}
 	if err != nil {
 		// probably a 500 or another api error, no need to try again and again with other teams
 		return false, fmt.Sprintf("error while getting org team, error: %s", err.Error())
 	}
+	if resp.StatusCode == http.StatusNotFound {
+		// we explicitly disallow the policy when there is no team on org
+		return false, fmt.Sprintf("no teams on org %s", event.Organization)
+	}
 	for _, allowedTeam := range allowedTeams {
 		for _, orgTeam := range orgTeams {
 			if orgTeam.Name == allowedTeam {

pkg/provider/gitea/acl_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"strings"
 	"testing"
 
 	giteaStructs "code.gitea.io/gitea/modules/structs"
@@ -17,6 +18,7 @@ import (
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
 	"gotest.tools/v3/assert"
+	"gotest.tools/v3/golden"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
 
@@ -93,7 +95,7 @@ func TestCheckPolicyAllowing(t *testing.T) {
 
 			gotAllowed, gotReason := gprovider.CheckPolicyAllowing(ctx, event, tt.allowedTeams)
 			assert.Equal(t, tt.wantAllowed, gotAllowed)
-			assert.Equal(t, tt.wantReason, gotReason)
+			golden.Assert(t, gotReason, strings.ReplaceAll(fmt.Sprintf("%s.golden", t.Name()), "/", "-"))
 		})
 	}
 }

pkg/provider/gitea/gitea_test.go

@@ -258,6 +258,11 @@ func TestProvider_GetFiles(t *testing.T) {
 				t.Errorf("Provider.GetFiles() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
+
+			if tt.wantErr {
+				return
+			}
+
 			sort.Strings(got.All)
 			sort.Strings(tt.want.All)
 

pkg/provider/gitea/testdata/TestCheckPolicyAllowing-error_while_getting_team_membership.golden

@@ -0,0 +1 @@
+error while getting org team, error: invalid character 't' in literal true (expecting 'r')

pkg/provider/gitea/testdata/TestCheckPolicyAllowing-no_team_in_org_is_found.golden

@@ -0,0 +1,3 @@
+error while getting org team, error: Unknown API Error: 404
+Request: '/api/v1/orgs/myorg/teams' with 'GET' method and '404 page not found
+' body

pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_a_member_of_the_allowed_team.golden

@@ -0,0 +1 @@
+allowing user: allowedUser as a member of the team: allowedTeam

pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_not_a_member_of_the_allowed_team.golden

@@ -0,0 +1 @@
+user: allowedUser is not a member of any of the allowed teams: [otherteam]

pkg/provider/github/acl.go

@@ -330,6 +330,9 @@ func (v *Provider) GetStringPullRequestComment(ctx context.Context, runevent *in
 		if err != nil {
 			return nil, err
 		}
+		if resp == nil {
+			return nil, fmt.Errorf("unexpected nil response from ListComments")
+		}
 		for _, v := range comments {
 			if acl.MatchRegexp(reg, v.GetBody()) {
 				ret = append(ret, v)

pkg/provider/github/repository_test.go

@@ -154,7 +154,10 @@ func TestConfigureRepository(t *testing.T) {
 			assert.Equal(t, configuring, tt.configuring)
 
 			if tt.wantErr != "" {
-				assert.Equal(t, tt.wantErr, err.Error())
+				assert.Assert(t, err != nil, "expected an error but got nil")
+				if err != nil {
+					assert.Equal(t, tt.wantErr, err.Error())
+				}
 			} else {
 				assert.NilError(t, err)
 			}

pkg/provider/gitlab/acl.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"net/http"
+	"strings"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/acl"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
@@ -19,8 +20,12 @@ func (v *Provider) IsAllowedOwnersFile(_ context.Context, event *info.Event) (bo
 	}
 	// OWNERS_ALIASES file existence is not required, if we get "not found" continue
 	ownerAliasesContent, resp, err := v.getObject("OWNERS_ALIASES", event.DefaultBranch, v.targetProjectID)
-	if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNotFound {
-		return false, err
+	if err != nil {
+		if !strings.Contains(err.Error(), "not found") {
+			return false, err
+		}
+	} else if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusNotFound {
+		return false, fmt.Errorf("unexpected status code %d from getObject for OWNERS_ALIASES", resp.StatusCode)
 	}
 	allowed, _ := acl.UserInOwnerFile(string(ownerContent), string(ownerAliasesContent), event.Sender)
 	return allowed, nil

pkg/provider/gitlab/acl_test.go

@@ -109,6 +109,7 @@ func TestIsAllowed(t *testing.T) {
 				}
 				if tt.ownerFile != "" {
 					thelp.MuxGetFile(mux, tt.fields.targetProjectID, "OWNERS", tt.ownerFile, false)
+					thelp.MuxGetFile(mux, tt.fields.targetProjectID, "OWNERS_ALIASES", tt.ownerFile, false)
 				}
 				if tt.commentContent != "" {
 					thelp.MuxDiscussionsNote(mux, tt.fields.targetProjectID,

test/bitbucket_datacenter_pull_request_test.go

@@ -105,7 +105,13 @@ func TestBitbucketDataCenterOnPathChangeAnnotationOnPRMerge(t *testing.T) {
 	defer tbbdc.TearDownNs(ctx, t, runcnx, targetNS)
 
 	branch, resp, err := client.Git.CreateRef(ctx, bitbucketWSOwner, tempBaseBranch, repo.Branch)
-	assert.NilError(t, err, "error creating branch: http status code: %d : %v", resp.Status, err)
+	msg := "error creating branch"
+	if resp != nil {
+		msg = fmt.Sprintf("error creating branch: http status code: %d : %v", resp.Status, err)
+	} else if err != nil {
+		msg = fmt.Sprintf("error creating branch: %v", err)
+	}
+	assert.NilError(t, err, msg)
 	runcnx.Clients.Log.Infof("Base branch %s has been created", branch.Name)
 
 	opts.BaseBranch = branch.Name

test/bitbucket_datacenter_push_test.go

@@ -37,7 +37,13 @@ func TestBitbucketDataCenterCELPathChangeOnPush(t *testing.T) {
 
 	mainBranchRef := "refs/heads/main"
 	branch, resp, err := client.Git.CreateRef(ctx, bitbucketWSOwner, targetNS, mainBranchRef)
-	assert.NilError(t, err, "error creating branch: http status code: %d : %v", resp.Status, err)
+	msg := "error creating branch"
+	if resp != nil {
+		msg = fmt.Sprintf("error creating branch: http status code: %d : %v", resp.Status, err)
+	} else if err != nil {
+		msg = fmt.Sprintf("error creating branch: %v", err)
+	}
+	assert.NilError(t, err, msg)
 	runcnx.Clients.Log.Infof("Branch %s has been created", branch.Name)
 	defer tbbs.TearDown(ctx, t, runcnx, client, nil, bitbucketWSOwner, branch.Name)
 

test/github_config_maxkeepruns_test.go

@@ -9,12 +9,10 @@ import (
 	"time"
 
 	ghlib "github.com/google/go-github/v71/github"
-	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github"
 	twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
 	"gotest.tools/v3/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"knative.dev/pkg/apis"
 )
 
 func TestGithubMaxKeepRuns(t *testing.T) {
@@ -45,17 +43,13 @@ func TestGithubMaxKeepRuns(t *testing.T) {
 	count := 0
 	for {
 		prs, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{})
-		if err == nil && len(prs.Items) == 1 {
-			if prs.Items[0].GetStatusCondition().GetCondition(apis.ConditionSucceeded).GetReason() == "Running" {
-				t.Logf("skipping %s since currently running", prs.Items[0].GetName())
-				continue
-			}
-			// making sure secret is not deleted for existing pipelinerun
-			if secretName, ok := prs.Items[0].GetAnnotations()[keys.GitAuthSecret]; ok {
-				sData, err := g.Cnx.Clients.Kube.CoreV1().Secrets(g.TargetNamespace).Get(ctx, secretName, metav1.GetOptions{})
-				assert.NilError(t, err, "Secret should not have been deleted while running pipelinerun")
-				assert.Assert(t, sData.Name != "")
-			}
+		assert.NilError(t, err)
+
+		if prs == nil {
+			t.Fatalf("PipelineRun list is nil in %s", g.TargetNamespace)
+		}
+
+		if len(prs.Items) == 0 {
 			break
 		}
 		time.Sleep(10 * time.Second)

test/pkg/bitbucketdatacenter/crd.go

@@ -2,6 +2,7 @@ package bitbucketdatacenter
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"strings"
 	"testing"
@@ -18,7 +19,13 @@ import (
 
 func CreateCRD(ctx context.Context, t *testing.T, client *scm.Client, run *params.Run, orgAndRepo, targetNS string) *scm.Repository {
 	repo, resp, err := client.Repositories.Find(ctx, orgAndRepo)
-	assert.NilError(t, err, "error getting repository: http status code: %d: %v", resp.Status, err)
+	msg := "error getting repository"
+	if resp != nil {
+		msg = fmt.Sprintf("error getting repository: http status code: %d: %v", resp.Status, err)
+	} else if err != nil {
+		msg = fmt.Sprintf("error getting repository: %v", err)
+	}
+	assert.NilError(t, err, msg)
 
 	url := strings.ReplaceAll(repo.Link, "/browse", "")
 	repository := &v1alpha1.Repository{