add tenv linter

[sivchari]

tenv is analyzer that detects using os.Setenv instead of t.Setenv.
in detail, please check following link.
https://github.com/sivchari/tenv

[butuzov]

Does it check the version from go.mod or compiled runtime?

[ldez]

The current implementation produces too many false-positive, the linter must be improved.

[sivchari]

@butuzov
I checked runtime.Version()

[sivchari]

@ldez
thaks for review.
ok, I'd improve it.

may I ask you to share the false positive you found when you have a moment?

[butuzov]

@butuzov
I checked rutime.Verision()

Maybe it's better to parse mod file to see if it require go.1.17 (see type Go)? So you can activate linter if it's forced or module's go is 1.17

[ldez]

I recommend checking how the other linters (ex: staticcheck) handle that.

Note: runtime.Version() will not work because it's based on the Go version of the golangci-lint build and not on the env or project Go version.

[sivchari]

I've fixed

[johejo]

I feel that os.Setenv in places where *testing.T cannot be referenced should not be warned by default.

[sivchari]

Why do you think that ? Please tell me reason.
And, if tenv is changed as you say implementation, tenv will can't detect following case.

package main_test

import (
	"os"
	"testing"
)

var env string

func setup() {
	env = os.Getenv("a")
	os.Setenv("a", "b")
}

func cleanup() {
	os.Setenv("a", env)
}

func TestF(t *testing.T) {
	setup()
	// do something ...
	t.Cleanup(cleanup)
}

What do you think ?

[johejo]

The following is based on my experience. I don't know if it applies to all Gophers.

The refactoring is probably

1. Pass *testing.T to the helper
2. Replace os.Setenv with t.Setenv in the helper

The first step is changing the function signatures, which can be painful at times.

If the default warning is only for the case that can reference *testing.T, user probably don't need to change the function signatures so it's probably not too difficult.

[sivchari]

This linter would rather have that refactoring as one of its goals.

Also, one of the motivations for using tenv is to change things that don't use testing.*

So the pain you mentioned is the right one, and changing this function signature is probably not difficult because it only in the test file.

[johejo]

By the way, tenv seems to target only *_test.go.
Is this intentional?
Helpers are sometimes cut out into separate packages.

[sivchari]

Yes, it's intentional.

It depends on the product whether you want to get the environment variables for testing in other packages.

Therefore, considering that this linter will support many methods, it's a proportionally large number of false-positive will be generated.

We can also determine by package name, but in Go, the _test suffix is just whether to include it in the build or not, so this is also not practical.

However, the file name _test.go is a naming convention, so we use this method.

[ldez]

I agree with @johejo that the linter should report problems only on functions that use b *testing.B or t *testing.T or tb testing.TB.
The fact to check all the content of a *_test.go file can be an option and not the default behavior.

[sivchari]

@ldez

thanks for comment.
I would like to change my opinion to one that is more supportive and easier to use.

@johejo

I'll change the policy of checking all *_test.go's method with the -all command and supporting *testing.T, *testing.B, and testing.TB by default
thanks for review.

[sivchari]

I update check range and add all option.
Please review when you have a chance.

[ldez]

The report messages can be improved, instead of func TestF is not using testing.Setenv I suggest this:

# the name of the variable (`t`, `b`, `tb`) can be used in the majority of the cases

os.Setenv() can be replaced by `t.Setenv()` in <Method/Function name>

os.Setenv() can be replaced by `b.Setenv()` in <Method/Function name>

os.Setenv() can be replaced by `tb.Setenv()` in <Method/Function name>

os.Setenv() can be replaced by `testing.Setenv()` in <Method/Function name>

---

I created a sample (2 files) with comments to explain what is for me the expected behavior of this linter:

foo.go

package sandbox

import (
	"os"
	"testing"
)

// never reported
var errF = os.Setenv("Foo", "bar")

func Foo() {
	// never reported
	os.Setenv("Foo", "bar")
}

func Bar(t *testing.T) {
	// reported by default (signature based)
	os.Setenv("Foo", "bar")
}

func Bor(b *testing.B) {
	// reported by default (signature based)
	os.Setenv("Foo", "bar")
}

func Bur(tb testing.TB) {
	// reported by default (signature based)
	os.Setenv("Foo", "bar")
}

foo_test.go

package sandbox

import (
	"os"
	"testing"
)

// never reported
var fii = os.Setenv("Foo", "bar")

func foo() {
	// reported only with the option "all" enabled
	os.Setenv("Foo", "bar")
}

func TestMain(m *testing.M) {
	// never report `TestMain(m *testing.M)` functions
	os.Setenv("Foo", "bar")
	os.Exit(m.Run())
}

func TestName(t *testing.T) {
	// reported by default (signature based)
	os.Setenv("Foo", "bar")
}

func BenchmarkName(b *testing.B) {
	// reported by default (signature based)
	os.Setenv("Foo", "bar")
	for i := 0; i < b.N; i++ {
		b.Name()
	}
}

[sivchari]

@ldez

Error:     analysistest.go:448: a/a_test.go:23:2: diagnostic "os.Setenv() can be replaced by `t.Setenv()` in TestF" does not match pattern "os.Setenv() can be replaced by `t.Setenv()` in TestF"
Error:     analysistest.go:448: a/a_test.go:24:2: diagnostic "os.Setenv() can be replaced by `t.Setenv()` in TestF" does not match pattern "os.Setenv() can be replaced by `t.Setenv()` in TestF"
Error:     analysistest.go:448: a/a_test.go:31:2: diagnostic "os.Setenv() can be replaced by `b.Setenv()` in BenchmarkF" does not match pattern "os.Setenv() can be replaced by `b.Setenv()` in BenchmarkF"
Error:     analysistest.go:448: a/a_test.go:32:2: diagnostic "os.Setenv() can be replaced by `b.Setenv()` in BenchmarkF" does not match pattern "os.Setenv() can be replaced by `b.Setenv()` in BenchmarkF"
Error:     analysistest.go:448: a/a_test.go:38:2: diagnostic "os.Setenv() can be replaced by `tb.Setenv()` in testTB" does not match pattern "os.Setenv() can be replaced by `tb.Setenv()` in testTB"
Error:     analysistest.go:512: a/a_test.go:23: no diagnostic was reported matching "os.Setenv() can be replaced by `t.Setenv()` in TestF"
Error:     analysistest.go:512: a/a_test.go:24: no diagnostic was reported matching "os.Setenv() can be replaced by `t.Setenv()` in TestF"
Error:     analysistest.go:512: a/a_test.go:31: no diagnostic was reported matching "os.Setenv() can be replaced by `b.Setenv()` in BenchmarkF"
Error:     analysistest.go:512: a/a_test.go:32: no diagnostic was reported matching "os.Setenv() can be replaced by `b.Setenv()` in BenchmarkF"
Error:     analysistest.go:512: a/a_test.go:38: no diagnostic was reported matching "os.Setenv() can be replaced by `tb.Setenv()` in testTB"

detail: sivchari/tenv#12

if you have a time, please teach me that why this test is failed.

[ldez]

Please re-read my message there are 2 files: foo.go and foo_test.go

[sivchari]

sorry, I fixed it.
please review.