tests: enforce conventions for go Test

Enforce our convention on test to use table driven tests and better
naming across the repository.

Add s imple check in linters.yaml to enforce this.
Update AGENTS.md so agents can respect it.

Co-Authored-By: Claude <[email protected]>
Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: 2 people

.tekton/linter.yaml

@@ -169,8 +169,60 @@ spec:
               image: docker.io/golang:1.26
               workingDir: $(workspaces.source.path)
               script: |
+                git config --global --add safe.directory $(workspaces.source.path)
                 make lint-e2e-naming
 
+            - name: check-test-conventions
+              displayName: "Check Go test naming and assertion conventions"
+              image: docker.io/golang:1.26
+              workingDir: $(workspaces.source.path)
+              script: |
+                #!/usr/bin/env bash
+                set -eu
+                git config --global --add safe.directory $(workspaces.source.path)
+
+                if [[ "{{ headers['X-Github-Event'] }}" != "pull_request" ]]; then
+                  echo "Not a pull request, skipping test convention check"
+                  exit 0
+                fi
+
+                base_sha="{{ body.pull_request.base.sha }}"
+                if [[ -z "$base_sha" ]]; then
+                  echo "Base SHA is empty, skipping"
+                  exit 0
+                fi
+
+                files=$(git diff --name-only "${base_sha}..HEAD" -- '*.go' | grep -v '^vendor/' | grep '_test\.go$' || true)
+                if [ -z "$files" ]; then
+                  echo "No test files modified."
+                  exit 0
+                fi
+
+                failed=0
+                while IFS= read -r file; do
+                  [ -z "$file" ] && continue
+                  [ ! -f "$file" ] && continue
+
+                  bad_names=$(grep -n '^func Test[A-Za-z]*_[A-Za-z]' "$file" || true)
+                  if [ -n "$bad_names" ]; then
+                    echo "ERROR: $file: test function names must use PascalCase (no underscores):"
+                    echo "$bad_names"
+                    failed=1
+                  fi
+
+                  bad_asserts=$(grep -En '\bt\.(Errorf|Fatalf|Fatal|Error)\(' "$file" || true)
+                  if [ -n "$bad_asserts" ]; then
+                    echo "WARNING: $file: use gotest.tools/v3/assert instead of t.Errorf/t.Fatalf/t.Fatal/t.Error:"
+                    echo "$bad_asserts"
+                  fi
+                done <<< "$files"
+
+                if [ "$failed" -ne 0 ]; then
+                  echo "Fix violations above. See AGENTS.md for testing conventions."
+                  exit 1
+                fi
+                echo "All modified test files pass convention checks."
+
             - name: check-generated-schemas
               displayName: "Check generated OpenAPI schemas"
               image: docker.io/golang:1.26

AGENTS.md

@@ -28,10 +28,55 @@
 
 ## Testing
 
-- **Unit tests**: Use `gotest.tools/v3` (never testify)
+### Test Structure
+
+- **Use table-driven tests**: Anonymous struct pattern with `tests := []struct{...}{...}`
+- **No underscores in test names**: Use PascalCase like `TestGetTektonDir`, not `TestGetTektonDir_Something`
+- **Subtest naming**: Use descriptive `name` field for `t.Run(tt.name, ...)`
+- **Complex setup**: Add `setup func(t *testing.T, ...)` field to test struct instead of creating separate test functions
+- **Example**:
+
+  ```go
+  func TestGetTektonDir(t *testing.T) {
+      tests := []struct {
+          name  string
+          event *info.Event
+          setup func(t *testing.T, mux *http.ServeMux)
+      }{
+          {
+              name: "simple case",
+              event: &info.Event{...},
+              setup: func(t *testing.T, mux *http.ServeMux) {
+                  t.Helper()
+                  mux.HandleFunc("/repos/...", ...)
+              },
+          },
+      }
+      for _, tt := range tests {
+          t.Run(tt.name, func(t *testing.T) { ... })
+      }
+  }
+  ```
+
+### Assertions
+
+- **Use `gotest.tools/v3/assert`** (never testify or pkg/assert)
+  - `assert.NilError(t, err)` - verify no error
+  - `assert.Assert(t, condition, msg)` - general assertions
+  - `assert.Equal(t, expected, actual)` - equality checks
+  - `assert.ErrorContains(t, err, substring)` - error validation
+
+### Test Types
+
+- **Unit tests**: Fast, focused, use mocks from `pkg/test/github`
 - **E2E tests**: Require specific setup - always ask user to run and copy output
 - **Gitea tests**: Most comprehensive and self-contained
-- **Commands**: `make test-no-cache`, `make html-coverage`
+
+### Commands
+
+- `make test` - Run test suite
+- `make test-no-cache` - Run without cache
+- `make html-coverage` - Generate coverage report
 
 ## Dependencies
 

pkg/acl/owners_test.go

@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"golang.org/x/exp/slices"
+	"gotest.tools/v3/assert"
 )
 
 func TestUserInOwnerFile(t *testing.T) {
@@ -127,13 +128,12 @@ func TestUserInOwnerFile(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			got, err := UserInOwnerFile(tt.args.ownersContent, tt.args.ownersAliasesContent, tt.args.sender)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("UserInOwnerFile() error = %v, wantErr %v", err, tt.wantErr)
+			if tt.wantErr {
+				assert.Assert(t, err != nil)
 				return
 			}
-			if got != tt.want {
-				t.Errorf("UserInOwnerFile() = %v, want %v", got, tt.want)
-			}
+			assert.NilError(t, err)
+			assert.Equal(t, got, tt.want)
 		})
 	}
 }
@@ -195,13 +195,9 @@ func TestExpandAliases(t *testing.T) {
 			// Can't use reflect.DeepEqual to compare the slices as expandAliases
 			// uses a map for dedup which does not preserve the order.
 			// We do not care about the order, just the content of the slice.
-			if len(got) != len(tt.want) {
-				t.Errorf("expandAliases() got = %v, want %v", got, tt.want)
-			}
+			assert.Equal(t, len(got), len(tt.want))
 			for _, v := range got {
-				if !slices.Contains(tt.want, v) {
-					t.Errorf("expandAliases() got = %v, want %v", got, tt.want)
-				}
+				assert.Assert(t, slices.Contains(tt.want, v), "got = %v, want %v", got, tt.want)
 			}
 		})
 	}

pkg/acl/regexp_test.go

@@ -100,9 +100,8 @@ func TestMatchRegexp(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := MatchRegexp(tt.args.reg, tt.args.comment); got != tt.want {
-				t.Errorf("MatchRegexp() = %v, want %v", got, tt.want)
-			}
+			got := MatchRegexp(tt.args.reg, tt.args.comment)
+			assert.Equal(t, got, tt.want)
 		})
 	}
 }

pkg/adapter/adapter_test.go

@@ -190,23 +190,17 @@ func TestHandleEvent(t *testing.T) {
 			defer ts.Close()
 
 			req, err := http.NewRequestWithContext(context.Background(), tn.requestType, ts.URL, bytes.NewReader(tn.event))
-			if err != nil {
-				t.Fatalf("error creating request: %s", err)
-			}
+			assert.NilError(t, err)
 			req.Header.Set("X-Github-Event", tn.eventType)
 
 			resp, err := http.DefaultClient.Do(req)
-			if err != nil {
-				t.Fatalf("error sending request: %s", err)
-			}
+			assert.NilError(t, err)
 			defer resp.Body.Close()
 
 			if tn.wantLogSnippet != "" {
 				assert.Assert(t, logCatcher.FilterMessageSnippet(tn.wantLogSnippet).Len() > 0, logCatcher.All())
 			}
-			if resp.StatusCode != tn.statusCode {
-				t.Fatalf("expected status code : %v but got %v ", tn.statusCode, resp.StatusCode)
-			}
+			assert.Equal(t, resp.StatusCode, tn.statusCode)
 		})
 	}
 }

pkg/adapter/incoming_test.go

@@ -50,7 +50,7 @@ Q1QWaigUQdpFfNCrqwJBANLgWaJV722PhQXOCmR+INvZ7ksIhJVcq/x1l2BYOLw2
 QsncVExbMiPa9Oclo5qLuTosS8qwHm1MJEytp3/SkB8=
 -----END RSA PRIVATE KEY-----`
 
-func Test_compareSecret(t *testing.T) {
+func TestCompareSecret(t *testing.T) {
 	type args struct {
 		incomingSecret string
 		secretValue    string
@@ -79,14 +79,12 @@ func Test_compareSecret(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := compareSecret(tt.args.incomingSecret, tt.args.secretValue); got != tt.want {
-				t.Errorf("compareSecret() = %v, want %v", got, tt.want)
-			}
+			assert.Equal(t, compareSecret(tt.args.incomingSecret, tt.args.secretValue), tt.want)
 		})
 	}
 }
 
-func Test_listener_detectIncoming(t *testing.T) {
+func TestListenerDetectIncoming(t *testing.T) {
 	const goodURL = "https://matched/by/incoming"
 	envRemove := env.PatchAll(t, map[string]string{"SYSTEM_NAMESPACE": "pipelinesascode"})
 	defer envRemove()
@@ -855,7 +853,7 @@ func Test_listener_detectIncoming(t *testing.T) {
 	}
 }
 
-func Test_listener_processIncoming(t *testing.T) {
+func TestListenerProcessIncoming(t *testing.T) {
 	tests := []struct {
 		name       string
 		want       provider.Interface
@@ -1081,7 +1079,7 @@ func TestApplyIncomingParams(t *testing.T) {
 	}
 }
 
-func Test_detectIncoming_legacy_warning(t *testing.T) {
+func TestDetectIncomingLegacyWarning(t *testing.T) {
 	ctx, _ := rtesting.SetupFakeContext(t)
 	testNamespace := &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1180,7 +1178,7 @@ func Test_detectIncoming_legacy_warning(t *testing.T) {
 	}
 }
 
-func Test_detectIncoming_body_params_are_parsed(t *testing.T) {
+func TestDetectIncomingBodyParamsAreParsed(t *testing.T) {
 	ctx, _ := rtesting.SetupFakeContext(t)
 	testNamespace := &corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1237,7 +1235,7 @@ func Test_detectIncoming_body_params_are_parsed(t *testing.T) {
 	assert.Assert(t, got)
 }
 
-func Test_parseIncomingPayload(t *testing.T) {
+func TestParseIncomingPayload(t *testing.T) {
 	tests := []struct {
 		name          string
 		method        string

pkg/adapter/sinker_test.go

@@ -85,7 +85,7 @@ func setupTestData(t *testing.T, repos []*v1alpha1.Repository) *params.Run {
 }
 
 // TestSetupClient_GitHubAppVsOther tests the different code paths for GitHub Apps vs other providers.
-func TestSetupClient_GitHubAppVsOther(t *testing.T) {
+func TestSetupClientGitHubAppVsOther(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -407,10 +407,10 @@ func TestFindMatchingRepository(t *testing.T) {
 	}
 }
 
-// TestProcessEvent_SkipCI_PushEvent tests the skip-CI logic for push events.
+// TestProcessEventSkipCIPushEvent tests the skip-CI logic for push events.
 // This tests that the SkipCI check works correctly for push events where the
 // commit message is available directly in the webhook payload (event.SHATitle).
-func TestProcessEvent_SkipCI_PushEvent(t *testing.T) {
+func TestProcessEventSkipCIPushEvent(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -473,10 +473,10 @@ func TestProcessEvent_SkipCI_PushEvent(t *testing.T) {
 	}
 }
 
-// TestGetCommitInfo_SetsHasSkipCommand tests that GetCommitInfo correctly sets
+// TestGetCommitInfoSetsHasSkipCommand tests that GetCommitInfo correctly sets
 // HasSkipCommand when the commit message contains skip-CI commands.
 // This tests the full flow that processEvent uses for PR events.
-func TestGetCommitInfo_SetsHasSkipCommand(t *testing.T) {
+func TestGetCommitInfoSetsHasSkipCommand(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -554,10 +554,10 @@ func TestGetCommitInfo_SetsHasSkipCommand(t *testing.T) {
 	}
 }
 
-// TestProcessEvent_SkipCI_Integration documents the skip-CI flow integration.
+// TestProcessEventSkipCIIntegration documents the skip-CI flow integration.
 // This is a documentation test that verifies the skip-CI decision logic
 // matches what's implemented in sinker.go processEvent().
-func TestProcessEvent_SkipCI_Integration(t *testing.T) {
+func TestProcessEventSkipCIIntegration(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {

pkg/cli/iostreams_test.go

@@ -15,23 +15,23 @@ func TestNewIOStreams(t *testing.T) {
 	assert.Assert(t, ios.ErrOut != nil)
 }
 
-func TestIOStreams_ColorEnabled(t *testing.T) {
+func TestIOStreamsColorEnabled(t *testing.T) {
 	ios := &IOStreams{colorEnabled: true}
 	assert.Equal(t, ios.ColorEnabled(), true)
 
 	ios.SetColorEnabled(false)
 	assert.Equal(t, ios.ColorEnabled(), false)
 }
 
-func TestIOStreams_ColorSupport256(t *testing.T) {
+func TestIOStreamsColorSupport256(t *testing.T) {
 	ios := &IOStreams{is256enabled: true}
 	assert.Equal(t, ios.ColorSupport256(), true)
 
 	ios.is256enabled = false
 	assert.Equal(t, ios.ColorSupport256(), false)
 }
 
-func TestIOStreams_IsStdoutTTY(t *testing.T) {
+func TestIOStreamsIsStdoutTTY(t *testing.T) {
 	tests := []struct {
 		name     string
 		setup    func() *IOStreams

pkg/cmd/tknpac/bootstrap/github_test.go

@@ -7,7 +7,7 @@ import (
 	"gotest.tools/v3/assert"
 )
 
-func Test_generateManifest(t *testing.T) {
+func TestGenerateManifest(t *testing.T) {
 	type args struct {
 		opts *bootstrapOpts
 	}
@@ -39,7 +39,7 @@ func Test_generateManifest(t *testing.T) {
 	}
 }
 
-func Test_getGHClient(t *testing.T) {
+func TestGetGHClient(t *testing.T) {
 	tests := []struct {
 		name    string
 		URL     string

pkg/cmd/tknpac/webhook/add_test.go

@@ -5,7 +5,6 @@ import (
 	"io"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/cli"
@@ -14,6 +13,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
+	"gotest.tools/v3/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	rtesting "knative.dev/pkg/reconciler/testing"
@@ -230,13 +230,12 @@ func TestWebhookAdd(t *testing.T) {
 				Info: info.Info{Kube: &info.KubeOpts{Namespace: tt.opts.Namespace}},
 			}
 			io, out := newIOStream()
-			if err := add(ctx, tt.opts, cs, io,
-				tt.repoName, tt.pacNamespace); (err != nil) != tt.wantErr {
-				t.Errorf("add() error = %v, wantErr %v", err, tt.wantErr)
+			err := add(ctx, tt.opts, cs, io, tt.repoName, tt.pacNamespace)
+			if tt.wantErr {
+				assert.Assert(t, err != nil)
 			} else {
-				if res := cmp.Diff(out.String(), tt.wantMsg); res != "" {
-					t.Errorf("Diff %s:", res)
-				}
+				assert.NilError(t, err)
+				assert.Equal(t, tt.wantMsg, out.String())
 			}
 		})
 	}