Drop forbidigo env.Get carve-out for _test.go#5330
Conversation
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
| 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 != "" { |
There was a problem hiding this comment.
It seems odd that we use os.LookupEnv instead of os.GetEnv here because we banned os.GetEnv.
Also, what's the value of using env.Get in acceptance test runner? I thought it's only needed for production code that we want to unit test.
Approval status: pending
|
2 participants
Removes the
text: "Use env\.Get"/path: "_test\.go$"exception from.golangci.yamlso the forbidigo rule applies uniformly. Touches ~34 test sites to callenv.Get(t.Context(), …)instead ofos.Getenv(…), threadingctxinto two helpers (getSkipReason,buildBaseParams) along the way.The three package-level
var X = os.Getenv("Y") != ""declarations inacceptance/acceptance_test.go(VerboseTest,benchmarkMode,ApplyCITimeoutMultipler) have not/ctxavailable, so they're switched to barebooldeclarations with the env reads moved into the existinginit()viaos.LookupEnv— which is not on the forbidigo list and preserves the "set and non-empty" semantics exactly.