build(tests): require //go:build test tag if importing test packages outside of _test.go files

[ARR4N]

Why this should be merged

Including test-only code in non-test files risks leaking said functionality into production.

How this works

A new linter finds all *.go files that (i) aren't _test.go; and (ii) import test packages¹. If any of these files don't include a build-constraint tag of test then the linter fails. To the best of my knowledge, there's no native tag that's set when running tests, so all runs of go test and ginkgo now need to include -tags test.

How this was tested

Manually: I wrote the linter first as a way to find the offending files.

Future work

See #3174 for files flagged by the linter that have been temporarily ignored.

Footnotes

1. Currently detects testing and stretchr/testify/* packages. As mockgen doesn't add build tags, detection of mocks is disabled pending a PR to them. ↩

[StephenButtolph]

This change LGTM - will wait to hear if @marun has any thoughts w.r.t. the tests/ directory

still looking

[StephenButtolph]

This change seems to break the default behavior for running tests in VS code.. ex:

Running the test results in:

go test -timeout 30s -coverprofile=/var/folders/5l/z00z8zbd0dbd6lf8djdgl_000000gn/T/vscode-godMWCa1/go-code-cover -run ^TestGossiperGossip$ github.com/ava-labs/avalanchego/network/p2p/gossip -coverpkg=all

# github.com/ava-labs/avalanchego/network/p2p/gossip [github.com/ava-labs/avalanchego/network/p2p/gossip.test]
/Users/stephen/go/src/github.com/ava-labs/avalanchego/network/p2p/gossip/gossip_test.go:107:30: undefined: common.FakeSender
/Users/stephen/go/src/github.com/ava-labs/avalanchego/network/p2p/gossip/gossip_test.go:136:29: undefined: common.FakeSender
/Users/stephen/go/src/github.com/ava-labs/avalanchego/network/p2p/gossip/gossip_test.go:512:22: undefined: common.FakeSender
FAIL	github.com/ava-labs/avalanchego/network/p2p/gossip [build failed]
FAIL

Should we add a .vscode/settings.json file? It would specify:

{
    "go.buildTags": "test"
}

If we do this then we end up with:

go test -timeout 30s -tags test -coverprofile=/var/folders/5l/z00z8zbd0dbd6lf8djdgl_000000gn/T/vscode-go731twr/go-code-cover -run ^TestGossiperGossip$ github.com/ava-labs/avalanchego/network/p2p/gossip -coverpkg=all

ok  	github.com/ava-labs/avalanchego/network/p2p/gossip	0.876s	coverage: 9.8% of statements in all

which works as expected.

[StephenButtolph]

There is also a go.testTags option, but it seems like that doesn't fix the errors presented by the IDE 😞 (although it would fix the running of the tests)

[ARR4N]

There is also a go.testTags option, but it seems like that doesn't fix the errors presented by the IDE 😞 (although it would fix the running of the tests)

There's a gopls build-flag setting that fixes the IDE complaints:

{
    "gopls": {
        "build.buildFlags": [
            "--tags='test'",
        ],
    },
    "go.testTags": "test",
}

[tsachiherman]

I'm surprised to see that this works, but wouldn't touch it if it does.

[StephenButtolph]

Is is documented somewhere that this is supported? This also surprises me.

[StephenButtolph]

It is documented: https://code.visualstudio.com/docs/languages/json#_json-with-comments. TIL

[StephenButtolph]

Maru is OOO - going to merge.