From d2337d0ec106e1aa8d5fadb3c15987b359f7190f Mon Sep 17 00:00:00 2001 From: Preston Van Loon Date: Wed, 3 Jun 2020 12:22:48 -0700 Subject: [PATCH] Disable some static analysis checks for coverage builds (#6105) * Disable some static analysis checks for coverage builds * Merge branch 'master' into fix-coverage-builds * disable lostcancel for third_party code * Merge branch 'fix-coverage-builds' of github.com:prysmaticlabs/prysm into fix-coverage-builds * Merge refs/heads/master into fix-coverage-builds * A few lostcancel fixes * Merge branch 'fix-coverage-builds' of github.com:prysmaticlabs/prysm into fix-coverage-builds * Merge refs/heads/master into fix-coverage-builds --- .bazelrc | 3 +++ BUILD.bazel | 17 +++++++++++++---- beacon-chain/p2p/service.go | 1 + beacon-chain/powchain/service.go | 1 + .../sync/initial-sync/blocks_fetcher_test.go | 6 ++++-- beacon-chain/sync/subscriber.go | 6 ++++-- .../sync/validate_attester_slashing_test.go | 3 ++- .../sync/validate_proposer_slashing_test.go | 3 ++- nogo_config.json | 7 ++++--- slasher/beaconclient/service.go | 1 + 10 files changed, 35 insertions(+), 13 deletions(-) diff --git a/.bazelrc b/.bazelrc index 02dd04c37..77f0b8d4b 100644 --- a/.bazelrc +++ b/.bazelrc @@ -5,6 +5,9 @@ test --test_verbose_timeout_warnings test --build_tests_only test --test_output=errors +# Clearly indicate that coverage is enabled to disable certain nogo checks. +coverage --define=coverage_enabled=1 + # Fix for rules_docker. See: https://github.com/bazelbuild/rules_docker/issues/842 build --host_force_python=PY2 test --host_force_python=PY2 diff --git a/BUILD.bazel b/BUILD.bazel index 73487b78c..1f9712ebf 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -88,15 +88,12 @@ nogo( "@org_golang_x_tools//go/analysis/passes/pkgfact:go_tool_library", "@org_golang_x_tools//go/analysis/passes/nilness:go_tool_library", "@org_golang_x_tools//go/analysis/passes/nilfunc:go_tool_library", - # lost cancel ignore doesn't seem to work when running with coverage - #"@org_golang_x_tools//go/analysis/passes/lostcancel:go_tool_library", "@org_golang_x_tools//go/analysis/passes/loopclosure:go_tool_library", "@org_golang_x_tools//go/analysis/passes/httpresponse:go_tool_library", "@org_golang_x_tools//go/analysis/passes/findcall:go_tool_library", "@org_golang_x_tools//go/analysis/passes/deepequalerrors:go_tool_library", "@org_golang_x_tools//go/analysis/passes/ctrlflow:go_tool_library", "@org_golang_x_tools//go/analysis/passes/copylock:go_tool_library", - "@org_golang_x_tools//go/analysis/passes/composite:go_tool_library", # "@org_golang_x_tools//go/analysis/passes/cgocall:go_tool_library", "@org_golang_x_tools//go/analysis/passes/buildtag:go_tool_library", "@org_golang_x_tools//go/analysis/passes/buildssa:go_tool_library", @@ -110,7 +107,19 @@ nogo( "//tools/analyzers/roughtime:go_tool_library", "//tools/analyzers/errcheck:go_tool_library", "//tools/analyzers/featureconfig:go_tool_library", - ], + ] + select({ + # nogo checks that fail with coverage enabled. + ":coverage_enabled": [], + "//conditions:default": [ + "@org_golang_x_tools//go/analysis/passes/lostcancel:go_tool_library", + "@org_golang_x_tools//go/analysis/passes/composite:go_tool_library", + ], + }), +) + +config_setting( + name = "coverage_enabled", + values = {"define": "coverage_enabled=1"}, ) common_files = { diff --git a/beacon-chain/p2p/service.go b/beacon-chain/p2p/service.go index 92561e5a2..4e2066b7b 100644 --- a/beacon-chain/p2p/service.go +++ b/beacon-chain/p2p/service.go @@ -94,6 +94,7 @@ type Service struct { func NewService(cfg *Config) (*Service, error) { var err error ctx, cancel := context.WithCancel(context.Background()) + _ = cancel // govet fix for lost cancel. Cancel is handled in service.Stop(). cache, err := ristretto.NewCache(&ristretto.Config{ NumCounters: 1000, MaxCost: 1000, diff --git a/beacon-chain/powchain/service.go b/beacon-chain/powchain/service.go index 2af2bdd60..7e966e3e5 100644 --- a/beacon-chain/powchain/service.go +++ b/beacon-chain/powchain/service.go @@ -172,6 +172,7 @@ func NewService(ctx context.Context, config *Web3ServiceConfig) (*Service, error ) } ctx, cancel := context.WithCancel(ctx) + _ = cancel // govet fix for lost cancel. Cancel is handled in service.Stop() depositTrie, err := trieutil.NewTrie(int(params.BeaconConfig().DepositContractTreeDepth)) if err != nil { cancel() diff --git a/beacon-chain/sync/initial-sync/blocks_fetcher_test.go b/beacon-chain/sync/initial-sync/blocks_fetcher_test.go index 4a2284ef9..60eb0e42d 100644 --- a/beacon-chain/sync/initial-sync/blocks_fetcher_test.go +++ b/beacon-chain/sync/initial-sync/blocks_fetcher_test.go @@ -436,7 +436,8 @@ func TestBlocksFetcherHandleRequest(t *testing.T) { p2p: p2p, }) - requestCtx, _ := context.WithTimeout(context.Background(), 2*time.Second) + requestCtx, reqCancel := context.WithTimeout(context.Background(), 2*time.Second) + defer reqCancel() go func() { response := fetcher.handleRequest(requestCtx, 1 /* start */, blockBatchLimit /* count */) select { @@ -529,7 +530,8 @@ func TestBlocksFetcherRequestBeaconBlocksByRangeRequest(t *testing.T) { // Test request fail over (error). err = fetcher.p2p.Disconnect(peers[1]) - ctx, _ = context.WithTimeout(context.Background(), time.Second*1) + ctx, cancel = context.WithTimeout(context.Background(), time.Second*1) + defer cancel() blocks, err = fetcher.requestBeaconBlocksByRange(ctx, peers[1], root, 1, 1, blockBatchLimit) testutil.AssertLogsContain(t, hook, "Request failed, trying to forward request to another peer") if err == nil || err.Error() != "context deadline exceeded" { diff --git a/beacon-chain/sync/subscriber.go b/beacon-chain/sync/subscriber.go index 1b6364a59..9723c7433 100644 --- a/beacon-chain/sync/subscriber.go +++ b/beacon-chain/sync/subscriber.go @@ -114,7 +114,8 @@ func (r *Service) subscribeWithBase(base proto.Message, topic string, validator // Pipeline decodes the incoming subscription data, runs the validation, and handles the // message. pipeline := func(msg *pubsub.Message) { - ctx, _ := context.WithTimeout(context.Background(), pubsubMessageTimeout) + ctx, cancel := context.WithTimeout(context.Background(), pubsubMessageTimeout) + defer cancel() ctx, span := trace.StartSpan(ctx, "sync.pubsub") defer span.End() @@ -169,7 +170,8 @@ func (r *Service) subscribeWithBase(base proto.Message, topic string, validator func wrapAndReportValidation(topic string, v pubsub.ValidatorEx) (string, pubsub.ValidatorEx) { return topic, func(ctx context.Context, pid peer.ID, msg *pubsub.Message) pubsub.ValidationResult { defer messagehandler.HandlePanic(ctx, msg) - ctx, _ = context.WithTimeout(ctx, pubsubMessageTimeout) + ctx, cancel := context.WithTimeout(ctx, pubsubMessageTimeout) + defer cancel() messageReceivedCounter.WithLabelValues(topic).Inc() b := v(ctx, pid, msg) if b == pubsub.ValidationReject { diff --git a/beacon-chain/sync/validate_attester_slashing_test.go b/beacon-chain/sync/validate_attester_slashing_test.go index 56e0df038..cec1dfedf 100644 --- a/beacon-chain/sync/validate_attester_slashing_test.go +++ b/beacon-chain/sync/validate_attester_slashing_test.go @@ -134,7 +134,8 @@ func TestValidateAttesterSlashing_ContextTimeout(t *testing.T) { slashing, state := setupValidAttesterSlashing(t) slashing.Attestation_1.Data.Target.Epoch = 100000000 - ctx, _ := context.WithTimeout(context.Background(), 100*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() c, err := lru.New(10) if err != nil { diff --git a/beacon-chain/sync/validate_proposer_slashing_test.go b/beacon-chain/sync/validate_proposer_slashing_test.go index 7ce895041..f530f65b7 100644 --- a/beacon-chain/sync/validate_proposer_slashing_test.go +++ b/beacon-chain/sync/validate_proposer_slashing_test.go @@ -170,7 +170,8 @@ func TestValidateProposerSlashing_ContextTimeout(t *testing.T) { slashing, state := setupValidProposerSlashing(t) slashing.Header_1.Header.Slot = 100000000 - ctx, _ := context.WithTimeout(context.Background(), 100*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() c, err := lru.New(10) if err != nil { diff --git a/nogo_config.json b/nogo_config.json index 2054d18aa..6fb1150eb 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -13,7 +13,8 @@ }, "lostcancel": { "exclude_files": { - "validator/client/runner.go": "No need to cancel right when goroutines begin" + "validator/client/runner.go": "No need to cancel right when goroutines begin", + "external/.*": "Third party code" } }, "nilness": { @@ -41,12 +42,12 @@ "composites": { "exclude_files": { "external/.*": "Third party code" - } + } }, "cgocall": { "exclude_files": { "external/.*": "Third party code" - } + } }, "assign": { "exclude_files": { diff --git a/slasher/beaconclient/service.go b/slasher/beaconclient/service.go index bfa20930b..11960efcf 100644 --- a/slasher/beaconclient/service.go +++ b/slasher/beaconclient/service.go @@ -75,6 +75,7 @@ type Config struct { // NewBeaconClientService instantiation. func NewBeaconClientService(ctx context.Context, cfg *Config) (*Service, error) { ctx, cancel := context.WithCancel(ctx) + _ = cancel // govet fix for lost cancel. Cancel is handled in service.Stop() publicKeyCache, err := cache.NewPublicKeyCache(0, nil) if err != nil { return nil, errors.Wrap(err, "could not create new cache")