Skip to content

Commit 2c02813

Browse files
committed
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 <[email protected]>
1 parent 78a7418 commit 2c02813

33 files changed

+159
-61
lines changed

hack/update-golden.sh

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

pkg/adapter/adapter.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -182,10 +182,14 @@ func (l listener) handleEvent(ctx context.Context) http.HandlerFunc {
182182
}
183183

184184
// figure out which provider request coming from
185-
if err != nil || gitProvider == nil {
185+
if err != nil {
186186
l.writeResponse(response, http.StatusOK, err.Error())
187187
return
188188
}
189+
if gitProvider == nil {
190+
l.writeResponse(response, http.StatusOK, "gitProvider is nil")
191+
return
192+
}
189193
gitProvider.SetPacInfo(&pacInfo)
190194

191195
s := sinker{

pkg/cli/webhook/bitbucket_cloud_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,10 @@ func TestAskBBWebhookConfig(t *testing.T) {
7878
bb := bitbucketCloudConfig{IOStream: io}
7979
err := bb.askBBWebhookConfig(tt.repoURL, tt.controllerURL, "", tt.personalaccesstoken)
8080
if tt.wantErrStr != "" {
81-
assert.Equal(t, err.Error(), tt.wantErrStr)
81+
assert.Assert(t, err != nil, "expected an error but got nil")
82+
if err != nil {
83+
assert.Equal(t, err.Error(), tt.wantErrStr)
84+
}
8285
return
8386
}
8487
assert.NilError(t, err)

pkg/cli/webhook/github_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,10 @@ func TestAskGHWebhookConfig(t *testing.T) {
8686
gh := gitHubConfig{IOStream: io}
8787
err := gh.askGHWebhookConfig(tt.repoURL, tt.controllerURL, "", tt.personalaccesstoken)
8888
if tt.wantErrStr != "" {
89-
assert.Equal(t, err.Error(), tt.wantErrStr)
89+
assert.Assert(t, err != nil, "expected an error but got nil")
90+
if err != nil {
91+
assert.Equal(t, err.Error(), tt.wantErrStr)
92+
}
9093
return
9194
}
9295
assert.NilError(t, err)

pkg/cli/webhook/gitlab_test.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,10 @@ func TestAskGLWebhookConfig(t *testing.T) {
7575
gl := gitLabConfig{IOStream: io}
7676
err := gl.askGLWebhookConfig(tt.repoURL, tt.controllerURL, tt.providerURL, tt.personalaccesstoken)
7777
if tt.wantErrStr != "" {
78-
assert.Equal(t, err.Error(), tt.wantErrStr)
78+
assert.Assert(t, err != nil, "expected an error but got nil")
79+
if err != nil {
80+
assert.Equal(t, err.Error(), tt.wantErrStr)
81+
}
7982
return
8083
}
8184
assert.NilError(t, err)

pkg/cmd/tknpac/bootstrap/bootstrap_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,9 @@ func TestGetDashboardURL(t *testing.T) {
320320
err := getDashboardURL(ctx, opts, run)
321321
if tt.wantError {
322322
assert.Assert(t, err != nil)
323-
assert.Assert(t, strings.Contains(err.Error(), tt.errorMsg))
323+
if err != nil {
324+
assert.Assert(t, strings.Contains(err.Error(), tt.errorMsg))
325+
}
324326
} else {
325327
assert.NilError(t, err)
326328
assert.Equal(t, tt.wantURL, opts.dashboardURL)

pkg/matcher/annotation_matcher_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2546,7 +2546,7 @@ func TestGetTargetBranch(t *testing.T) {
25462546
matched, targetEvent, targetBranch, err := getTargetBranch(tt.prun, tt.event)
25472547
if tt.expectedError != "" {
25482548
assert.Assert(t, err != nil)
2549-
assert.Error(t, err, tt.expectedError, err.Error())
2549+
assert.Error(t, err, tt.expectedError)
25502550
return
25512551
}
25522552
assert.NilError(t, err)

pkg/matcher/repo_runinfo_matcher.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,10 @@ import (
1414
func MatchEventURLRepo(ctx context.Context, cs *params.Run, event *info.Event, ns string) (*apipac.Repository, error) {
1515
repositories, err := cs.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(ns).List(
1616
ctx, metav1.ListOptions{})
17-
sort.RepositorySortByCreationOldestTime(repositories.Items)
1817
if err != nil {
1918
return nil, err
2019
}
20+
sort.RepositorySortByCreationOldestTime(repositories.Items)
2121
for _, repo := range repositories.Items {
2222
repo.Spec.URL = strings.TrimSuffix(repo.Spec.URL, "/")
2323
if repo.Spec.URL == event.URL {

pkg/matcher/repo_runinfo_matcher_test.go

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository"
1414
"go.uber.org/zap"
1515
zapobserver "go.uber.org/zap/zaptest/observer"
16-
"gotest.tools/v3/assert"
1716
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1817
rtesting "knative.dev/pkg/reconciler/testing"
1918
)
@@ -181,18 +180,25 @@ func Test_getRepoByCR(t *testing.T) {
181180
}
182181
got, err := MatchEventURLRepo(ctx, client, &tt.args.runevent, "")
183182

184-
if err == nil && tt.wantErr {
185-
assert.NilError(t, err, "GetRepoByCR() error = %v, wantErr %v", err, tt.wantErr)
183+
if (err != nil) != tt.wantErr {
184+
t.Errorf("MatchEventURLRepo() error = %v, wantErr %v", err, tt.wantErr)
185+
return
186186
}
187-
if tt.wantTargetNS == "" && got != nil {
188-
t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.GetNamespace(), tt.wantTargetNS)
189-
}
190-
if tt.wantTargetNS != "" && got == nil {
191-
t.Errorf("GetRepoByCR() want nil got '%v'", tt.wantTargetNS)
187+
188+
if tt.wantErr {
189+
return
192190
}
193191

194-
if tt.wantTargetNS != "" && tt.wantTargetNS != got.GetNamespace() {
195-
t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.GetNamespace(), tt.wantTargetNS)
192+
if tt.wantTargetNS == "" {
193+
if got != nil {
194+
t.Errorf("GetRepoByCR() got = '%v', want nil", got.GetNamespace())
195+
}
196+
} else {
197+
if got == nil {
198+
t.Errorf("GetRepoByCR() want non-nil got nil")
199+
} else if tt.wantTargetNS != got.GetNamespace() {
200+
t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.GetNamespace(), tt.wantTargetNS)
201+
}
196202
}
197203
})
198204
}

pkg/pipelineascode/errors_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,9 @@ func TestCheckAccessOrErrror(t *testing.T) {
101101
if tt.expectedErr {
102102
assert.Assert(t, err != nil, "Expected error but got nil")
103103
if tt.expectedErrMsg != "" {
104-
assert.Assert(t, err.Error() != "", "Expected error message but got empty string")
104+
if err != nil { // Add this check
105+
assert.Assert(t, err.Error() != "", "Expected error message but got empty string")
106+
}
105107
assert.ErrorContains(t, err, tt.expectedErrMsg)
106108
}
107109
} else {

0 commit comments

Comments
 (0)