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

ARR4N · 2024-07-26T16:27:07Z

Why this should be merged

See #3173 original rationale. Using build tags is proving problematic so this is the beginning of a refactor to move test-only code out of production packages.

How this works

Mechanical refactoring. For package foo:

Move all test-only code into foo/footest
1. These are typically the foo/test_*.go files
Update all tests depending on the code now in footest
If a foo/*_test.go file results in a circular dependency:
1. Change the offending file's package to foo_test instead of foo (not footest without the underscore); this will be compiled separately and then linked into foo's test binary (source)
2. (optional) Dot-import foo into foo_test. In all other circumstances we wouldn't expect to qualify the foo.* exported identifiers by prepending the package name, so the dot import mirrors this. This usage of dot imports MUST NOT be considered a precedent for introducing the pattern for any other reason.

In some cases no refactoring was necessary as no other packages depended on the test-only code. These were merely renamed to include a _test.go suffix.

How this was tested

CI

cache/lru_cache_test.go

… to test files

darioush · 2024-08-01T15:54:30Z

.golangci.yml

      - prefix(github.com/ava-labs/avalanchego)
      - alias
+      - dot


ni: seems can be put back

I think having the dot import be the bottom makes sense

This was a deliberate change to make the dot imports more obvious (I think it should be done for "blank" underscore imports too, but that's beyond this PR).

Both dot and underscore imports fundamentally change the way the rest of the code works, but in a way that's easily missed. Having them called out through placement is one way to mitigate this.

darioush · 2024-08-01T15:58:56Z

codec/codectest/codectest.go

@@ -127,7 +127,7 @@ type myStruct struct {
 }

 // Test marshaling/unmarshaling a complicated struct
-func TestStruct(codec GeneralCodec, t testing.TB) {
+func TestStruct(cdec codec.GeneralCodec, t testing.TB) {


nit: could we call the variable c (or generalCodec)? cdec seemed like a misspelling to me initially

Either that or alias the package import?

seemed like a misspelling to me initially

Hehe I didn't like this either, and was hoping nobody would care.

Either that or alias the package import?

Good idea. There's actually prior art in this vein so I'll rename the package to codecpkg.

marun

What's here LGTM apart from the few nits identified by @darioush. Are you intending to update the linter in this PR?

marun · 2024-08-01T20:29:31Z

chains/atomic/atomictest/shared_memory.go


 import (
 	"math/rand"
 	"testing"

 	"github.com/stretchr/testify/require"

+	"github.com/ava-labs/avalanchego/chains/atomic"


(No action required) So the dot import is restricted to package renames in test files (e.g. atomic_test.go has package atomic_test and is allowed to use dot imports)? I guess that makes sense, though allowing dot import of a target package by its subordinate test package might not be so bad.

marun · 2024-08-01T20:31:04Z

codec/codectest/codectest.go

@@ -127,7 +127,7 @@ type myStruct struct {
 }

 // Test marshaling/unmarshaling a complicated struct
-func TestStruct(codec GeneralCodec, t testing.TB) {
+func TestStruct(cdec codec.GeneralCodec, t testing.TB) {


Either that or alias the package import?

marun · 2024-08-01T20:43:39Z

snow/networking/benchlist/benchable_test.go

@@ -1,8 +1,6 @@
 // Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved.
 // See the file LICENSE for licensing terms.

-//go:build test
-


(No action required) Why is this file suffixed with _test if it doesn't contain a test? Also not clear why TestBenchable should be exported if it's defined in a test file from which nothing can be imported.

Why is this file suffixed with _test if it doesn't contain a test?

Because it contains code that is required by the other benchlist tests but by nothing else. The file was just incorrectly named. From the PR description: In some cases no refactoring was necessary as no other packages depended on the test-only code. These were merely renamed to include a _test.go suffix.

Also not clear why TestBenchable should be exported if it's defined in a test file from which nothing can be imported.

Typically it wouldn't be, and it has no effect either way. I'm just trying to keep this PR to the minimum required to remove the tags. Things like this and the stuttering that Stephen mentions are deliberately out of scope, lest the delta blows up.

StephenButtolph · 2024-08-02T01:47:11Z

cache/cachetest/cacher.go

-//go:build test
-
-package cache
+package cachetest


I think there are a number of fields that stutter based on this rename - do we want to rename things like cachetest.TestIntSize to cachetest.IntSize?

If we want to move that into a followup because the change right now is "methodical" whereas this would be a bit more of a "case-by-case" basis... I think that's fine... But I think it should be a fast-follow... Not an amorphous TODO (I'd personally be in-favor of just cleaning them up in this PR)

I think this applies to most (if not all) of the test packages in this refactor

They were deliberately ignored to be allow this PR to have the smallest possible footprint required to remove test tags. I'll do them as an immediate PR (might even be ready before you re-review this one).

(might even be ready before you re-review this one)

I take this back. There are sooooo many of them.

StephenButtolph · 2024-08-02T01:47:52Z

.golangci.yml

      - prefix(github.com/ava-labs/avalanchego)
      - alias
+      - dot


I think having the dot import be the bottom makes sense

StephenButtolph · 2024-08-02T02:05:24Z

snow/validators/statetest/statetest.go

Shouldn't this be the validatorstest package? (side note - hate that it is plural)

Ugh that breaks the coreth dependency.

Style-guide recommendation: https://google.github.io/styleguide/go/decisions#:~:text=If%20you%20need%20to%20import%20a%20package%20whose,used%20when%20such%20a%20variable%20is%20in%20scope.

ARR4N · 2024-08-02T10:52:12Z

What's here LGTM apart from the few nits identified by @darioush. Are you intending to update the linter in this PR?

Ahh, yes, thanks for the reminder.

ARR4N added 3 commits July 26, 2024 17:26

refactor: introduce cache/cachtest

efa5a74

chore: include changes forgotten in last commit

b843a56

refactor: introduce chains/atomic/atomictest

1012187

StephenButtolph reviewed Jul 26, 2024

View reviewed changes

cache/lru_cache_test.go Show resolved Hide resolved

StephenButtolph reviewed Jul 26, 2024

View reviewed changes

cache/lru_cache_test.go Show resolved Hide resolved

ARR4N added 6 commits July 26, 2024 18:25

refactor: introduce codec/codectest

7d552b8

refactor: introduce database/dbtest

34fd483

refactor: introduce ids/idstest

ef718a1

fix: x/merkledb tests use database/dbtest

bda8137

chore: move dot imports to end to bring attention to them

9a301dd

chore: fix import ordering and pray to the CI linter gods

bd9fea9

ARR4N marked this pull request as ready for review July 26, 2024 18:58

ARR4N requested review from danlaine, darioush, dboehm-avalabs, joshua-kim and ceyonur as code owners July 26, 2024 18:58

ARR4N requested review from StephenButtolph, marun and yacovm and removed request for darioush, danlaine, ceyonur, joshua-kim and dboehm-avalabs July 26, 2024 18:59

ARR4N added 5 commits August 1, 2024 09:05

refactor: introduce wallet/subnet/primary/common/utxotest

0e96fa4

refactor: introduce vms/platformvm/warp/signertest

e38117f

refactor: introduce snow/validators/statetest

82991fd

refactor: introduce snow/networking/sender/sendertest

3a095e1

refactor: move snow/networking/benchlist test-only code to test file

28f2de9

ARR4N requested review from abi87 and dhrubabasu as code owners August 1, 2024 09:12

ARR4N added 7 commits August 1, 2024 10:17

Merge branch 'master' into arr4n/test-packages-over-tags

891e7fe

refactor: introduce snow/engine/avalanche/vertex/vertextest

7e2f1d7

refactor: move snow/engine/avalanche/bootstrap/queue test-only code…

37b9e20

… to test files

refactor: move snow/consensus/snowball test-only code to test file

9b9a5da

chore: depend on coreth tag compatible with these changes #monorepo

3c505f9

fix: make gci happy

52bf535

chore: run go mod tidy

9430f4c

darioush approved these changes Aug 1, 2024

View reviewed changes

marun reviewed Aug 1, 2024

View reviewed changes

StephenButtolph reviewed Aug 2, 2024

View reviewed changes

ARR4N added 2 commits August 2, 2024 11:45

refactor: alias codecpkg in codectest to use codec as a variable

1377cb4

Style-guide recommendation: https://google.github.io/styleguide/go/decisions#:~:text=If%20you%20need%20to%20import%20a%20package%20whose,used%20when%20such%20a%20variable%20is%20in%20scope.

chore: remove linter exception for go:build test tags

d44ea82

ARR4N added 4 commits August 2, 2024 12:01

chore: rename statetest to validatorstest

968e3ef

Merge branch 'master' into arr4n/test-packages-over-tags

17c63ff

chore: depend on coreth tag compatible with these changes #monorepo

2a031b2

fix: test merged from master that uses new package

3203eac

ARR4N requested a review from StephenButtolph August 2, 2024 12:00

ARR4N mentioned this pull request Aug 2, 2024

refactor: rename *test.Test* identifiers #3260

Merged

StephenButtolph added this to the v1.11.11 milestone Aug 2, 2024

StephenButtolph added the cleanup Code quality improvement label Aug 2, 2024

StephenButtolph assigned ARR4N Aug 2, 2024

StephenButtolph approved these changes Aug 2, 2024

View reviewed changes

StephenButtolph added this pull request to the merge queue Aug 2, 2024

Merged via the queue into master with commit c2f1f46 Aug 2, 2024
20 checks passed

StephenButtolph deleted the arr4n/test-packages-over-tags branch August 2, 2024 15:44

ARR4N mentioned this pull request Aug 12, 2024

Fix test tags ava-labs/coreth#631

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ARR4N commented Jul 26, 2024 •

edited

Loading

darioush Aug 1, 2024

StephenButtolph Aug 2, 2024

ARR4N Aug 2, 2024

darioush Aug 1, 2024 •

edited

Loading

marun Aug 1, 2024

ARR4N Aug 2, 2024

marun left a comment

marun Aug 1, 2024

marun Aug 1, 2024

marun Aug 1, 2024

ARR4N Aug 2, 2024

StephenButtolph Aug 2, 2024

ARR4N Aug 2, 2024

ARR4N Aug 2, 2024

ARR4N Aug 2, 2024

StephenButtolph Aug 2, 2024

StephenButtolph Aug 2, 2024

ARR4N Aug 2, 2024

ARR4N Aug 2, 2024

ARR4N commented Aug 2, 2024

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

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

Conversation

ARR4N commented Jul 26, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darioush Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ARR4N commented Aug 2, 2024

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

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

ARR4N commented Jul 26, 2024 •

edited

Loading

darioush Aug 1, 2024 •

edited

Loading