From 6b9300d82002a079ef84a0b9f90df34983663557 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Fri, 22 May 2026 09:31:57 +0200 Subject: [PATCH] Drop forbidigo env.Get carve-out for _test.go Plumbs t.Context() into ~34 test sites so they use env.Get instead of os.Getenv, and rewrites three package-level os.Getenv vars in acceptance/acceptance_test.go to use os.LookupEnv (which preserves the "set and non-empty" semantics without tripping the lint). Co-authored-by: Isaac --- .golangci.yaml | 4 -- acceptance/acceptance_test.go | 53 +++++++++++++++---------- acceptance/dbr_test.go | 27 +++++++------ cmd/apps/dev_test.go | 3 +- cmd/apps/import_test.go | 3 +- integration/cmd/fs/helpers_test.go | 4 +- integration/cmd/secrets/secrets_test.go | 6 +-- integration/libs/filer/helpers_test.go | 4 +- libs/exec/exec_test.go | 3 +- 9 files changed, 59 insertions(+), 48 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index 6f41b1cee22..87080c53968 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -176,10 +176,6 @@ linters: - path: bundle/direct/dresources/.*_test.go linters: - exhaustruct - - text: "Use env\\.Get" - path: "_test\\.go$" - linters: - - forbidigo # TODO: remove these exceptions by moving the dependency out of experimental/. - path: cmd/apps/init.go linters: diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index f6ec0805fb2..cd70e194bbf 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -31,6 +31,7 @@ import ( "github.com/databricks/cli/acceptance/internal" "github.com/databricks/cli/internal/testutil" "github.com/databricks/cli/libs/auth" + "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/testdiff" "github.com/databricks/cli/libs/testserver" "github.com/stretchr/testify/require" @@ -39,7 +40,7 @@ import ( var ( KeepTmp bool NoRepl bool - VerboseTest bool = os.Getenv("VERBOSE_TEST") != "" + VerboseTest bool Tail bool Forcerun bool LogRequests bool @@ -63,7 +64,7 @@ var InprocessMode bool const TestLogPrefix = "TESTLOG: " // In benchmark mode we disable parallel run of all tests that contain work "benchmark" in their path -var benchmarkMode = os.Getenv("BENCHMARK_PARAMS") != "" +var benchmarkMode bool func init() { flag.BoolVar(&InprocessMode, "inprocess", false, "Run CLI in the same process as test (for debugging)") @@ -80,6 +81,16 @@ func init() { flag.BoolVar(&WorkspaceTmpDir, "workspace-tmp-dir", false, "Run tests on the workspace file system (For DBR testing).") flag.BoolVar(&OnlyOutTestToml, "only-out-test-toml", false, "Only regenerate out.test.toml files without running tests") flag.BoolVar(&Subset, "subset", false, "Select a subset of EnvMatrix variants that cover all output files. Auto-enabled on -update (unless -run specifies a variant with '=').") + + if v, _ := os.LookupEnv("VERBOSE_TEST"); v != "" { + VerboseTest = true + } + if v, _ := os.LookupEnv("BENCHMARK_PARAMS"); v != "" { + benchmarkMode = true + } + if v, _ := os.LookupEnv("GITHUB_WORKFLOW"); v != "" { + ApplyCITimeoutMultipler = true + } } const ( @@ -104,7 +115,7 @@ const ( userReplacementsFilename = "ACC_REPLS" ) -var ApplyCITimeoutMultipler = os.Getenv("GITHUB_WORKFLOW") != "" +var ApplyCITimeoutMultipler bool var exeSuffix = func() string { if runtime.GOOS == "windows" { @@ -132,7 +143,7 @@ func TestInprocessMode(t *testing.T) { if InprocessMode && !Forcerun { t.Skip("Already tested by TestAccept") } - if os.Getenv("CLOUD_ENV") != "" { + if env.Get(t.Context(), "CLOUD_ENV") != "" { t.Skip("No need to run this as integration test.") } @@ -152,7 +163,7 @@ func setReplsForTestEnvVars(t *testing.T, repls *testdiff.ReplacementsContext) { "TEST_INSTANCE_POOL_ID", } for _, envVar := range envVars { - if value := os.Getenv(envVar); value != "" { + if value := env.Get(t.Context(), envVar); value != "" { repls.Set(value, "["+envVar+"]") } } @@ -236,7 +247,7 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int { repls.SetPath(wheelPath, "[DATABRICKS_BUNDLES_WHEEL]") } - coverDir := os.Getenv("CLI_GOCOVERDIR") + coverDir := env.Get(t.Context(), "CLI_GOCOVERDIR") if coverDir != "" { require.NoError(t, os.MkdirAll(coverDir, os.ModePerm)) @@ -286,7 +297,7 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int { // Make /tools/ available (e.g. yamlfmt) filepath.Join(cwd, "..", "tools"), - os.Getenv("PATH"), + env.Get(t.Context(), "PATH"), } t.Setenv("PATH", strings.Join(paths, string(os.PathListSeparator))) @@ -300,17 +311,17 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int { uvInstall := getUVPythonInstallDir(t) t.Setenv("UV_PYTHON_INSTALL_DIR", uvInstall) - cloudEnv := os.Getenv("CLOUD_ENV") + cloudEnv := env.Get(t.Context(), "CLOUD_ENV") if cloudEnv == "" { internal.StartDefaultServer(t, LogRequests) - if os.Getenv("TEST_DEFAULT_WAREHOUSE_ID") == "" { + if env.Get(t.Context(), "TEST_DEFAULT_WAREHOUSE_ID") == "" { t.Setenv("TEST_DEFAULT_WAREHOUSE_ID", testserver.TestDefaultWarehouseId) } - if os.Getenv("TEST_DEFAULT_CLUSTER_ID") == "" { + if env.Get(t.Context(), "TEST_DEFAULT_CLUSTER_ID") == "" { t.Setenv("TEST_DEFAULT_CLUSTER_ID", testserver.TestDefaultClusterId) } - if os.Getenv("TEST_INSTANCE_POOL_ID") == "" { + if env.Get(t.Context(), "TEST_INSTANCE_POOL_ID") == "" { t.Setenv("TEST_INSTANCE_POOL_ID", testserver.TestDefaultInstancePoolId) } } @@ -399,7 +410,7 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int { t.Skip("Skipping test execution (only regenerating out.test.toml)") } - skipReason := getSkipReason(&config, configPath) + skipReason := getSkipReason(t, &config, configPath) if skipReason != "" { skippedDirs += 1 t.Skip(skipReason) @@ -470,7 +481,7 @@ func testAccept(t *testing.T, inprocessMode bool, singleTest string) int { } func getEnvFilters(t *testing.T) []string { - envFilterValue := os.Getenv(EnvFilterVar) + envFilterValue := env.Get(t.Context(), EnvFilterVar) if envFilterValue == "" { return nil } @@ -522,8 +533,8 @@ func validateTestPhase(phase int) error { } // Return a reason to skip the test. Empty string means "don't skip". -func getSkipReason(config *internal.TestConfig, configPath string) string { - if os.Getenv("DATABRICKS_TEST_SKIPLOCAL") != "" && isTruePtr(config.Local) { +func getSkipReason(t *testing.T, config *internal.TestConfig, configPath string) string { + if env.Get(t.Context(), "DATABRICKS_TEST_SKIPLOCAL") != "" && isTruePtr(config.Local) { return "Disabled via DATABRICKS_TEST_SKIPLOCAL environment variable in " + configPath } @@ -540,7 +551,7 @@ func getSkipReason(config *internal.TestConfig, configPath string) string { return fmt.Sprintf("Disabled via GOOS.%s setting in %s", runtime.GOOS, configPath) } - cloudEnv := os.Getenv("CLOUD_ENV") + cloudEnv := env.Get(t.Context(), "CLOUD_ENV") isRunningOnCloud := cloudEnv != "" if isRunningOnCloud { @@ -566,15 +577,15 @@ func getSkipReason(config *internal.TestConfig, configPath string) string { ) } - if isTruePtr(config.RequiresUnityCatalog) && os.Getenv("TEST_METASTORE_ID") == "" { + if isTruePtr(config.RequiresUnityCatalog) && env.Get(t.Context(), "TEST_METASTORE_ID") == "" { return fmt.Sprintf("Disabled via RequiresUnityCatalog setting in %s (TEST_METASTORE_ID is empty)", configPath) } - if isTruePtr(config.RequiresWarehouse) && os.Getenv("TEST_DEFAULT_WAREHOUSE_ID") == "" { + if isTruePtr(config.RequiresWarehouse) && env.Get(t.Context(), "TEST_DEFAULT_WAREHOUSE_ID") == "" { return fmt.Sprintf("Disabled via RequiresWarehouse setting in %s (TEST_DEFAULT_WAREHOUSE_ID is empty)", configPath) } - if isTruePtr(config.RequiresCluster) && os.Getenv("TEST_DEFAULT_CLUSTER_ID") == "" { + if isTruePtr(config.RequiresCluster) && env.Get(t.Context(), "TEST_DEFAULT_CLUSTER_ID") == "" { return fmt.Sprintf("Disabled via RequiresCluster setting in %s (TEST_DEFAULT_CLUSTER_ID is empty)", configPath) } @@ -611,7 +622,7 @@ func runTest(t *testing.T, } tailOutput := Tail - cloudEnv := os.Getenv("CLOUD_ENV") + cloudEnv := env.Get(t.Context(), "CLOUD_ENV") isRunningOnCloud := cloudEnv != "" if isRunningOnCloud && isTruePtr(config.CloudSlow) && testing.Verbose() { @@ -685,7 +696,7 @@ func runTest(t *testing.T, cmd.Env = auth.ProcessEnv(cfg) - rateLimit := os.Getenv("DATABRICKS_RATE_LIMIT") + rateLimit := env.Get(t.Context(), "DATABRICKS_RATE_LIMIT") if rateLimit == "" { if isRunningOnCloud { rateLimit = "100" diff --git a/acceptance/dbr_test.go b/acceptance/dbr_test.go index 2fe498698a1..d3ecfaebdf4 100644 --- a/acceptance/dbr_test.go +++ b/acceptance/dbr_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/databricks/cli/internal/testarchive" + "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/filer" "github.com/databricks/databricks-sdk-go" "github.com/databricks/databricks-sdk-go/service/compute" @@ -159,23 +160,23 @@ func uploadRunner(ctx context.Context, t *testing.T, w *databricks.WorkspaceClie } // buildBaseParams builds the common parameters for test tasks. -func buildBaseParams(testDir, archiveName, debugLogPath string) map[string]string { +func buildBaseParams(ctx context.Context, testDir, archiveName, debugLogPath string) map[string]string { return map[string]string{ "archive_path": path.Join(testDir, archiveName), - "cloud_env": os.Getenv("CLOUD_ENV"), - "test_default_warehouse_id": os.Getenv("TEST_DEFAULT_WAREHOUSE_ID"), - "test_default_cluster_id": os.Getenv("TEST_DEFAULT_CLUSTER_ID"), - "test_instance_pool_id": os.Getenv("TEST_INSTANCE_POOL_ID"), - "test_metastore_id": os.Getenv("TEST_METASTORE_ID"), - "test_user_email": os.Getenv("TEST_USER_EMAIL"), - "test_sp_application_id": os.Getenv("TEST_SP_APPLICATION_ID"), + "cloud_env": env.Get(ctx, "CLOUD_ENV"), + "test_default_warehouse_id": env.Get(ctx, "TEST_DEFAULT_WAREHOUSE_ID"), + "test_default_cluster_id": env.Get(ctx, "TEST_DEFAULT_CLUSTER_ID"), + "test_instance_pool_id": env.Get(ctx, "TEST_INSTANCE_POOL_ID"), + "test_metastore_id": env.Get(ctx, "TEST_METASTORE_ID"), + "test_user_email": env.Get(ctx, "TEST_USER_EMAIL"), + "test_sp_application_id": env.Get(ctx, "TEST_SP_APPLICATION_ID"), "debug_log_path": debugLogPath, } } // runDbrTests creates a job and runs it to execute cloud and local acceptance tests on DBR. func runDbrTests(ctx context.Context, t *testing.T, w *databricks.WorkspaceClient, testDir, archiveName, runnerName string, config dbrTestConfig) { - cloudEnv := os.Getenv("CLOUD_ENV") + cloudEnv := env.Get(ctx, "CLOUD_ENV") if cloudEnv == "" { t.Fatal("CLOUD_ENV is not set. Please run DBR tests from a CI environment with deco env run.") } @@ -208,7 +209,7 @@ func runDbrTests(ctx context.Context, t *testing.T, w *databricks.WorkspaceClien require.NoError(t, err) // Build cloud test parameters (Cloud=true tests, run with CLOUD_ENV set) - cloudParams := buildBaseParams(testDir, archiveName, debugLogPath) + cloudParams := buildBaseParams(ctx, testDir, archiveName, debugLogPath) cloudParams["test_type"] = "cloud" cloudParams["test_filter"] = config.cloudTestFilter @@ -349,16 +350,16 @@ func runDbrAcceptanceTests(t *testing.T, config dbrTestConfig) { // OR // make dbr-test func TestDbrAcceptance(t *testing.T) { - if os.Getenv("DBR_ENABLED") != "true" { + if env.Get(t.Context(), "DBR_ENABLED") != "true" { t.Skip("Skipping DBR test: DBR_ENABLED not set") } - if os.Getenv("CLOUD_ENV") == "" { + if env.Get(t.Context(), "CLOUD_ENV") == "" { t.Skip("Skipping DBR test: CLOUD_ENV not set") } runDbrAcceptanceTests(t, dbrTestConfig{ timeout: 3 * time.Hour, - verbose: os.Getenv("DBR_TEST_VERBOSE") != "", + verbose: env.Get(t.Context(), "DBR_TEST_VERBOSE") != "", }) } diff --git a/cmd/apps/dev_test.go b/cmd/apps/dev_test.go index a1e8deef447..b445ef9fb86 100644 --- a/cmd/apps/dev_test.go +++ b/cmd/apps/dev_test.go @@ -8,6 +8,7 @@ import ( "github.com/databricks/cli/libs/apps/vite" "github.com/databricks/cli/libs/cmdio" + "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -54,7 +55,7 @@ func TestViteServerScriptContent(t *testing.T) { func TestStartViteDevServerNoNode(t *testing.T) { // Skip this test if node is not available or in CI environments - if os.Getenv("CI") != "" { + if env.Get(t.Context(), "CI") != "" { t.Skip("Skipping node-dependent test in CI") } diff --git a/cmd/apps/import_test.go b/cmd/apps/import_test.go index b50ec937991..7cd0595aced 100644 --- a/cmd/apps/import_test.go +++ b/cmd/apps/import_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/databricks/cli/libs/dyn" + "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -186,7 +187,7 @@ func TestInlineAppConfigFileErrors(t *testing.T) { if runtime.GOOS == "windows" { // On Windows, use icacls to deny read access to the current user - username := os.Getenv("USERNAME") + username := env.Get(t.Context(), "USERNAME") cmd := exec.Command("icacls", filename, "/deny", username+":(R)") err = cmd.Run() require.NoError(t, err) diff --git a/integration/cmd/fs/helpers_test.go b/integration/cmd/fs/helpers_test.go index e1bebb28f42..861622ce1a1 100644 --- a/integration/cmd/fs/helpers_test.go +++ b/integration/cmd/fs/helpers_test.go @@ -1,13 +1,13 @@ package fs_test import ( - "os" "path" "path/filepath" "github.com/databricks/cli/integration/internal/acc" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/filer" "github.com/stretchr/testify/require" ) @@ -32,7 +32,7 @@ func setupDbfsFiler(t testutil.TestingT) (filer.Filer, string) { func setupUcVolumesFiler(t testutil.TestingT) (filer.Filer, string) { _, wt := acc.WorkspaceTest(t) - if os.Getenv("TEST_METASTORE_ID") == "" { + if env.Get(t.Context(), "TEST_METASTORE_ID") == "" { t.Skip("Skipping tests that require a UC Volume when metastore id is not set.") } diff --git a/integration/cmd/secrets/secrets_test.go b/integration/cmd/secrets/secrets_test.go index 31ee71d90f2..36103b8590d 100644 --- a/integration/cmd/secrets/secrets_test.go +++ b/integration/cmd/secrets/secrets_test.go @@ -4,12 +4,12 @@ import ( "context" "encoding/base64" "fmt" - "os" "testing" "github.com/databricks/cli/integration/internal/acc" "github.com/databricks/cli/internal/testcli" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/env" "github.com/databricks/databricks-sdk-go/service/workspace" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -67,7 +67,7 @@ func assertSecretBytesValue(t *acc.WorkspaceT, scope, key string, expected []byt func TestSecretsPutSecretStringValue(tt *testing.T) { // aws-prod-ucws sets CLOUD_ENV to "ucws" - if os.Getenv("CLOUD_ENV") == "ucws" { + if env.Get(tt.Context(), "CLOUD_ENV") == "ucws" { /* FAIL integration/cmd/secrets.TestSecretsPutSecretStringValue (re-run 2) (1201.25s) secrets_test.go:73: args: secrets, put-secret, cli-acc-c4ea35e34b52466cbf9a090c431d6a0b, test-key, --string-value, test-value @@ -95,7 +95,7 @@ func TestSecretsPutSecretStringValue(tt *testing.T) { } func TestSecretsPutSecretBytesValue(tt *testing.T) { - if os.Getenv("CLOUD_ENV") == "ucws" { + if env.Get(tt.Context(), "CLOUD_ENV") == "ucws" { tt.Skip("Skipping to unblock PRs; re-enable if works") } diff --git a/integration/libs/filer/helpers_test.go b/integration/libs/filer/helpers_test.go index a3a3aaae589..d01ccb2078a 100644 --- a/integration/libs/filer/helpers_test.go +++ b/integration/libs/filer/helpers_test.go @@ -3,13 +3,13 @@ package filer_test import ( "errors" "net/http" - "os" "path" "path/filepath" "github.com/databricks/cli/integration/internal/acc" "github.com/databricks/cli/internal/testutil" + "github.com/databricks/cli/libs/env" "github.com/databricks/cli/libs/filer" "github.com/databricks/databricks-sdk-go/apierr" "github.com/stretchr/testify/require" @@ -61,7 +61,7 @@ func setupDbfsFiler(t testutil.TestingT) (filer.Filer, string) { func setupUcVolumesFiler(t testutil.TestingT) (filer.Filer, string) { _, wt := acc.WorkspaceTest(t) - if os.Getenv("TEST_METASTORE_ID") == "" { + if env.Get(t.Context(), "TEST_METASTORE_ID") == "" { t.Skip("Skipping tests that require a UC Volume when metastore id is not set.") } diff --git a/libs/exec/exec_test.go b/libs/exec/exec_test.go index ffcf7a0a023..74f0c92d09e 100644 --- a/libs/exec/exec_test.go +++ b/libs/exec/exec_test.go @@ -10,6 +10,7 @@ import ( "sync" "testing" + "github.com/databricks/cli/libs/env" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -84,7 +85,7 @@ func testExecutorWithShell(t *testing.T, shell string) { // Create temporary directory with only the shell executable in the PATH. tmpDir := t.TempDir() - t.Setenv("PATH", fmt.Sprintf("%s%c%s", tmpDir, os.PathListSeparator, os.Getenv("PATH"))) + t.Setenv("PATH", fmt.Sprintf("%s%c%s", tmpDir, os.PathListSeparator, env.Get(t.Context(), "PATH"))) if runtime.GOOS == "windows" { err = os.Symlink(p, fmt.Sprintf("%s/%s.exe", tmpDir, shell)) require.NoError(t, err)