Enabled thelper linter.

This linter cleans up our test output by omiting helper functions from test failure messages, instead labeling the failure with the calling function's details. This makes it easier to locate the true origin of the failure. There are no expected functional changes in this PR. The code changes in this PR are all changes to testing code to comply with Go style and best practices, specifically `t.Helper`: - https://pkg.go.dev/testing#T.Helper - https://goo.gle/go-style/best-practices#error-handling-in-test-helpers Context: #5899 /kind cleanup
tektoncd · Jan 2, 2023 · 1a9b7ed · 1a9b7ed
1 parent 1920789
commit 1a9b7ed
Show file tree

Hide file tree

Showing 53 changed files with 262 additions and 106 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -31,6 +31,7 @@ linters:
   - misspell
   - revive
   - typecheck
+  - thelper
   - unconvert
   - unused
   - usestdlibvars

diff --git a/pkg/apis/config/artifact_bucket_test.go b/pkg/apis/config/artifact_bucket_test.go
@@ -91,6 +91,7 @@ func TestGetArtifactBucketConfigName(t *testing.T) {
 }
 
 func verifyConfigFileWithExpectedArtifactBucketConfig(t *testing.T, fileName string, expectedConfig *config.ArtifactBucket) {
+	t.Helper()
 	cm := test.ConfigMapFromTestFile(t, fileName)
 	if ab, err := config.NewArtifactBucketFromConfigMap(cm); err == nil {
 		if d := cmp.Diff(ab, expectedConfig); d != "" {

diff --git a/pkg/apis/config/artifact_pvc_test.go b/pkg/apis/config/artifact_pvc_test.go
@@ -89,6 +89,7 @@ func TestGetArtifactPVCConfigName(t *testing.T) {
 }
 
 func verifyConfigFileWithExpectedArtifactPVCConfig(t *testing.T, fileName string, expectedConfig *config.ArtifactPVC) {
+	t.Helper()
 	cm := test.ConfigMapFromTestFile(t, fileName)
 	if ab, err := config.NewArtifactPVCFromConfigMap(cm); err == nil {
 		if d := cmp.Diff(ab, expectedConfig); d != "" {

diff --git a/pkg/apis/config/default_test.go b/pkg/apis/config/default_test.go
@@ -310,6 +310,7 @@ func verifyConfigFileWithExpectedConfig(t *testing.T, fileName string, expectedC
 }
 
 func verifyConfigFileWithExpectedError(t *testing.T, fileName string) {
+	t.Helper()
 	cm := test.ConfigMapFromTestFile(t, fileName)
 	if _, err := config.NewDefaultsFromConfigMap(cm); err == nil {
 		t.Errorf("NewDefaultsFromConfigMap(actual) was expected to return an error")

diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go
@@ -302,6 +302,7 @@ func TestCheckWarnResourceVerificationMode(t *testing.T) {
 }
 
 func verifyConfigFileWithExpectedFeatureFlagsConfig(t *testing.T, fileName string, expectedConfig *config.FeatureFlags) {
+	t.Helper()
 	cm := test.ConfigMapFromTestFile(t, fileName)
 	if flags, err := config.NewFeatureFlagsFromConfigMap(cm); err == nil {
 		if d := cmp.Diff(expectedConfig, flags); d != "" {

diff --git a/pkg/apis/config/metrics_test.go b/pkg/apis/config/metrics_test.go
@@ -69,6 +69,7 @@ func TestNewMetricsFromEmptyConfigMap(t *testing.T) {
 }
 
 func verifyConfigFileWithExpectedMetricsConfig(t *testing.T, fileName string, expectedConfig *config.Metrics) {
+	t.Helper()
 	cm := test.ConfigMapFromTestFile(t, fileName)
 	if ab, err := config.NewMetricsFromConfigMap(cm); err == nil {
 		if d := cmp.Diff(ab, expectedConfig); d != "" {

diff --git a/pkg/apis/config/resolver/feature_flags_test.go b/pkg/apis/config/resolver/feature_flags_test.go
@@ -115,6 +115,7 @@ func TestNewFeatureFlagsConfigMapErrors(t *testing.T) {
 }
 
 func verifyConfigFileWithExpectedFeatureFlagsConfig(t *testing.T, fileName string, expectedConfig *resolver.FeatureFlags) {
+	t.Helper()
 	cm := test.ConfigMapFromTestFile(t, fileName)
 	if flags, err := resolver.NewFeatureFlagsFromConfigMap(cm); err == nil {
 		if d := cmp.Diff(expectedConfig, flags); d != "" {

diff --git a/pkg/apis/config/trusted_resources_test.go b/pkg/apis/config/trusted_resources_test.go
@@ -61,6 +61,7 @@ func TestNewTrustedResourcesFromEmptyConfigMap(t *testing.T) {
 }
 
 func verifyConfigFileWithExpectedTrustedResourcesConfig(t *testing.T, fileName string, expectedConfig *config.TrustedResources) {
+	t.Helper()
 	cm := test.ConfigMapFromTestFile(t, fileName)
 	if ab, err := config.NewTrustedResourcesConfigFromConfigMap(cm); err == nil {
 		if d := cmp.Diff(ab, expectedConfig); d != "" {

diff --git a/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go b/pkg/apis/pipeline/v1beta1/pipeline_validation_test.go
@@ -3639,6 +3639,7 @@ func getTaskSpec() TaskSpec {
 }
 
 func enableFeatures(t *testing.T, features []string) func(context.Context) context.Context {
+	t.Helper()
 	return func(ctx context.Context) context.Context {
 		s := config.NewStore(logtesting.TestLogger(t))
 		data := make(map[string]string)

diff --git a/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go b/pkg/apis/pipeline/v1beta1/pipelineref_validation_test.go
@@ -193,6 +193,7 @@ func TestPipelineRef_Valid(t *testing.T) {
 }
 
 func enableTektonOCIBundles(t *testing.T) func(context.Context) context.Context {
+	t.Helper()
 	return func(ctx context.Context) context.Context {
 		s := config.NewStore(logtesting.TestLogger(t))
 		s.OnConfigChanged(&corev1.ConfigMap{

diff --git a/pkg/credentials/initialize_test.go b/pkg/credentials/initialize_test.go
@@ -74,6 +74,7 @@ func TestTryCopyCredFileMissing(t *testing.T) {
 }
 
 func writeFakeCred(t *testing.T, dir, name, contents string) string {
+	t.Helper()
 	flags := os.O_RDWR | os.O_CREATE | os.O_TRUNC
 	path := filepath.Join(dir, name)
 	cred, err := os.OpenFile(path, flags, 0600)

diff --git a/pkg/entrypoint/entrypointer_test.go b/pkg/entrypoint/entrypointer_test.go
@@ -688,6 +688,7 @@ func (f *fakeResultsWriter) Run(ctx context.Context, args ...string) error {
 }
 
 func createTmpDir(t *testing.T, name string) string {
+	t.Helper()
 	tmpDir, err := os.MkdirTemp("", name)
 	if err != nil {
 		t.Fatalf("unexpected error creating temporary dir: %v", err)

diff --git a/pkg/git/git_test.go b/pkg/git/git_test.go
@@ -30,6 +30,7 @@ import (
 const fileMode = 0755 // rwxr-xr-x
 
 func withTemporaryGitConfig(t *testing.T) {
+	t.Helper()
 	gitConfigDir := t.TempDir()
 	key := "GIT_CONFIG_GLOBAL"
 	t.Setenv(key, filepath.Join(gitConfigDir, "config"))
@@ -173,7 +174,7 @@ func TestValidateGitAuth(t *testing.T) {
 			}
 
 			validateGitAuth(logger, credsDir, tt.url)
-			checkLogMessage(tt.logMessage, log, 0, t)
+			checkLogMessage(t, tt.logMessage, log, 0)
 		})
 	}
 }
@@ -379,13 +380,14 @@ func TestFetch(t *testing.T) {
 			if tt.spec.Submodules {
 				logLine = 1
 			}
-			checkLogMessage(tt.logMessage, log, logLine, t)
+			checkLogMessage(t, tt.logMessage, log, logLine)
 		})
 	}
 }
 
 // Create a temporary Git dir locally for testing against instead of using a potentially flaky remote URL.
 func createTempGit(t *testing.T, logger *zap.SugaredLogger, gitDir string, submodPath string) {
+	t.Helper()
 	if _, err := run(logger, "", "init", gitDir); err != nil {
 		t.Fatal(err)
 	}
@@ -417,15 +419,17 @@ func createTempGit(t *testing.T, logger *zap.SugaredLogger, gitDir string, submo
 	}
 }
 
-func checkLogMessage(logMessage string, log *observer.ObservedLogs, logLine int, t *testing.T) {
-	if logMessage != "" {
-		allLogLines := log.All()
-		if len(allLogLines) == 0 {
-			t.Fatal("We didn't receive any logging")
-		}
-		gotmsg := allLogLines[logLine].Message
-		if !strings.Contains(gotmsg, logMessage) {
-			t.Errorf("log message: '%s'\n should contain: '%s'", logMessage, gotmsg)
-		}
+func checkLogMessage(t *testing.T, logMessage string, log *observer.ObservedLogs, logLine int) {
+	t.Helper()
+	if logMessage == "" {
+		return
+	}
+	allLogLines := log.All()
+	if len(allLogLines) == 0 {
+		t.Fatal("We didn't receive any logging")
+	}
+	gotmsg := allLogLines[logLine].Message
+	if !strings.Contains(gotmsg, logMessage) {
+		t.Errorf("log message: '%s'\n should contain: '%s'", logMessage, gotmsg)
 	}
 }
diff --git a/pkg/internal/computeresources/limitrange/limitrange_test.go b/pkg/internal/computeresources/limitrange/limitrange_test.go
@@ -34,6 +34,7 @@ import (
 )
 
 func setupTestData(t *testing.T, serviceaccounts []corev1.ServiceAccount, limitranges []corev1.LimitRange) (context.Context, func()) {
+	t.Helper()
 	ctx, _ := ttesting.SetupFakeContext(t)
 	ctx, cancel := context.WithCancel(ctx)
 	kubeclient := fakekubeclient.Get(ctx)

diff --git a/pkg/internal/computeresources/transformer_test.go b/pkg/internal/computeresources/transformer_test.go
@@ -939,6 +939,7 @@ func TestTransformerMultipleContainer(t *testing.T) {
 }
 
 func cmpRequestsAndLimits(t *testing.T, want, got corev1.PodSpec) {
+	t.Helper()
 	// diff init containers
 	if len(want.InitContainers) != len(got.InitContainers) {
 		t.Errorf("Expected %d init containers, got %d", len(want.InitContainers), len(got.InitContainers))

diff --git a/pkg/pod/entrypoint_lookup_impl_test.go b/pkg/pod/entrypoint_lookup_impl_test.go
@@ -182,6 +182,7 @@ func TestGetImageWithImagePullSecrets(t *testing.T) {
 }
 
 func mustRandomImage(t *testing.T) v1.Image {
+	t.Helper()
 	img, err := random.Image(10, 10)
 	if err != nil {
 		t.Fatal(err)

diff --git a/pkg/pullrequest/api_test.go b/pkg/pullrequest/api_test.go
@@ -77,6 +77,7 @@ func defaultResource() *Resource {
 }
 
 func newHandler(t *testing.T) (*Handler, *fake.Data) {
+	t.Helper()
 	logger := zaptest.NewLogger(t, zaptest.WrapOptions(zap.AddCaller())).Sugar()
 	client, data := fake.NewDefault()
 

diff --git a/pkg/reconciler/customrun/customrun_test.go b/pkg/reconciler/customrun/customrun_test.go
@@ -45,6 +45,7 @@ import (
 )
 
 func initializeCustomRunControllerAssets(t *testing.T, d test.Data) (test.Assets, func()) {
+	t.Helper()
 	ctx, _ := ttesting.SetupFakeContext(t)
 	ctx = ttesting.SetupFakeCloudClientContext(ctx, d.ExpectedCloudEventCount)
 	ctx, cancel := context.WithCancel(ctx)

diff --git a/pkg/reconciler/events/cloudevent/cloud_event_controller_test.go b/pkg/reconciler/events/cloudevent/cloud_event_controller_test.go
@@ -770,8 +770,8 @@ func TestEmitCloudEventsWhenConditionChange(t *testing.T) {
 }
 
 func setupFakeContext(t *testing.T, behaviour FakeClientBehaviour, withClient bool, expectedEventCount int) context.Context {
-	var ctx context.Context
-	ctx, _ = rtesting.SetupFakeContext(t)
+	t.Helper()
+	ctx, _ := rtesting.SetupFakeContext(t)
 	if withClient {
 		ctx = WithClient(ctx, &behaviour, expectedEventCount)
 	}