From 20f9cb6dcfb3f5fcc9be598eb3b8a0210e215944 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 11:37:15 +0200 Subject: [PATCH 01/12] golangci-lint: disable depguard linter Current versions of this linter default to no allowing third-party dependencies by default (only stdlib and golang.org/x/sys). As there's currently no configuration set to prevent a specific import, this results in some imports being flagged; fs/manifest_test.go:11:2: import 'github.com/google/go-cmp/cmp' is not allowed from list 'Main' (depguard) "github.com/google/go-cmp/cmp" ^ assert/assert.go:96:2: import 'github.com/google/go-cmp/cmp' is not allowed from list 'Main' (depguard) gocmp "github.com/google/go-cmp/cmp" ^ assert/assert_test.go:8:2: import 'github.com/google/go-cmp/cmp' is not allowed from list 'Main' (depguard) gocmp "github.com/google/go-cmp/cmp" ^ Signed-off-by: Sebastiaan van Stijn --- .golangci.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index 8923200..164bd51 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -32,7 +32,6 @@ linters: disable-all: true enable: - bodyclose - - depguard - dogsled - errcheck - errorlint From 74270958c2327398a40728846e8f7d1b3f2be0e6 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 11:39:20 +0200 Subject: [PATCH 02/12] assert/cmp: fix "nolintlint" false positive The nolintlint linter considers the `nolint` to be redundant; assert/cmp/compare.go:251:2: directive `//nolint:errorlint // unwrapping is not appropriate here` is unused for linter "errorlint" (nolintlint) //nolint:errorlint // unwrapping is not appropriate here ^ But depending on the go version, it may trigger a warning: assert/cmp/compare.go:252:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint) if _, ok := err.(causer); ok { ^ Signed-off-by: Sebastiaan van Stijn --- assert/cmp/compare.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assert/cmp/compare.go b/assert/cmp/compare.go index 118844f..3ced3de 100644 --- a/assert/cmp/compare.go +++ b/assert/cmp/compare.go @@ -248,7 +248,7 @@ type causer interface { } func formatErrorMessage(err error) string { - //nolint:errorlint // unwrapping is not appropriate here + //nolint:errorlint,nolintlint // unwrapping is not appropriate here if _, ok := err.(causer); ok { return fmt.Sprintf("%q\n%+v", err, err) } From 10b8a7534899d6ab585b0792f239908c5e16eb18 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 11:45:12 +0200 Subject: [PATCH 03/12] assert: fix empty-block (revive) assert/assert_test.go:108:3: empty-block: this block is empty, you can remove it (revive) for range []int{1, 2, 3, 4} { } Signed-off-by: Sebastiaan van Stijn --- assert/assert_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/assert/assert_test.go b/assert/assert_test.go index 97ec8dd..6c480b7 100644 --- a/assert/assert_test.go +++ b/assert/assert_test.go @@ -105,12 +105,14 @@ func TestAssertWithBoolMultiLineFailure(t *testing.T) { fakeT := &fakeTestingT{} Assert(fakeT, func() bool { - for range []int{1, 2, 3, 4} { + for i := range []int{1, 2, 3, 4} { + _ = i } return false }()) expectFailNowed(t, fakeT, `assertion failed: expression is false: func() bool { - for range []int{1, 2, 3, 4} { + for i := range []int{1, 2, 3, 4} { + _ = i } return false }()`) From 996232d86427fcb9dcf93bb990102c65f4ee832c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 11:55:23 +0200 Subject: [PATCH 04/12] internal/source: don't return nil result without an error (nilnil) internal/source/source.go:75:2: return both the `nil` error and invalid value: use a sentinel error instead (nilnil) return nil, nil ^ Signed-off-by: Sebastiaan van Stijn --- internal/source/source.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/internal/source/source.go b/internal/source/source.go index df61234..9ac4bfa 100644 --- a/internal/source/source.go +++ b/internal/source/source.go @@ -72,7 +72,7 @@ func getNodeAtLine(fileset *token.FileSet, astFile ast.Node, lineNum int) (ast.N return node, err } } - return nil, nil + return nil, errors.New("failed to find expression") } func scanToLine(fileset *token.FileSet, node ast.Node, lineNum int) ast.Node { @@ -92,11 +92,8 @@ func scanToLine(fileset *token.FileSet, node ast.Node, lineNum int) ast.Node { func getCallExprArgs(fileset *token.FileSet, astFile ast.Node, line int) ([]ast.Expr, error) { node, err := getNodeAtLine(fileset, astFile, line) - switch { - case err != nil: + if err != nil { return nil, err - case node == nil: - return nil, fmt.Errorf("failed to find an expression") } debug("found node: %s", debugFormatNode{node}) @@ -104,7 +101,7 @@ func getCallExprArgs(fileset *token.FileSet, astFile ast.Node, line int) ([]ast. visitor := &callExprVisitor{} ast.Walk(visitor, node) if visitor.expr == nil { - return nil, errors.New("failed to find call expression") + return nil, errors.New("failed to find an expression") } debug("callExpr: %s", debugFormatNode{visitor.expr}) return visitor.expr.Args, nil From 26a77530800575391725d0987438b0fb6f8dade8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:02:38 +0200 Subject: [PATCH 05/12] internal/source: rename variables that collided with builtins (revive) "cap" is now a builtin: internal/source/source_test.go:50:2: redefines-builtin-id: redefinition of the built-in function cap (revive) cap := &capture{} ^ internal/source/source_test.go:73:2: redefines-builtin-id: redefinition of the built-in function cap (revive) cap := &capture{} ^ internal/source/source_test.go:86:2: redefines-builtin-id: redefinition of the built-in function cap (revive) cap := &capture{} ^ Signed-off-by: Sebastiaan van Stijn --- internal/source/source_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/internal/source/source_test.go b/internal/source/source_test.go index 3c21819..aa30304 100644 --- a/internal/source/source_test.go +++ b/internal/source/source_test.go @@ -47,13 +47,13 @@ func shim(_, _, _ string) (string, error) { func TestFormattedCallExprArg_InDefer(t *testing.T) { skip.If(t, isGoVersion18) - cap := &capture{} + c := &capture{} func() { - defer cap.shim("first", "second") + defer c.shim("first", "second") }() - assert.NilError(t, cap.err) - assert.Equal(t, cap.value, `"second"`) + assert.NilError(t, c.err) + assert.Equal(t, c.value, `"second"`) } func isGoVersion18() bool { @@ -70,25 +70,25 @@ func (c *capture) shim(_, _ string) { } func TestFormattedCallExprArg_InAnonymousDefer(t *testing.T) { - cap := &capture{} + c := &capture{} func() { fmt.Println() defer fmt.Println() - defer func() { cap.shim("first", "second") }() + defer func() { c.shim("first", "second") }() }() - assert.NilError(t, cap.err) - assert.Equal(t, cap.value, `"second"`) + assert.NilError(t, c.err) + assert.Equal(t, c.value, `"second"`) } func TestFormattedCallExprArg_InDeferMultipleDefers(t *testing.T) { skip.If(t, isGoVersion18) - cap := &capture{} + c := &capture{} func() { fmt.Println() defer fmt.Println() - defer cap.shim("first", "second") + defer c.shim("first", "second") }() - assert.ErrorContains(t, cap.err, "ambiguous call expression") + assert.ErrorContains(t, c.err, "ambiguous call expression") } From b0b714c572d9e6421171c02102fd482dc7d9cfcb Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:05:05 +0200 Subject: [PATCH 06/12] fs: don't name unused parameters (revive) fs/report_test.go:187:19: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive) matcher := func(b []byte) CompareResult { ^ fs/report_test.go:196:19: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive) matcher := func(b []byte) CompareResult { ^ fs/report.go:107:46: printf: non-constant format string in call to gotest.tools/v3/fs.existenceProblem (govet) p = append(p, existenceProblem("content", r.FailureMessage())) ^ Signed-off-by: Sebastiaan van Stijn --- fs/report_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/report_test.go b/fs/report_test.go index 389d608..7c962c2 100644 --- a/fs/report_test.go +++ b/fs/report_test.go @@ -184,7 +184,7 @@ func TestMatchFileContent(t *testing.T) { defer dir.Remove() t.Run("content matches", func(t *testing.T) { - matcher := func(b []byte) CompareResult { + matcher := func([]byte) CompareResult { return is.ResultSuccess } manifest := Expected(t, @@ -193,7 +193,7 @@ func TestMatchFileContent(t *testing.T) { }) t.Run("content does not match", func(t *testing.T) { - matcher := func(b []byte) CompareResult { + matcher := func([]byte) CompareResult { return is.ResultFailure("data content differs from expected") } manifest := Expected(t, From 7b3f988818384ccd959d1c5d8bf9d1b7ab2c30c0 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:07:19 +0200 Subject: [PATCH 07/12] assert: don't name unused parameters (revive) assert/assert_test.go:249:12: unused-parameter: parameter 'b' seems to be unused, consider removing or renaming it as _ (revive) f := func(b bool) {} ^ Signed-off-by: Sebastiaan van Stijn --- assert/assert_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assert/assert_test.go b/assert/assert_test.go index 6c480b7..6deede8 100644 --- a/assert/assert_test.go +++ b/assert/assert_test.go @@ -246,7 +246,7 @@ func TestCheckEqualFailure(t *testing.T) { func TestCheck_MultipleFunctionsOnTheSameLine(t *testing.T) { fakeT := &fakeTestingT{} - f := func(b bool) {} + f := func(bool) {} f(Check(fakeT, false)) // TODO: update the expected when there is a more correct fix expectFailed(t, fakeT, From 89b340171cc369f27bf39be72a923bd5ee704eb5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:21:35 +0200 Subject: [PATCH 08/12] fs: fix printf: non-constant format string in call (govet) Current versions of govet check for printf functions called with a format string, but no arguments. This results in linting errors: fs/report.go:107:46: printf: non-constant format string in call to gotest.tools/v3/fs.existenceProblem (govet) p = append(p, existenceProblem("content", r.FailureMessage())) ^ This patch changes existenceProblem to use format.Message, which is used elsewhere for similar situations. Signed-off-by: Sebastiaan van Stijn --- fs/report.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/report.go b/fs/report.go index 952aa26..773df21 100644 --- a/fs/report.go +++ b/fs/report.go @@ -50,8 +50,8 @@ func errProblem(reason string, err error) problem { return problem(fmt.Sprintf("%s: %s", reason, err)) } -func existenceProblem(filename, reason string, args ...interface{}) problem { - return problem(filename + ": " + fmt.Sprintf(reason, args...)) +func existenceProblem(filename string, msgAndArgs ...interface{}) problem { + return problem(filename + ": " + format.Message(msgAndArgs...)) } func eqResource(x, y resource) []problem { From 8375bfbda1917f56048adaed59744a93535a79c4 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:26:32 +0200 Subject: [PATCH 09/12] poll: don't name unused parameters (revive) poll/example_test.go:38:16: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive) check := func(t poll.LogT) poll.Result { ^ poll/poll_test.go:21:21: unused-parameter: parameter 'args' seems to be unused, consider removing or renaming it as _ (revive) func (t *fakeT) Log(args ...interface{}) {} ^ poll/poll_test.go:23:22: unused-parameter: parameter 'format' seems to be unused, consider removing or renaming it as _ (revive) func (t *fakeT) Logf(format string, args ...interface{}) {} ^ poll/poll_test.go:28:16: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive) check := func(t LogT) Result { ^ poll/poll.go:154:18: printf: non-constant format string in call to gotest.tools/v3/poll.Continue (govet) return Continue(buf.String()) ^ Signed-off-by: Sebastiaan van Stijn --- poll/example_test.go | 2 +- poll/poll_test.go | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/poll/example_test.go b/poll/example_test.go index 2e3c32d..21690d0 100644 --- a/poll/example_test.go +++ b/poll/example_test.go @@ -35,7 +35,7 @@ func isDesiredState() bool { return false } func getState() string { return "" } func ExampleSettingOp() { - check := func(t poll.LogT) poll.Result { + check := func(poll.LogT) poll.Result { if isDesiredState() { return poll.Success() } diff --git a/poll/poll_test.go b/poll/poll_test.go index 36bd457..e0ba720 100644 --- a/poll/poll_test.go +++ b/poll/poll_test.go @@ -18,14 +18,14 @@ func (t *fakeT) Fatalf(format string, args ...interface{}) { panic("exit wait on") } -func (t *fakeT) Log(args ...interface{}) {} +func (t *fakeT) Log(...interface{}) {} -func (t *fakeT) Logf(format string, args ...interface{}) {} +func (t *fakeT) Logf(string, ...interface{}) {} func TestWaitOn(t *testing.T) { counter := 0 end := 4 - check := func(t LogT) Result { + check := func(LogT) Result { if counter == end { return Success() } @@ -40,7 +40,7 @@ func TestWaitOn(t *testing.T) { func TestWaitOnWithTimeout(t *testing.T) { fakeT := &fakeT{} - check := func(t LogT) Result { + check := func(LogT) Result { return Continue("not done") } @@ -53,7 +53,7 @@ func TestWaitOnWithTimeout(t *testing.T) { func TestWaitOnWithCheckTimeout(t *testing.T) { fakeT := &fakeT{} - check := func(t LogT) Result { + check := func(LogT) Result { time.Sleep(1 * time.Second) return Continue("not done") } @@ -65,7 +65,7 @@ func TestWaitOnWithCheckTimeout(t *testing.T) { func TestWaitOnWithCheckError(t *testing.T) { fakeT := &fakeT{} - check := func(t LogT) Result { + check := func(LogT) Result { return Error(fmt.Errorf("broke")) } @@ -76,7 +76,7 @@ func TestWaitOnWithCheckError(t *testing.T) { func TestWaitOn_WithCompare(t *testing.T) { fakeT := &fakeT{} - check := func(t LogT) Result { + check := func(LogT) Result { return Compare(cmp.Equal(3, 4)) } From f5aded3409c862cd54e8eb96b60ecbe87eb4e2c2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:29:06 +0200 Subject: [PATCH 10/12] assert: TestPathDebug: fix missing assertion assert/opt/opt_test.go:254:20: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive) func TestPathDebug(t *testing.T) { ^ Signed-off-by: Sebastiaan van Stijn --- assert/opt/opt_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/assert/opt/opt_test.go b/assert/opt/opt_test.go index c036d18..56019a3 100644 --- a/assert/opt/opt_test.go +++ b/assert/opt/opt_test.go @@ -261,5 +261,5 @@ func TestPathDebug(t *testing.T) { "label1": {}, }, } - gocmp.Equal(fixture, fixture, gocmp.FilterPath(PathDebug, gocmp.Ignore())) + assert.Check(t, gocmp.Equal(fixture, fixture, gocmp.FilterPath(PathDebug, gocmp.Ignore()))) } From 5500d158921dc6b5f79cfce8c28031d25e13fe2a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:32:21 +0200 Subject: [PATCH 11/12] x/subtest: don't name unused parameters (revive) x/subtest/example_test.go:47:23: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive) func startFakeService(t subtest.TestContext) string { ^ Signed-off-by: Sebastiaan van Stijn --- x/subtest/example_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/subtest/example_test.go b/x/subtest/example_test.go index b4365e0..1e0cf0a 100644 --- a/x/subtest/example_test.go +++ b/x/subtest/example_test.go @@ -44,7 +44,7 @@ func ExampleRun_tableTest() { } } -func startFakeService(t subtest.TestContext) string { +func startFakeService(_ subtest.TestContext) string { return "url" } From d21c5224dfd64238bac305bf55ef873384132dbe Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 22 Aug 2024 12:34:43 +0200 Subject: [PATCH 12/12] poll: fix printf: non-constant format string in call (govet) poll/poll.go:154:18: printf: non-constant format string in call to gotest.tools/v3/poll.Continue (govet) return Continue(buf.String()) ^ Signed-off-by: Sebastiaan van Stijn --- poll/poll.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/poll/poll.go b/poll/poll.go index cfd6d43..ab2e9da 100644 --- a/poll/poll.go +++ b/poll/poll.go @@ -151,7 +151,7 @@ func Compare(compare cmp.Comparison) Result { if assert.RunComparison(buf, assert.ArgsAtZeroIndex, compare) { return Success() } - return Continue(buf.String()) + return Continue("%v", buf.String()) } type logBuffer struct {