From c22514c5d1c70490b7e53384e72ebc33ea963989 Mon Sep 17 00:00:00 2001 From: Doug Fawley Date: Wed, 8 Nov 2023 09:04:44 -0800 Subject: [PATCH] vet: various cleanups - Use staticcheck for pkg comments instead of special code. - Remove golint use (it is deprecated & very slow). - Re-add deprecation warnings and update exclusions. - Add all checkers to staticcheck (e.g. mismatched receivers, underscores). - Add a manual check for misspelled test functions that prevent running. --- .../rls/internal/adaptive/adaptive_test.go | 10 +- benchmark/benchmain/main.go | 13 +- vet.sh | 159 ++++++++---------- .../outlierdetection/callcounter_test.go | 10 +- xds/internal/resolver/xds_resolver_test.go | 1 - xds/internal/xdsclient/authority_test.go | 3 +- .../xdsclient/tests/cds_watchers_test.go | 2 +- 7 files changed, 88 insertions(+), 110 deletions(-) diff --git a/balancer/rls/internal/adaptive/adaptive_test.go b/balancer/rls/internal/adaptive/adaptive_test.go index 2205b533eec7..8b74a7cfa567 100644 --- a/balancer/rls/internal/adaptive/adaptive_test.go +++ b/balancer/rls/internal/adaptive/adaptive_test.go @@ -25,13 +25,13 @@ import ( ) // stats returns a tuple with accepts, throttles for the current time. -func (th *Throttler) stats() (int64, int64) { +func (t *Throttler) stats() (int64, int64) { now := timeNowFunc() - th.mu.Lock() - a, t := th.accepts.sum(now), th.throttles.sum(now) - th.mu.Unlock() - return a, t + t.mu.Lock() + a, th := t.accepts.sum(now), t.throttles.sum(now) + t.mu.Unlock() + return a, th } // Enums for responses. diff --git a/benchmark/benchmain/main.go b/benchmark/benchmain/main.go index 9486c65daf31..cfa4e5c2d400 100644 --- a/benchmark/benchmain/main.go +++ b/benchmark/benchmain/main.go @@ -61,7 +61,6 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/benchmark" - bm "google.golang.org/grpc/benchmark" "google.golang.org/grpc/benchmark/flags" "google.golang.org/grpc/benchmark/latency" "google.golang.org/grpc/benchmark/stats" @@ -378,11 +377,11 @@ func makeClients(bf stats.Features) ([]testgrpc.BenchmarkServiceClient, func()) })) } lis = nw.Listener(lis) - stopper := bm.StartServer(bm.ServerInfo{Type: "protobuf", Listener: lis}, sopts...) + stopper := benchmark.StartServer(benchmark.ServerInfo{Type: "protobuf", Listener: lis}, sopts...) conns := make([]*grpc.ClientConn, bf.Connections) clients := make([]testgrpc.BenchmarkServiceClient, bf.Connections) for cn := 0; cn < bf.Connections; cn++ { - conns[cn] = bm.NewClientConn("" /* target not used */, opts...) + conns[cn] = benchmark.NewClientConn("" /* target not used */, opts...) clients[cn] = testgrpc.NewBenchmarkServiceClient(conns[cn]) } @@ -430,7 +429,7 @@ func makeFuncStream(bf stats.Features) (rpcCallFunc, rpcCleanupFunc) { if bf.EnablePreloader { req = preparedMsg[cn][pos] } else { - pl := bm.NewPayload(testpb.PayloadType_COMPRESSABLE, reqSizeBytes) + pl := benchmark.NewPayload(testpb.PayloadType_COMPRESSABLE, reqSizeBytes) req = &testpb.SimpleRequest{ ResponseType: pl.Type, ResponseSize: int32(respSizeBytes), @@ -488,7 +487,7 @@ func setupStream(bf stats.Features, unconstrained bool) ([][]testgrpc.BenchmarkS } } - pl := bm.NewPayload(testpb.PayloadType_COMPRESSABLE, bf.ReqSizeBytes) + pl := benchmark.NewPayload(testpb.PayloadType_COMPRESSABLE, bf.ReqSizeBytes) req := &testpb.SimpleRequest{ ResponseType: pl.Type, ResponseSize: int32(bf.RespSizeBytes), @@ -515,13 +514,13 @@ func prepareMessages(streams [][]testgrpc.BenchmarkService_StreamingCallClient, // Makes a UnaryCall gRPC request using the given BenchmarkServiceClient and // request and response sizes. func unaryCaller(client testgrpc.BenchmarkServiceClient, reqSize, respSize int) { - if err := bm.DoUnaryCall(client, reqSize, respSize); err != nil { + if err := benchmark.DoUnaryCall(client, reqSize, respSize); err != nil { logger.Fatalf("DoUnaryCall failed: %v", err) } } func streamCaller(stream testgrpc.BenchmarkService_StreamingCallClient, req any) { - if err := bm.DoStreamingRoundTripPreloaded(stream, req); err != nil { + if err := benchmark.DoStreamingRoundTripPreloaded(stream, req); err != nil { logger.Fatalf("DoStreamingRoundTrip failed: %v", err) } } diff --git a/vet.sh b/vet.sh index ee29f4381275..038990b4f7bc 100755 --- a/vet.sh +++ b/vet.sh @@ -77,6 +77,9 @@ fi not grep 'func Test[^(]' *_test.go not grep 'func Test[^(]' test/*.go +# - Check for typos in test function names +git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' + # - Do not import x/net/context. not git grep -l 'x/net/context' -- "*.go" @@ -94,7 +97,7 @@ git grep -l -e 'grpclog.I' --or -e 'grpclog.W' --or -e 'grpclog.E' --or -e 'grpc not git grep "\(import \|^\s*\)\"github.com/golang/protobuf/ptypes/" -- "*.go" # - Ensure all usages of grpc_testing package are renamed when importing. -not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- "*.go" +not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- "*.go" # - Ensure all xds proto imports are renamed to *pb or *grpc. git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "' @@ -110,7 +113,6 @@ for MOD_FILE in $(find . -name 'go.mod'); do go vet -all ./... | fail_on_output gofmt -s -d -l . 2>&1 | fail_on_output goimports -l . 2>&1 | not grep -vE "\.pb\.go" - golint ./... 2>&1 | not grep -vE "/grpc_testing_not_regenerate/.*\.pb\.go:" go mod tidy -compat=1.19 git status --porcelain 2>&1 | fail_on_output || \ @@ -119,94 +121,73 @@ for MOD_FILE in $(find . -name 'go.mod'); do done # - Collection of static analysis checks -# -# TODO(dfawley): don't use deprecated functions in examples or first-party -# plugins. -# TODO(dfawley): enable ST1019 (duplicate imports) but allow for protobufs. SC_OUT="$(mktemp)" -staticcheck -go 1.19 -checks 'inherit,-ST1015,-ST1019,-SA1019' ./... > "${SC_OUT}" || true -# Error if anything other than deprecation warnings are printed. -not grep -v "is deprecated:.*SA1019" "${SC_OUT}" -# Only ignore the following deprecated types/fields/functions. -not grep -Fv '.CredsBundle -.HeaderMap -.Metadata is deprecated: use Attributes -.NewAddress -.NewServiceConfig -.Type is deprecated: use Attributes -BuildVersion is deprecated -balancer.ErrTransientFailure -balancer.Picker -extDesc.Filename is deprecated -github.com/golang/protobuf/jsonpb is deprecated -grpc.CallCustomCodec -grpc.Code -grpc.Compressor -grpc.CustomCodec -grpc.Decompressor -grpc.MaxMsgSize -grpc.MethodConfig -grpc.NewGZIPCompressor -grpc.NewGZIPDecompressor -grpc.RPCCompressor -grpc.RPCDecompressor -grpc.ServiceConfig -grpc.WithCompressor -grpc.WithDecompressor -grpc.WithDialer -grpc.WithMaxMsgSize -grpc.WithServiceConfig -grpc.WithTimeout -http.CloseNotifier -info.SecurityVersion -proto is deprecated -proto.InternalMessageInfo is deprecated -proto.EnumName is deprecated -proto.ErrInternalBadWireType is deprecated -proto.FileDescriptor is deprecated -proto.Marshaler is deprecated -proto.MessageType is deprecated -proto.RegisterEnum is deprecated -proto.RegisterFile is deprecated -proto.RegisterType is deprecated -proto.RegisterExtension is deprecated -proto.RegisteredExtension is deprecated -proto.RegisteredExtensions is deprecated -proto.RegisterMapType is deprecated -proto.Unmarshaler is deprecated +staticcheck -go 1.19 -checks 'all' ./... > "${SC_OUT}" || true + +# Error for anything other than checks that need exclusions. +grep -v "(ST1000)" "${SC_OUT}" | grep -v "(SA1019)" | grep -v "(ST1003)" | not grep -v "(ST1019)\|\(other import of\)" + +# Exclude underscore checks for generated code. +grep "(ST1003)" "${SC_OUT}" | not grep -Fv '.pb.go:' 'code_string_test.go:' + +# Error for duplicate imports not including grpc protos. +grep "(ST1019)\|\(other import of\)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused +channelz/grpc_channelz_v1" +go-control-plane/envoy +grpclb/grpc_lb_v1" +health/grpc_health_v1" +interop/grpc_testing" +orca/v3" +proto/grpc_gcp" +proto/grpc_lookup_v1" +reflection/grpc_reflection_v1" +reflection/grpc_reflection_v1alpha" +XXXXX PleaseIgnoreUnused' + +# Error for any package comments not in generated code. +grep "(ST1000)" "${SC_OUT}" | not grep -v "\.pb\.go:" + +# Only ignore the following deprecated types/fields/functions and exclude +# generated code. +grep "(SA1019)" "${SC_OUT}" | not grep -Fv 'XXXXX PleaseIgnoreUnused +XXXXX Protobuf related deprecation errors: +"github.com/golang/protobuf +.pb.go: +: ptypes. +proto.RegisterType +XXXXX gRPC internal usage deprecation errors: +"google.golang.org/grpc +: grpc. +: v1alpha. +: v1alphareflectionpb. +BalancerAttributes is deprecated: +CredsBundle is deprecated: +Metadata is deprecated: use Attributes instead. +NewSubConn is deprecated: +OverrideServerName is deprecated: +RemoveSubConn is deprecated: +SecurityVersion is deprecated: Target is deprecated: Use the Target field in the BuildOptions instead. -xxx_messageInfo_ -' "${SC_OUT}" - -# - special golint on package comments. -lint_package_comment_per_package() { - # Number of files in this go package. - fileCount=$(go list -f '{{len .GoFiles}}' $1) - if [ ${fileCount} -eq 0 ]; then - return 0 - fi - # Number of package errors generated by golint. - lintPackageCommentErrorsCount=$(golint --min_confidence 0 $1 | grep -c "should have a package comment") - # golint complains about every file that's missing the package comment. If the - # number of files for this package is greater than the number of errors, there's - # at least one file with package comment, good. Otherwise, fail. - if [ ${fileCount} -le ${lintPackageCommentErrorsCount} ]; then - echo "Package $1 (with ${fileCount} files) is missing package comment" - return 1 - fi -} -lint_package_comment() { - set +ex - - count=0 - for i in $(go list ./...); do - lint_package_comment_per_package "$i" - ((count += $?)) - done - - set -ex - return $count -} -lint_package_comment +UpdateAddresses is deprecated: +UpdateSubConnState is deprecated: +balancer.ErrTransientFailure is deprecated: +grpc/reflection/v1alpha/reflection.proto +XXXXX xDS deprecated fields we support +.ExactMatch +.PrefixMatch +.SafeRegexMatch +.SuffixMatch +GetContainsMatch +GetExactMatch +GetMatchSubjectAltNames +GetPrefixMatch +GetSafeRegexMatch +GetSuffixMatch +GetTlsCertificateCertificateProviderInstance +GetValidationContextCertificateProviderInstance +XXXXX TODO: Remove the below deprecation usages: +CloseNotifier +Roots.Subjects +XXXXX PleaseIgnoreUnused' echo SUCCESS diff --git a/xds/internal/balancer/outlierdetection/callcounter_test.go b/xds/internal/balancer/outlierdetection/callcounter_test.go index 8e4f5f29b5f8..9807838e65e5 100644 --- a/xds/internal/balancer/outlierdetection/callcounter_test.go +++ b/xds/internal/balancer/outlierdetection/callcounter_test.go @@ -38,19 +38,19 @@ func (b1 *bucket) Equal(b2 *bucket) bool { return b1.numFailures == b2.numFailures } -func (cc1 *callCounter) Equal(cc2 *callCounter) bool { - if cc1 == nil && cc2 == nil { +func (cc *callCounter) Equal(cc2 *callCounter) bool { + if cc == nil && cc2 == nil { return true } - if (cc1 != nil) != (cc2 != nil) { + if (cc != nil) != (cc2 != nil) { return false } - ab1 := (*bucket)(atomic.LoadPointer(&cc1.activeBucket)) + ab1 := (*bucket)(atomic.LoadPointer(&cc.activeBucket)) ab2 := (*bucket)(atomic.LoadPointer(&cc2.activeBucket)) if !ab1.Equal(ab2) { return false } - return cc1.inactiveBucket.Equal(cc2.inactiveBucket) + return cc.inactiveBucket.Equal(cc2.inactiveBucket) } // TestClear tests that clear on the call counter clears (everything set to 0) diff --git a/xds/internal/resolver/xds_resolver_test.go b/xds/internal/resolver/xds_resolver_test.go index 71e395730180..0c898e45ff9e 100644 --- a/xds/internal/resolver/xds_resolver_test.go +++ b/xds/internal/resolver/xds_resolver_test.go @@ -67,7 +67,6 @@ import ( v3discoverypb "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3" _ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer" // Register the cds LB policy - _ "google.golang.org/grpc/xds/internal/balancer/cdsbalancer" // To parse LB config _ "google.golang.org/grpc/xds/internal/httpfilter/router" // Register the router filter ) diff --git a/xds/internal/xdsclient/authority_test.go b/xds/internal/xdsclient/authority_test.go index 09a81759a1f3..8168cd203f05 100644 --- a/xds/internal/xdsclient/authority_test.go +++ b/xds/internal/xdsclient/authority_test.go @@ -31,7 +31,6 @@ import ( "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/testutils" - xdstestutils "google.golang.org/grpc/xds/internal/testutils" "google.golang.org/grpc/xds/internal/xdsclient/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource/version" @@ -66,7 +65,7 @@ func setupTest(ctx context.Context, t *testing.T, opts e2e.ManagementServerOptio } a, err := newAuthority(authorityArgs{ - serverCfg: xdstestutils.ServerConfigForAddress(t, ms.Address), + serverCfg: testutils.ServerConfigForAddress(t, ms.Address), bootstrapCfg: &bootstrap.Config{ NodeProto: &v3corepb.Node{Id: nodeID}, }, diff --git a/xds/internal/xdsclient/tests/cds_watchers_test.go b/xds/internal/xdsclient/tests/cds_watchers_test.go index 615c68da7e60..2bd0226788ab 100644 --- a/xds/internal/xdsclient/tests/cds_watchers_test.go +++ b/xds/internal/xdsclient/tests/cds_watchers_test.go @@ -633,7 +633,7 @@ func (s) TestCDSWatch_ValidResponseCancelsExpiryTimerBehavior(t *testing.T) { // 2. An update to other resource should result in the invocation of the watch // callback associated with that resource. It should not result in the // invocation of the watch callback associated with the deleted resource. -func (s) TesCDSWatch_ResourceRemoved(t *testing.T) { +func (s) TestCDSWatch_ResourceRemoved(t *testing.T) { mgmtServer, nodeID, bootstrapContents, _, cleanup := e2e.SetupManagementServer(t, e2e.ManagementServerOptions{}) defer cleanup()