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

ARR4N · 2024-07-04T16:57:50Z

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.

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

…tside of `_test.go` files

…ult linters

This reverts commit b7f0222 because it breaks a number of CI tests. There remains an open issue to do this (in a different PR).

StephenButtolph

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

still looking

StephenButtolph · 2024-07-05T18:00:19Z

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 · 2024-07-05T18:01:21Z

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 · 2024-07-08T10:29:36Z

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 · 2024-07-08T15:19:23Z

.vscode/settings.json

+{
+    "gopls": {
+        "build.buildFlags": [
+            // Context: https://github.com/ava-labs/avalanchego/pull/3173


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

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

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

StephenButtolph · 2024-07-08T16:41:15Z

Maru is OOO - going to merge.

This allows us to get more useful output when there is a change; e.g.,: ```console $ .github/scripts/verify_golangci_yaml_changes.sh 'ffi/.golangci.yaml' has unexpected changes from AvalancheGo. View the upstream changes at: https://github.com/ava-labs/avalanchego/commits/master/.golangci.yml and apply them if necessary. Current changes: --- .github/.golangci.yaml 2025-08-02 10:04:10 +++ ffi/.golangci.yaml 2025-08-02 10:01:55 @@ -12,7 +12,7 @@ # # Allowed values: readonly|vendor|mod # By default, it isn't set. - modules-download-mode: mod + modules-download-mode: readonly # Include non-test files tagged as test-only. # Context: ava-labs/avalanchego#3173 ``` Additionally, we can run `apply` to apply our patch against the upstream file and refresh the patch file: ```console $ .github/scripts/verify_golangci_yaml_changes.sh apply patching file '.github/.golangci.yaml' Successfully applied the patch from .github/.golangci.yaml.patch to ffi/.golangci.yaml Updated expected changes in .github/.golangci.yaml.patch ```

I also got bored and updated the script. This allows us to get more useful output when there is a change; e.g.,: ```console $ .github/scripts/verify_golangci_yaml_changes.sh 'ffi/.golangci.yaml' has unexpected changes from AvalancheGo. View the upstream changes at: https://github.com/ava-labs/avalanchego/commits/master/.golangci.yml and apply them if necessary. Current changes: --- .github/.golangci.yaml 2025-08-02 10:04:10 +++ ffi/.golangci.yaml 2025-08-02 10:01:55 @@ -12,7 +12,7 @@ # # Allowed values: readonly|vendor|mod # By default, it isn't set. - modules-download-mode: mod + modules-download-mode: readonly # Include non-test files tagged as test-only. # Context: ava-labs/avalanchego#3173 ``` Additionally, we can run `apply` to apply our patch against the upstream file and refresh the patch file: ```console $ .github/scripts/verify_golangci_yaml_changes.sh apply patching file '.github/.golangci.yaml' Successfully applied the patch from .github/.golangci.yaml.patch to ffi/.golangci.yaml Updated expected changes in .github/.golangci.yaml.patch ``` fixes #1164

build(tests): require //go:build test tag if importing "testing" ou…

f8b785c

…tside of `_test.go` files

ARR4N changed the title ~~build(tests): require //go:build test tag if importing "testing" outside of _test.go files~~ build(tests): require //go:build test tag if importing testing outside of _test.go files Jul 4, 2024

fix: add test tag to fuzzing

0f630cc

ARR4N mentioned this pull request Jul 4, 2024

Import of testing in production binary #3174

Open

ARR4N added 5 commits July 4, 2024 18:27

fix: temporary workaround for bootstrap test

007dd75

fix: exclude files needed for compiling AvalancheGo binary

02b44c0

fix: it builds! (Yes, I'm using CI as a build farm)

cdd54cb

chore: run golangci-lint with test tag + include new test in defa…

7aec2b1

…ult linters

fix(lint): explicit find root

24b3cac

ARR4N marked this pull request as ready for review July 4, 2024 18:51

ARR4N requested review from abi87, danlaine, dhrubabasu and StephenButtolph as code owners July 4, 2024 18:51

ARR4N requested review from marun and removed request for dhrubabasu, danlaine and abi87 July 4, 2024 18:59

ARR4N added 2 commits July 5, 2024 09:36

chore: add test build tag to files importing testify/*

09be7d2

feat(linter): detect files importing testify/*

e410926

ARR4N requested a review from joshua-kim as a code owner July 5, 2024 08:46

ARR4N changed the title ~~build(tests): require //go:build test tag if importing testing outside of _test.go files~~ build(tests): require //go:build test tag if importing test packages outside of _test.go files Jul 5, 2024

ARR4N added 4 commits July 5, 2024 10:06

fix: placate ginkgo

cdaf587

fix: shell linting

d9af4ad

fix: more ginkgo --tags test

42251a6

Merge branch 'master' into arr4n/test-only-build-tags

b030d5d

ARR4N mentioned this pull request Jul 5, 2024

Build constraints in generated files uber-go/mock#190

Closed

ARR4N added 3 commits July 5, 2024 15:30

refactor: remove test || !test workaround

3a91b1d

refactor: remove final test || !test workaround

b7f0222

Revert "refactor: remove final test || !test workaround"

6abc968

This reverts commit b7f0222 because it breaks a number of CI tests. There remains an open issue to do this (in a different PR).

darioush approved these changes Jul 5, 2024

View reviewed changes

StephenButtolph previously approved these changes Jul 5, 2024

View reviewed changes

StephenButtolph assigned ARR4N Jul 6, 2024

StephenButtolph added the cleanup Code quality improvement label Jul 6, 2024

StephenButtolph added this to the v1.11.10 milestone Jul 6, 2024

chore: VSCode gopls and testing tags

f70bfb0

chore: revert removal of .vscode* from .gitignore

c531c88

ARR4N mentioned this pull request Jul 8, 2024

build(tests): require //go:build test tag if importing test packages outside of _test.go files ava-labs/coreth#597

Draft

tsachiherman reviewed Jul 8, 2024

View reviewed changes

StephenButtolph approved these changes Jul 8, 2024

View reviewed changes

StephenButtolph added this pull request to the merge queue Jul 8, 2024

Merged via the queue into master with commit 2af3890 Jul 8, 2024

StephenButtolph deleted the arr4n/test-only-build-tags branch July 8, 2024 16:55

ARR4N mentioned this pull request Jul 16, 2024

Implement action test suite helper ava-labs/hypersdk#1139

Merged

ARR4N mentioned this pull request Jul 26, 2024

refactor: introduce *test packages in lieu of //go:build test #3238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

ARR4N commented Jul 4, 2024 •

edited

Loading

Uh oh!

StephenButtolph left a comment

Uh oh!

StephenButtolph commented Jul 5, 2024

Uh oh!

StephenButtolph commented Jul 5, 2024 •

edited

Loading

Uh oh!

ARR4N commented Jul 8, 2024

Uh oh!

tsachiherman Jul 8, 2024

Uh oh!

StephenButtolph Jul 8, 2024

Uh oh!

StephenButtolph Jul 8, 2024

Uh oh!

StephenButtolph commented Jul 8, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

ARR4N commented Jul 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why this should be merged

How this works

How this was tested

Future work

Footnotes

Uh oh!

StephenButtolph left a comment

Choose a reason for hiding this comment

Uh oh!

StephenButtolph commented Jul 5, 2024

Uh oh!

StephenButtolph commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ARR4N commented Jul 8, 2024

Uh oh!

tsachiherman Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

StephenButtolph Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

StephenButtolph Jul 8, 2024

Choose a reason for hiding this comment

Uh oh!

StephenButtolph commented Jul 8, 2024

Uh oh!

Uh oh!

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

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

ARR4N commented Jul 4, 2024 •

edited

Loading

StephenButtolph commented Jul 5, 2024 •

edited

Loading