From 266da4c3ab06599a0d01a4ca16b02e88a1462590 Mon Sep 17 00:00:00 2001 From: Luca Comellini Date: Tue, 24 Sep 2024 10:08:26 -0700 Subject: [PATCH] Add more linters (#382) --- .golangci.yml | 11 +++++ Makefile | 2 +- client/nginx_test.go | 82 +++++++++++++++++++++++++--------- tests/client_no_stream_test.go | 1 + tests/client_test.go | 12 +++++ 5 files changed, 86 insertions(+), 22 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6c887ac..0624468 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -5,24 +5,33 @@ linters-settings: ignore-generated-header: true rules: - name: blank-imports + - name: constant-logical-expr - name: context-as-argument - name: context-keys-type + - name: defer - name: dot-imports + - name: duplicated-imports - name: empty-block - name: error-naming - name: error-return - name: error-strings - name: errorf - name: exported + - name: import-shadowing - name: increment-decrement - name: indent-error-flow - name: package-comments - name: range + - name: range-val-address + - name: range-val-in-closure - name: receiver-naming - name: redefines-builtin-id + - name: string-of-int - name: superfluous-else - name: time-naming + - name: unchecked-type-assertion - name: unexported-return + - name: unnecessary-stmt - name: unreachable-code - name: unused-parameter - name: var-declaration @@ -60,6 +69,7 @@ linters: - ineffassign - intrange - makezero + - mirror - misspell - musttag - nilerr @@ -68,6 +78,7 @@ linters: - perfsprint - prealloc - predeclared + - paralleltest - reassign - revive - staticcheck diff --git a/Makefile b/Makefile index a458bd5..7db5c74 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ GOLANGCI_LINT_VERSION = v1.61.0 test: unit-test test-integration test-integration-no-stream-block clean lint: - docker run --pull always --rm -v $(shell pwd):/nginx-plus-go-client -w /nginx-plus-go-client -v $(shell go env GOCACHE):/cache/go -e GOCACHE=/cache/go -e GOLANGCI_LINT_CACHE=/cache/go -v $(shell go env GOPATH)/pkg:/go/pkg golangci/golangci-lint:$(GOLANGCI_LINT_VERSION) golangci-lint --color always run + go run github.com/golangci/golangci-lint/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION) run --fix unit-test: go test -v -shuffle=on -race client/*.go diff --git a/client/nginx_test.go b/client/nginx_test.go index 290abeb..86dff19 100644 --- a/client/nginx_test.go +++ b/client/nginx_test.go @@ -14,6 +14,7 @@ func TestDetermineUpdates(t *testing.T) { t.Parallel() maxConns := 1 tests := []struct { + name string updated []UpstreamServer nginx []UpstreamServer expectedToAdd []UpstreamServer @@ -57,6 +58,7 @@ func TestDetermineUpdates(t *testing.T) { Server: "10.0.0.2:80", }, }, + name: "replace all", }, { updated: []UpstreamServer{ @@ -95,6 +97,7 @@ func TestDetermineUpdates(t *testing.T) { Server: "10.0.0.1:80", }, }, + name: "add and delete", }, { updated: []UpstreamServer{ @@ -119,6 +122,7 @@ func TestDetermineUpdates(t *testing.T) { Server: "10.0.0.3:80", }, }, + name: "same", }, { // empty values @@ -153,14 +157,18 @@ func TestDetermineUpdates(t *testing.T) { MaxConns: &maxConns, }, }, + name: "update field and delete", }, } for _, test := range tests { - toAdd, toDelete, toUpdate := determineUpdates(test.updated, test.nginx) - if !reflect.DeepEqual(toAdd, test.expectedToAdd) || !reflect.DeepEqual(toDelete, test.expectedToDelete) || !reflect.DeepEqual(toUpdate, test.expectedToUpdate) { - t.Errorf("determineUpdates(%v, %v) = (%v, %v, %v)", test.updated, test.nginx, toAdd, toDelete, toUpdate) - } + t.Run(test.name, func(t *testing.T) { + t.Parallel() + toAdd, toDelete, toUpdate := determineUpdates(test.updated, test.nginx) + if !reflect.DeepEqual(toAdd, test.expectedToAdd) || !reflect.DeepEqual(toDelete, test.expectedToDelete) || !reflect.DeepEqual(toUpdate, test.expectedToUpdate) { + t.Errorf("determineUpdates(%v, %v) = (%v, %v, %v)", test.updated, test.nginx, toAdd, toDelete, toUpdate) + } + }) } } @@ -168,6 +176,7 @@ func TestStreamDetermineUpdates(t *testing.T) { t.Parallel() maxConns := 1 tests := []struct { + name string updated []StreamUpstreamServer nginx []StreamUpstreamServer expectedToAdd []StreamUpstreamServer @@ -211,6 +220,7 @@ func TestStreamDetermineUpdates(t *testing.T) { Server: "10.0.0.2:80", }, }, + name: "replace all", }, { updated: []StreamUpstreamServer{ @@ -249,6 +259,7 @@ func TestStreamDetermineUpdates(t *testing.T) { Server: "10.0.0.1:80", }, }, + name: "add and delete", }, { updated: []StreamUpstreamServer{ @@ -276,6 +287,7 @@ func TestStreamDetermineUpdates(t *testing.T) { Server: "10.0.0.3:80", }, }, + name: "same", }, { // empty values @@ -310,14 +322,18 @@ func TestStreamDetermineUpdates(t *testing.T) { MaxConns: &maxConns, }, }, + name: "update field and delete", }, } for _, test := range tests { - toAdd, toDelete, toUpdate := determineStreamUpdates(test.updated, test.nginx) - if !reflect.DeepEqual(toAdd, test.expectedToAdd) || !reflect.DeepEqual(toDelete, test.expectedToDelete) || !reflect.DeepEqual(toUpdate, test.expectedToUpdate) { - t.Errorf("determiteUpdates(%v, %v) = (%v, %v, %v)", test.updated, test.nginx, toAdd, toDelete, toUpdate) - } + t.Run(test.name, func(t *testing.T) { + t.Parallel() + toAdd, toDelete, toUpdate := determineStreamUpdates(test.updated, test.nginx) + if !reflect.DeepEqual(toAdd, test.expectedToAdd) || !reflect.DeepEqual(toDelete, test.expectedToDelete) || !reflect.DeepEqual(toUpdate, test.expectedToUpdate) { + t.Errorf("determiteUpdates(%v, %v) = (%v, %v, %v)", test.updated, test.nginx, toAdd, toDelete, toUpdate) + } + }) } } @@ -367,16 +383,20 @@ func TestAddPortToServer(t *testing.T) { } for _, test := range tests { - result := addPortToServer(test.address) - if result != test.expected { - t.Errorf("addPortToServer(%v) returned %v but expected %v for %v", test.address, result, test.expected, test.msg) - } + t.Run(test.msg, func(t *testing.T) { + t.Parallel() + result := addPortToServer(test.address) + if result != test.expected { + t.Errorf("addPortToServer(%v) returned %v but expected %v for %v", test.address, result, test.expected, test.msg) + } + }) } } func TestHaveSameParameters(t *testing.T) { t.Parallel() tests := []struct { + msg string server UpstreamServer serverNGX UpstreamServer expected bool @@ -385,11 +405,13 @@ func TestHaveSameParameters(t *testing.T) { server: UpstreamServer{}, serverNGX: UpstreamServer{}, expected: true, + msg: "empty", }, { server: UpstreamServer{ID: 2}, serverNGX: UpstreamServer{ID: 3}, expected: true, + msg: "different ID", }, { server: UpstreamServer{}, @@ -403,6 +425,7 @@ func TestHaveSameParameters(t *testing.T) { Down: &defaultDown, }, expected: true, + msg: "default values", }, { server: UpstreamServer{ @@ -428,35 +451,43 @@ func TestHaveSameParameters(t *testing.T) { Down: &defaultDown, }, expected: true, + msg: "same values", }, { server: UpstreamServer{SlowStart: "10s"}, serverNGX: UpstreamServer{}, expected: false, + msg: "different SlowStart", }, { server: UpstreamServer{}, serverNGX: UpstreamServer{SlowStart: "10s"}, expected: false, + msg: "different SlowStart 2", }, { server: UpstreamServer{SlowStart: "20s"}, serverNGX: UpstreamServer{SlowStart: "10s"}, expected: false, + msg: "different SlowStart 3", }, } for _, test := range tests { - result := haveSameParameters(test.server, test.serverNGX) - if result != test.expected { - t.Errorf("haveSameParameters(%v, %v) returned %v but expected %v", test.server, test.serverNGX, result, test.expected) - } + t.Run(test.msg, func(t *testing.T) { + t.Parallel() + result := haveSameParameters(test.server, test.serverNGX) + if result != test.expected { + t.Errorf("haveSameParameters(%v, %v) returned %v but expected %v", test.server, test.serverNGX, result, test.expected) + } + }) } } func TestHaveSameParametersForStream(t *testing.T) { t.Parallel() tests := []struct { + msg string server StreamUpstreamServer serverNGX StreamUpstreamServer expected bool @@ -465,11 +496,13 @@ func TestHaveSameParametersForStream(t *testing.T) { server: StreamUpstreamServer{}, serverNGX: StreamUpstreamServer{}, expected: true, + msg: "empty", }, { server: StreamUpstreamServer{ID: 2}, serverNGX: StreamUpstreamServer{ID: 3}, expected: true, + msg: "different ID", }, { server: StreamUpstreamServer{}, @@ -483,6 +516,7 @@ func TestHaveSameParametersForStream(t *testing.T) { Down: &defaultDown, }, expected: true, + msg: "default values", }, { server: StreamUpstreamServer{ @@ -508,24 +542,30 @@ func TestHaveSameParametersForStream(t *testing.T) { Down: &defaultDown, }, expected: true, + msg: "same values", }, { server: StreamUpstreamServer{}, serverNGX: StreamUpstreamServer{SlowStart: "10s"}, expected: false, + msg: "different SlowStart", }, { server: StreamUpstreamServer{SlowStart: "20s"}, serverNGX: StreamUpstreamServer{SlowStart: "10s"}, expected: false, + msg: "different SlowStart 2", }, } for _, test := range tests { - result := haveSameParametersForStream(test.server, test.serverNGX) - if result != test.expected { - t.Errorf("haveSameParametersForStream(%v, %v) returned %v but expected %v", test.server, test.serverNGX, result, test.expected) - } + t.Run(test.msg, func(t *testing.T) { + t.Parallel() + result := haveSameParametersForStream(test.server, test.serverNGX) + if result != test.expected { + t.Errorf("haveSameParametersForStream(%v, %v) returned %v but expected %v", test.server, test.serverNGX, result, test.expected) + } + }) } } @@ -665,9 +705,9 @@ func TestClientWithMaxAPI(t *testing.T) { } func TestGetStats_NoStreamEndpoint(t *testing.T) { + t.Parallel() var writeLock sync.Mutex - t.Parallel() ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { writeLock.Lock() defer writeLock.Unlock() diff --git a/tests/client_no_stream_test.go b/tests/client_no_stream_test.go index 525e369..92ee013 100644 --- a/tests/client_no_stream_test.go +++ b/tests/client_no_stream_test.go @@ -13,6 +13,7 @@ import ( // The API returns a special error code that we can use to determine if the API // is misconfigured or of the stream block is missing. func TestStatsNoStream(t *testing.T) { + t.Parallel() c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { t.Fatalf("Error connecting to nginx: %v", err) diff --git a/tests/client_test.go b/tests/client_test.go index bb97609..a93af8f 100644 --- a/tests/client_test.go +++ b/tests/client_test.go @@ -33,6 +33,7 @@ var ( defaultWeight = 1 ) +//nolint:paralleltest func TestStreamClient(t *testing.T) { c, err := client.NewNginxClient( helpers.GetAPIEndpoint(), @@ -254,6 +255,7 @@ func TestStreamClient(t *testing.T) { } func TestStreamUpstreamServer(t *testing.T) { + t.Parallel() c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { t.Fatalf("Error connecting to nginx: %v", err) @@ -302,6 +304,7 @@ func TestStreamUpstreamServer(t *testing.T) { } } +//nolint:paralleltest func TestClient(t *testing.T) { c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { @@ -524,6 +527,7 @@ func TestClient(t *testing.T) { } } +//nolint:paralleltest func TestUpstreamServer(t *testing.T) { c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { @@ -573,6 +577,7 @@ func TestUpstreamServer(t *testing.T) { } } +//nolint:paralleltest func TestStats(t *testing.T) { c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { @@ -715,6 +720,7 @@ func TestStats(t *testing.T) { } } +//nolint:paralleltest func TestUpstreamServerDefaultParameters(t *testing.T) { c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { @@ -765,6 +771,7 @@ func TestUpstreamServerDefaultParameters(t *testing.T) { } } +//nolint:paralleltest func TestStreamStats(t *testing.T) { c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { @@ -843,6 +850,7 @@ func TestStreamStats(t *testing.T) { } } +//nolint:paralleltest func TestStreamUpstreamServerDefaultParameters(t *testing.T) { c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { @@ -891,6 +899,7 @@ func TestStreamUpstreamServerDefaultParameters(t *testing.T) { } } +//nolint:paralleltest func TestKeyValue(t *testing.T) { zoneName := "zone_one" c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) @@ -989,6 +998,7 @@ func TestKeyValue(t *testing.T) { } } +//nolint:paralleltest func TestKeyValueStream(t *testing.T) { zoneName := "zone_one_stream" c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) @@ -1087,6 +1097,7 @@ func TestKeyValueStream(t *testing.T) { } func TestStreamZoneSync(t *testing.T) { + t.Parallel() c1, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { t.Fatalf("Error connecting to nginx: %v", err) @@ -1214,6 +1225,7 @@ func compareStreamUpstreamServers(x []client.StreamUpstreamServer, y []client.St } func TestUpstreamServerWithDrain(t *testing.T) { + t.Parallel() c, err := client.NewNginxClient(helpers.GetAPIEndpoint()) if err != nil { t.Fatalf("Error connecting to nginx: %v", err)