Enable testifylint and fix the issues (#2065)

## Changes
- Enable new linter: testifylint.
- Apply fixes with --fix.
- Fix remaining issues (mostly with aider).

There were 2 cases we --fix did the wrong thing - this seems to a be a
bug in linter: Antonboom/testifylint#210

Nonetheless, I kept that check enabled, it seems useful, just need to be
fixed manually after autofix.

## Tests
Existing tests

.golangci.yaml

@@ -11,6 +11,7 @@ linters:
     - gofmt
     - gofumpt
     - goimports
+    - testifylint
 linters-settings:
   govet:
     enable-all: true
@@ -32,7 +33,10 @@ linters-settings:
   gofumpt:
     module-path: github.com/databricks/cli
     extra-rules: true
-  #goimports:
-  #  local-prefixes: github.com/databricks/cli
+  testifylint:
+    enable-all: true
+    disable:
+      # good check, but we have too many assert.(No)?Errorf? so excluding for now
+      - require-error
 issues:
   exclude-dirs-use-default: false  # recommended by docs https://golangci-lint.run/usage/false-positives/

bundle/bundle_test.go

@@ -2,7 +2,6 @@ package bundle
 
 import (
 	"context"
-	"errors"
 	"io/fs"
 	"os"
 	"path/filepath"
@@ -16,7 +15,7 @@ import (
 
 func TestLoadNotExists(t *testing.T) {
 	b, err := Load(context.Background(), "/doesntexist")
-	assert.True(t, errors.Is(err, fs.ErrNotExist))
+	assert.ErrorIs(t, err, fs.ErrNotExist)
 	assert.Nil(t, b)
 }
 

bundle/config/mutator/configure_dashboard_defaults_test.go

@@ -109,19 +109,19 @@ func TestConfigureDashboardDefaultsEmbedCredentials(t *testing.T) {
 	// Set to true; still true.
 	v, err = dyn.Get(b.Config.Value(), "resources.dashboards.d1.embed_credentials")
 	if assert.NoError(t, err) {
-		assert.Equal(t, true, v.MustBool())
+		assert.True(t, v.MustBool())
 	}
 
 	// Set to false; still false.
 	v, err = dyn.Get(b.Config.Value(), "resources.dashboards.d2.embed_credentials")
 	if assert.NoError(t, err) {
-		assert.Equal(t, false, v.MustBool())
+		assert.False(t, v.MustBool())
 	}
 
 	// Not set; now false.
 	v, err = dyn.Get(b.Config.Value(), "resources.dashboards.d3.embed_credentials")
 	if assert.NoError(t, err) {
-		assert.Equal(t, false, v.MustBool())
+		assert.False(t, v.MustBool())
 	}
 
 	// No valid dashboard; no change.

bundle/config/mutator/default_queueing_test.go

@@ -28,8 +28,8 @@ func TestDefaultQueueingApplyNoJobs(t *testing.T) {
 		},
 	}
 	d := bundle.Apply(context.Background(), b, DefaultQueueing())
-	assert.Len(t, d, 0)
-	assert.Len(t, b.Config.Resources.Jobs, 0)
+	assert.Empty(t, d)
+	assert.Empty(t, b.Config.Resources.Jobs)
 }
 
 func TestDefaultQueueingApplyJobsAlreadyEnabled(t *testing.T) {
@@ -47,7 +47,7 @@ func TestDefaultQueueingApplyJobsAlreadyEnabled(t *testing.T) {
 		},
 	}
 	d := bundle.Apply(context.Background(), b, DefaultQueueing())
-	assert.Len(t, d, 0)
+	assert.Empty(t, d)
 	assert.True(t, b.Config.Resources.Jobs["job"].Queue.Enabled)
 }
 
@@ -66,7 +66,7 @@ func TestDefaultQueueingApplyEnableQueueing(t *testing.T) {
 		},
 	}
 	d := bundle.Apply(context.Background(), b, DefaultQueueing())
-	assert.Len(t, d, 0)
+	assert.Empty(t, d)
 	assert.NotNil(t, b.Config.Resources.Jobs["job"].Queue)
 	assert.True(t, b.Config.Resources.Jobs["job"].Queue.Enabled)
 }
@@ -96,7 +96,7 @@ func TestDefaultQueueingApplyWithMultipleJobs(t *testing.T) {
 		},
 	}
 	d := bundle.Apply(context.Background(), b, DefaultQueueing())
-	assert.Len(t, d, 0)
+	assert.Empty(t, d)
 	assert.False(t, b.Config.Resources.Jobs["job1"].Queue.Enabled)
 	assert.True(t, b.Config.Resources.Jobs["job2"].Queue.Enabled)
 	assert.True(t, b.Config.Resources.Jobs["job3"].Queue.Enabled)

bundle/config/mutator/environments_compat_test.go

@@ -44,7 +44,7 @@ func TestEnvironmentsToTargetsWithEnvironmentsDefined(t *testing.T) {
 
 	diags := bundle.Apply(context.Background(), b, mutator.EnvironmentsToTargets())
 	require.NoError(t, diags.Error())
-	assert.Len(t, b.Config.Environments, 0)
+	assert.Empty(t, b.Config.Environments)
 	assert.Len(t, b.Config.Targets, 1)
 }
 
@@ -61,6 +61,6 @@ func TestEnvironmentsToTargetsWithTargetsDefined(t *testing.T) {
 
 	diags := bundle.Apply(context.Background(), b, mutator.EnvironmentsToTargets())
 	require.NoError(t, diags.Error())
-	assert.Len(t, b.Config.Environments, 0)
+	assert.Empty(t, b.Config.Environments)
 	assert.Len(t, b.Config.Targets, 1)
 }

bundle/config/mutator/merge_job_tasks_test.go

@@ -74,8 +74,8 @@ func TestMergeJobTasks(t *testing.T) {
 	assert.Equal(t, "i3.2xlarge", cluster.NodeTypeId)
 	assert.Equal(t, 4, cluster.NumWorkers)
 	assert.Len(t, task0.Libraries, 2)
-	assert.Equal(t, task0.Libraries[0].Whl, "package1")
-	assert.Equal(t, task0.Libraries[1].Pypi.Package, "package2")
+	assert.Equal(t, "package1", task0.Libraries[0].Whl)
+	assert.Equal(t, "package2", task0.Libraries[1].Pypi.Package)
 
 	// This task was left untouched.
 	task1 := j.Tasks[1].NewCluster

bundle/config/mutator/process_target_mode_test.go

@@ -163,18 +163,18 @@ func TestProcessTargetModeDevelopment(t *testing.T) {
 
 	// Job 1
 	assert.Equal(t, "[dev lennart] job1", b.Config.Resources.Jobs["job1"].Name)
-	assert.Equal(t, b.Config.Resources.Jobs["job1"].Tags["existing"], "tag")
-	assert.Equal(t, b.Config.Resources.Jobs["job1"].Tags["dev"], "lennart")
-	assert.Equal(t, b.Config.Resources.Jobs["job1"].Schedule.PauseStatus, jobs.PauseStatusPaused)
+	assert.Equal(t, "tag", b.Config.Resources.Jobs["job1"].Tags["existing"])
+	assert.Equal(t, "lennart", b.Config.Resources.Jobs["job1"].Tags["dev"])
+	assert.Equal(t, jobs.PauseStatusPaused, b.Config.Resources.Jobs["job1"].Schedule.PauseStatus)
 
 	// Job 2
 	assert.Equal(t, "[dev lennart] job2", b.Config.Resources.Jobs["job2"].Name)
-	assert.Equal(t, b.Config.Resources.Jobs["job2"].Tags["dev"], "lennart")
-	assert.Equal(t, b.Config.Resources.Jobs["job2"].Schedule.PauseStatus, jobs.PauseStatusUnpaused)
+	assert.Equal(t, "lennart", b.Config.Resources.Jobs["job2"].Tags["dev"])
+	assert.Equal(t, jobs.PauseStatusUnpaused, b.Config.Resources.Jobs["job2"].Schedule.PauseStatus)
 
 	// Pipeline 1
 	assert.Equal(t, "[dev lennart] pipeline1", b.Config.Resources.Pipelines["pipeline1"].Name)
-	assert.Equal(t, false, b.Config.Resources.Pipelines["pipeline1"].Continuous)
+	assert.False(t, b.Config.Resources.Pipelines["pipeline1"].Continuous)
 	assert.True(t, b.Config.Resources.Pipelines["pipeline1"].PipelineSpec.Development)
 
 	// Experiment 1

bundle/config/mutator/resolve_variable_references_test.go

@@ -185,11 +185,11 @@ func TestResolveVariableReferencesForPrimitiveNonStringFields(t *testing.T) {
 	// Apply for the variable prefix. This should resolve the variables to their values.
 	diags = bundle.Apply(context.Background(), b, ResolveVariableReferences("variables"))
 	require.NoError(t, diags.Error())
-	assert.Equal(t, true, b.Config.Resources.Jobs["job1"].JobSettings.NotificationSettings.NoAlertForCanceledRuns)
-	assert.Equal(t, true, b.Config.Resources.Jobs["job1"].JobSettings.NotificationSettings.NoAlertForSkippedRuns)
+	assert.True(t, b.Config.Resources.Jobs["job1"].JobSettings.NotificationSettings.NoAlertForCanceledRuns)
+	assert.True(t, b.Config.Resources.Jobs["job1"].JobSettings.NotificationSettings.NoAlertForSkippedRuns)
 	assert.Equal(t, 1, b.Config.Resources.Jobs["job1"].JobSettings.Tasks[0].NewCluster.Autoscale.MinWorkers)
 	assert.Equal(t, 2, b.Config.Resources.Jobs["job1"].JobSettings.Tasks[0].NewCluster.Autoscale.MaxWorkers)
-	assert.Equal(t, 0.5, b.Config.Resources.Jobs["job1"].JobSettings.Tasks[0].NewCluster.AzureAttributes.SpotBidMaxPrice)
+	assert.InDelta(t, 0.5, b.Config.Resources.Jobs["job1"].JobSettings.Tasks[0].NewCluster.AzureAttributes.SpotBidMaxPrice, 0.0001)
 }
 
 func TestResolveComplexVariable(t *testing.T) {

bundle/config/mutator/rewrite_workspace_prefix_test.go

@@ -71,7 +71,7 @@ func TestNoWorkspacePrefixUsed(t *testing.T) {
 	}
 
 	for _, d := range diags {
-		require.Equal(t, d.Severity, diag.Warning)
+		require.Equal(t, diag.Warning, d.Severity)
 		require.Contains(t, expectedErrors, d.Summary)
 		delete(expectedErrors, d.Summary)
 	}

bundle/config/mutator/set_variables_test.go

@@ -30,7 +30,7 @@ func TestSetVariableFromProcessEnvVar(t *testing.T) {
 
 	err = convert.ToTyped(&variable, v)
 	require.NoError(t, err)
-	assert.Equal(t, variable.Value, "process-env")
+	assert.Equal(t, "process-env", variable.Value)
 }
 
 func TestSetVariableUsingDefaultValue(t *testing.T) {
@@ -48,7 +48,7 @@ func TestSetVariableUsingDefaultValue(t *testing.T) {
 
 	err = convert.ToTyped(&variable, v)
 	require.NoError(t, err)
-	assert.Equal(t, variable.Value, "default")
+	assert.Equal(t, "default", variable.Value)
 }
 
 func TestSetVariableWhenAlreadyAValueIsAssigned(t *testing.T) {
@@ -70,7 +70,7 @@ func TestSetVariableWhenAlreadyAValueIsAssigned(t *testing.T) {
 
 	err = convert.ToTyped(&variable, v)
 	require.NoError(t, err)
-	assert.Equal(t, variable.Value, "assigned-value")
+	assert.Equal(t, "assigned-value", variable.Value)
 }
 
 func TestSetVariableEnvVarValueDoesNotOverridePresetValue(t *testing.T) {
@@ -95,7 +95,7 @@ func TestSetVariableEnvVarValueDoesNotOverridePresetValue(t *testing.T) {
 
 	err = convert.ToTyped(&variable, v)
 	require.NoError(t, err)
-	assert.Equal(t, variable.Value, "assigned-value")
+	assert.Equal(t, "assigned-value", variable.Value)
 }
 
 func TestSetVariablesErrorsIfAValueCouldNotBeResolved(t *testing.T) {

bundle/config/resources_test.go

@@ -37,11 +37,11 @@ func TestCustomMarshallerIsImplemented(t *testing.T) {
 		field := rt.Field(i)
 
 		// Fields in Resources are expected be of the form map[string]*resourceStruct
-		assert.Equal(t, field.Type.Kind(), reflect.Map, "Resource %s is not a map", field.Name)
+		assert.Equal(t, reflect.Map, field.Type.Kind(), "Resource %s is not a map", field.Name)
 		kt := field.Type.Key()
-		assert.Equal(t, kt.Kind(), reflect.String, "Resource %s is not a map with string keys", field.Name)
+		assert.Equal(t, reflect.String, kt.Kind(), "Resource %s is not a map with string keys", field.Name)
 		vt := field.Type.Elem()
-		assert.Equal(t, vt.Kind(), reflect.Ptr, "Resource %s is not a map with pointer values", field.Name)
+		assert.Equal(t, reflect.Ptr, vt.Kind(), "Resource %s is not a map with pointer values", field.Name)
 
 		// Marshalling a resourceStruct will panic if resourceStruct does not have a custom marshaller
 		// This is because resourceStruct embeds a Go SDK struct that implements

bundle/config/validate/files_to_sync_test.go

@@ -87,7 +87,7 @@ func TestFilesToSync_EverythingIgnored(t *testing.T) {
 	ctx := context.Background()
 	rb := bundle.ReadOnly(b)
 	diags := bundle.ApplyReadOnly(ctx, rb, FilesToSync())
-	require.Equal(t, 1, len(diags))
+	require.Len(t, diags, 1)
 	assert.Equal(t, diag.Warning, diags[0].Severity)
 	assert.Equal(t, "There are no files to sync, please check your .gitignore", diags[0].Summary)
 }
@@ -101,7 +101,7 @@ func TestFilesToSync_EverythingExcluded(t *testing.T) {
 	ctx := context.Background()
 	rb := bundle.ReadOnly(b)
 	diags := bundle.ApplyReadOnly(ctx, rb, FilesToSync())
-	require.Equal(t, 1, len(diags))
+	require.Len(t, diags, 1)
 	assert.Equal(t, diag.Warning, diags[0].Severity)
 	assert.Equal(t, "There are no files to sync, please check your .gitignore and sync.exclude configuration", diags[0].Summary)
 }

bundle/config/validate/job_cluster_key_defined_test.go

@@ -34,7 +34,7 @@ func TestJobClusterKeyDefined(t *testing.T) {
 	}
 
 	diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), JobClusterKeyDefined())
-	require.Len(t, diags, 0)
+	require.Empty(t, diags)
 	require.NoError(t, diags.Error())
 }
 
@@ -59,8 +59,8 @@ func TestJobClusterKeyNotDefined(t *testing.T) {
 	diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), JobClusterKeyDefined())
 	require.Len(t, diags, 1)
 	require.NoError(t, diags.Error())
-	require.Equal(t, diags[0].Severity, diag.Warning)
-	require.Equal(t, diags[0].Summary, "job_cluster_key do-not-exist is not defined")
+	require.Equal(t, diag.Warning, diags[0].Severity)
+	require.Equal(t, "job_cluster_key do-not-exist is not defined", diags[0].Summary)
 }
 
 func TestJobClusterKeyDefinedInDifferentJob(t *testing.T) {
@@ -92,6 +92,6 @@ func TestJobClusterKeyDefinedInDifferentJob(t *testing.T) {
 	diags := bundle.ApplyReadOnly(context.Background(), bundle.ReadOnly(b), JobClusterKeyDefined())
 	require.Len(t, diags, 1)
 	require.NoError(t, diags.Error())
-	require.Equal(t, diags[0].Severity, diag.Warning)
-	require.Equal(t, diags[0].Summary, "job_cluster_key do-not-exist is not defined")
+	require.Equal(t, diag.Warning, diags[0].Severity)
+	require.Equal(t, "job_cluster_key do-not-exist is not defined", diags[0].Summary)
 }

bundle/context_test.go

@@ -12,7 +12,7 @@ func TestGetPanics(t *testing.T) {
 	defer func() {
 		r := recover()
 		require.NotNil(t, r, "The function did not panic")
-		assert.Equal(t, r, "context not configured with bundle")
+		assert.Equal(t, "context not configured with bundle", r)
 	}()
 
 	Get(context.Background())

bundle/deploy/state_pull_test.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
-	"errors"
 	"io"
 	"io/fs"
 	"os"
@@ -279,7 +278,7 @@ func TestStatePullNoState(t *testing.T) {
 	require.NoError(t, err)
 
 	_, err = os.Stat(statePath)
-	require.True(t, errors.Is(err, fs.ErrNotExist))
+	require.ErrorIs(t, err, fs.ErrNotExist)
 }
 
 func TestStatePullOlderState(t *testing.T) {

bundle/deploy/state_update_test.go

@@ -60,15 +60,15 @@ func TestStateUpdate(t *testing.T) {
 	require.NoError(t, err)
 
 	require.Equal(t, int64(1), state.Seq)
-	require.Equal(t, state.Files, Filelist{
+	require.Equal(t, Filelist{
 		{
 			LocalPath: "test1.py",
 		},
 		{
 			LocalPath:  "test2.py",
 			IsNotebook: true,
 		},
-	})
+	}, state.Files)
 	require.Equal(t, build.GetInfo().Version, state.CliVersion)
 
 	diags = bundle.Apply(ctx, b, s)
@@ -79,15 +79,15 @@ func TestStateUpdate(t *testing.T) {
 	require.NoError(t, err)
 
 	require.Equal(t, int64(2), state.Seq)
-	require.Equal(t, state.Files, Filelist{
+	require.Equal(t, Filelist{
 		{
 			LocalPath: "test1.py",
 		},
 		{
 			LocalPath:  "test2.py",
 			IsNotebook: true,
 		},
-	})
+	}, state.Files)
 	require.Equal(t, build.GetInfo().Version, state.CliVersion)
 
 	// Valid non-empty UUID is generated.
@@ -130,15 +130,15 @@ func TestStateUpdateWithExistingState(t *testing.T) {
 	require.NoError(t, err)
 
 	require.Equal(t, int64(11), state.Seq)
-	require.Equal(t, state.Files, Filelist{
+	require.Equal(t, Filelist{
 		{
 			LocalPath: "test1.py",
 		},
 		{
 			LocalPath:  "test2.py",
 			IsNotebook: true,
 		},
-	})
+	}, state.Files)
 	require.Equal(t, build.GetInfo().Version, state.CliVersion)
 
 	// Existing UUID is not overwritten.

bundle/deploy/terraform/convert_test.go

@@ -254,10 +254,10 @@ func TestBundleToTerraformPipeline(t *testing.T) {
 	assert.Equal(t, "my pipeline", resource.Name)
 	assert.Len(t, resource.Library, 2)
 	assert.Len(t, resource.Notification, 2)
-	assert.Equal(t, resource.Notification[0].Alerts, []string{"on-update-fatal-failure"})
-	assert.Equal(t, resource.Notification[0].EmailRecipients, []string{"[email protected]"})
-	assert.Equal(t, resource.Notification[1].Alerts, []string{"on-update-failure", "on-flow-failure"})
-	assert.Equal(t, resource.Notification[1].EmailRecipients, []string{"[email protected]", "[email protected]"})
+	assert.Equal(t, []string{"on-update-fatal-failure"}, resource.Notification[0].Alerts)
+	assert.Equal(t, []string{"[email protected]"}, resource.Notification[0].EmailRecipients)
+	assert.Equal(t, []string{"on-update-failure", "on-flow-failure"}, resource.Notification[1].Alerts)
+	assert.Equal(t, []string{"[email protected]", "[email protected]"}, resource.Notification[1].EmailRecipients)
 	assert.Nil(t, out.Data)
 }
 
@@ -454,7 +454,7 @@ func TestBundleToTerraformModelServing(t *testing.T) {
 	assert.Equal(t, "name", resource.Name)
 	assert.Equal(t, "model_name", resource.Config.ServedModels[0].ModelName)
 	assert.Equal(t, "1", resource.Config.ServedModels[0].ModelVersion)
-	assert.Equal(t, true, resource.Config.ServedModels[0].ScaleToZeroEnabled)
+	assert.True(t, resource.Config.ServedModels[0].ScaleToZeroEnabled)
 	assert.Equal(t, "Small", resource.Config.ServedModels[0].WorkloadSize)
 	assert.Equal(t, "model_name-1", resource.Config.TrafficConfig.Routes[0].ServedModelName)
 	assert.Equal(t, 100, resource.Config.TrafficConfig.Routes[0].TrafficPercentage)

bundle/deploy/terraform/init_test.go

@@ -225,7 +225,7 @@ func TestSetProxyEnvVars(t *testing.T) {
 	env := make(map[string]string, 0)
 	err := setProxyEnvVars(context.Background(), env, b)
 	require.NoError(t, err)
-	assert.Len(t, env, 0)
+	assert.Empty(t, env)
 
 	// Lower case set.
 	clearEnv()
@@ -293,7 +293,7 @@ func TestSetUserProfileFromInheritEnvVars(t *testing.T) {
 	require.NoError(t, err)
 
 	assert.Contains(t, env, "USERPROFILE")
-	assert.Equal(t, env["USERPROFILE"], "c:\\foo\\c")
+	assert.Equal(t, "c:\\foo\\c", env["USERPROFILE"])
 }
 
 func TestInheritEnvVarsWithAbsentTFConfigFile(t *testing.T) {