From 597770a12e062053bd8f9dff90899cd4e78dffa2 Mon Sep 17 00:00:00 2001 From: Chmouel Boudjnah Date: Wed, 6 Aug 2025 10:27:55 +0200 Subject: [PATCH] fix: Make error handling and tests more robust - Added nil checks to prevent panics from dereferencing null pointers - Improved error handling logic and messages in various components - Enhanced test suite robustness by adding nil checks to assertions - Migrated Gitea ACL tests to use golden files for improved maintainability - Adjusted golden file update script to target specific packages Signed-off-by: Chmouel Boudjnah --- hack/update-golden.sh | 3 ++- pkg/adapter/adapter.go | 6 ++++- pkg/cli/webhook/bitbucket_cloud_test.go | 5 +++- pkg/cli/webhook/github_test.go | 5 +++- pkg/cli/webhook/gitlab_test.go | 5 +++- pkg/cmd/tknpac/bootstrap/bootstrap_test.go | 4 ++- pkg/matcher/annotation_matcher_test.go | 2 +- pkg/matcher/repo_runinfo_matcher.go | 2 +- pkg/matcher/repo_runinfo_matcher_test.go | 26 ++++++++++++------- pkg/pipelineascode/pipelineascode.go | 6 ++++- .../bitbucketdatacenter_test.go | 10 +++++-- pkg/provider/gitea/acl.go | 8 +++--- pkg/provider/gitea/acl_test.go | 4 ++- pkg/provider/gitea/gitea_test.go | 5 ++++ ...error_while_getting_team_membership.golden | 1 + ...icyAllowing-no_team_in_org_is_found.golden | 3 +++ ...ser_is_a_member_of_the_allowed_team.golden | 1 + ...is_not_a_member_of_the_allowed_team.golden | 1 + pkg/provider/github/acl.go | 3 +++ pkg/provider/github/repository_test.go | 5 +++- pkg/provider/gitlab/acl.go | 9 +++++-- pkg/provider/gitlab/acl_test.go | 1 + .../bitbucket_datacenter_pull_request_test.go | 8 +++++- test/bitbucket_datacenter_push_test.go | 8 +++++- test/github_config_maxkeepruns_test.go | 20 +++++--------- test/pkg/bitbucketdatacenter/crd.go | 9 ++++++- test/pkg/bitbucketdatacenter/pr.go | 16 ++++++++++-- test/pkg/gitea/scm.go | 9 +++++-- test/pkg/github/setup.go | 19 ++++++++------ 29 files changed, 147 insertions(+), 57 deletions(-) create mode 100644 pkg/provider/gitea/testdata/TestCheckPolicyAllowing-error_while_getting_team_membership.golden create mode 100644 pkg/provider/gitea/testdata/TestCheckPolicyAllowing-no_team_in_org_is_found.golden create mode 100644 pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_a_member_of_the_allowed_team.golden create mode 100644 pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_not_a_member_of_the_allowed_team.golden diff --git a/hack/update-golden.sh b/hack/update-golden.sh index 96cf48212..f34959c1a 100755 --- a/hack/update-golden.sh +++ b/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 diff --git a/pkg/adapter/adapter.go b/pkg/adapter/adapter.go index d1a404339..c6ea16231 100644 --- a/pkg/adapter/adapter.go +++ b/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{ diff --git a/pkg/cli/webhook/bitbucket_cloud_test.go b/pkg/cli/webhook/bitbucket_cloud_test.go index 6c3228ed2..65cab421d 100644 --- a/pkg/cli/webhook/bitbucket_cloud_test.go +++ b/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) diff --git a/pkg/cli/webhook/github_test.go b/pkg/cli/webhook/github_test.go index 2485be6e0..afce591e5 100644 --- a/pkg/cli/webhook/github_test.go +++ b/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) diff --git a/pkg/cli/webhook/gitlab_test.go b/pkg/cli/webhook/gitlab_test.go index 7e290e90c..7c8fde5cf 100644 --- a/pkg/cli/webhook/gitlab_test.go +++ b/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) diff --git a/pkg/cmd/tknpac/bootstrap/bootstrap_test.go b/pkg/cmd/tknpac/bootstrap/bootstrap_test.go index a87431fab..f6460728d 100644 --- a/pkg/cmd/tknpac/bootstrap/bootstrap_test.go +++ b/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) diff --git a/pkg/matcher/annotation_matcher_test.go b/pkg/matcher/annotation_matcher_test.go index 0a2c4592b..5923d70b0 100644 --- a/pkg/matcher/annotation_matcher_test.go +++ b/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) diff --git a/pkg/matcher/repo_runinfo_matcher.go b/pkg/matcher/repo_runinfo_matcher.go index 40e099358..a0ab439b6 100644 --- a/pkg/matcher/repo_runinfo_matcher.go +++ b/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 { diff --git a/pkg/matcher/repo_runinfo_matcher_test.go b/pkg/matcher/repo_runinfo_matcher_test.go index 6a89f6a0f..c0b41ba25 100644 --- a/pkg/matcher/repo_runinfo_matcher_test.go +++ b/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) + } } }) } diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go index c0384b9f7..fadadb365 100644 --- a/pkg/pipelineascode/pipelineascode.go +++ b/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 { diff --git a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go b/pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go index 928388d5c..ce49dd55d 100644 --- a/pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go +++ b/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) diff --git a/pkg/provider/gitea/acl.go b/pkg/provider/gitea/acl.go index c7b04ea52..317f4a927 100644 --- a/pkg/provider/gitea/acl.go +++ b/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 { diff --git a/pkg/provider/gitea/acl_test.go b/pkg/provider/gitea/acl_test.go index bc6fa87cc..df76efadd 100644 --- a/pkg/provider/gitea/acl_test.go +++ b/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()), "/", "-")) }) } } diff --git a/pkg/provider/gitea/gitea_test.go b/pkg/provider/gitea/gitea_test.go index 281755781..907f46be1 100644 --- a/pkg/provider/gitea/gitea_test.go +++ b/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) diff --git a/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-error_while_getting_team_membership.golden b/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-error_while_getting_team_membership.golden new file mode 100644 index 000000000..93dc23f93 --- /dev/null +++ b/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') \ No newline at end of file diff --git a/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-no_team_in_org_is_found.golden b/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-no_team_in_org_is_found.golden new file mode 100644 index 000000000..d92ed774f --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_a_member_of_the_allowed_team.golden b/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_a_member_of_the_allowed_team.golden new file mode 100644 index 000000000..c87580ea2 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_not_a_member_of_the_allowed_team.golden b/pkg/provider/gitea/testdata/TestCheckPolicyAllowing-user_is_not_a_member_of_the_allowed_team.golden new file mode 100644 index 000000000..54a12b8bf --- /dev/null +++ b/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] \ No newline at end of file diff --git a/pkg/provider/github/acl.go b/pkg/provider/github/acl.go index e6f144a75..552f0cd6c 100644 --- a/pkg/provider/github/acl.go +++ b/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) diff --git a/pkg/provider/github/repository_test.go b/pkg/provider/github/repository_test.go index eca3b7ba5..a4f7a01a5 100644 --- a/pkg/provider/github/repository_test.go +++ b/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) } diff --git a/pkg/provider/gitlab/acl.go b/pkg/provider/gitlab/acl.go index da70ce5bb..936065251 100644 --- a/pkg/provider/gitlab/acl.go +++ b/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 diff --git a/pkg/provider/gitlab/acl_test.go b/pkg/provider/gitlab/acl_test.go index 77aa98fe0..f17c0eff9 100644 --- a/pkg/provider/gitlab/acl_test.go +++ b/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, diff --git a/test/bitbucket_datacenter_pull_request_test.go b/test/bitbucket_datacenter_pull_request_test.go index 0afa3b206..e3c7c9c6d 100644 --- a/test/bitbucket_datacenter_pull_request_test.go +++ b/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 diff --git a/test/bitbucket_datacenter_push_test.go b/test/bitbucket_datacenter_push_test.go index 36d95f640..1bc666f11 100644 --- a/test/bitbucket_datacenter_push_test.go +++ b/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) diff --git a/test/github_config_maxkeepruns_test.go b/test/github_config_maxkeepruns_test.go index 423812996..53cacdcb9 100644 --- a/test/github_config_maxkeepruns_test.go +++ b/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) diff --git a/test/pkg/bitbucketdatacenter/crd.go b/test/pkg/bitbucketdatacenter/crd.go index eef3f44a2..1376408ba 100644 --- a/test/pkg/bitbucketdatacenter/crd.go +++ b/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{ diff --git a/test/pkg/bitbucketdatacenter/pr.go b/test/pkg/bitbucketdatacenter/pr.go index c87615c62..42220e065 100644 --- a/test/pkg/bitbucketdatacenter/pr.go +++ b/test/pkg/bitbucketdatacenter/pr.go @@ -20,7 +20,13 @@ func CreatePR(ctx context.Context, t *testing.T, client *goscm.Client, runcnx *p } branch, resp, err := client.Git.CreateRef(ctx, orgAndRepo, targetNS, baseBranchRef) - 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) gitCloneURL, err := scm.MakeGitCloneURL(repo.Clone, opts.UserName, opts.Password) @@ -45,7 +51,13 @@ func CreatePR(ctx context.Context, t *testing.T, client *goscm.Client, runcnx *p Base: baseBranchRef, } pr, resp, err := client.PullRequests.Create(ctx, orgAndRepo, prOpts) - assert.NilError(t, err, "error creating pull request: http status code: %d : %v", resp.Status, err) + msg = "error creating pull request" + if resp != nil { + msg = fmt.Sprintf("error creating pull request: http status code: %d : %v", resp.Status, err) + } else if err != nil { + msg = fmt.Sprintf("error creating pull request: %v", err) + } + assert.NilError(t, err, msg) runcnx.Clients.Log.Infof("Created Pull Request with Title '%s'. Head branch '%s' ⮕ Base Branch '%s'", pr.Title, pr.Head.Ref, pr.Base.Ref) return pr } diff --git a/test/pkg/gitea/scm.go b/test/pkg/gitea/scm.go index 91720874e..9663a8eee 100644 --- a/test/pkg/gitea/scm.go +++ b/test/pkg/gitea/scm.go @@ -63,8 +63,8 @@ func PushFilesToRefAPI(t *testing.T, topts *TestOpts, entries map[string]string) }, } fr, _, err := topts.GiteaCNX.Client().CreateFile(topts.Opts.Organization, topts.Opts.Repo, filename, fOpts) - sha = fr.Commit.SHA assert.NilError(t, err) + sha = fr.Commit.SHA } return sha, nil } @@ -173,7 +173,12 @@ func CreateTeam(topts *TestOpts, orgName, teamName string) (*gitea.Team, error) }, Name: teamName, }) - topts.ParamsRun.Clients.Log.Infof("Team %s has been created on Org %s", team.Name, orgName) + if err != nil { + return nil, err + } + if team != nil { + topts.ParamsRun.Clients.Log.Infof("Team %s has been created on Org %s", team.Name, orgName) + } return team, err } diff --git a/test/pkg/github/setup.go b/test/pkg/github/setup.go index 45d059791..aa58e54cd 100644 --- a/test/pkg/github/setup.go +++ b/test/pkg/github/setup.go @@ -44,20 +44,23 @@ func Setup(ctx context.Context, onSecondController, viaDirectWebhook bool) (cont } } - var split []string + var repoOwner string if !viaDirectWebhook { - split = strings.Split(githubRepoOwnerGithubApp, "/") - } - if viaDirectWebhook { - githubToken = os.Getenv("TEST_GITHUB_TOKEN") - split = strings.Split(githubRepoOwnerDirectWebhook, "/") + repoOwner = githubRepoOwnerGithubApp + } else { + repoOwner = githubRepoOwnerDirectWebhook } + if onSecondController { githubURL = os.Getenv("TEST_GITHUB_SECOND_API_URL") - githubRepoOwnerGithubApp = os.Getenv("TEST_GITHUB_SECOND_REPO_OWNER_GITHUBAPP") + repoOwner = os.Getenv("TEST_GITHUB_SECOND_REPO_OWNER_GITHUBAPP") githubToken = os.Getenv("TEST_GITHUB_SECOND_TOKEN") controllerURL = os.Getenv("TEST_GITHUB_SECOND_EL_URL") - split = strings.Split(githubRepoOwnerGithubApp, "/") + } + + split := strings.Split(repoOwner, "/") + if len(split) != 2 { + return ctx, nil, options.E2E{}, github.New(), fmt.Errorf("repository owner/repo env var is not in the format owner/repo: %s", repoOwner) } run := params.New()