search

grpc / grpc-go

The Go language implementation of gRPC. HTTP/2 based RPC
https://grpc.io
Apache License 2.0
20.96k stars 4.35k forks source link

Linter rule for using context.Background() without a timeout in tests #7304

Open atollena opened 4 months ago

atollena commented 4 months ago

Use case(s) - what problem will this feature solve?

We want timeouts in every test, so anytime an operation needs a context (even during test setup), we want to avoid code using context.Background() or context.TODO() is needed there should be something like context.WithTimeout(context.Background(), defaultTestTimeout).

Proposed Solution

Add a linter rule that makes sure whenever we use context.Background() in test, it is using WithTimeout. Something like

git grep 'context.Background()' -- "*_test.go" | not grep -v 'context.WithTimeout(context.Background()'

Alternatives Considered

Do not lint this and continue relying on reviews to catch this.

Additional Context

Prompted by https://github.com/grpc/grpc-go/pull/7271#discussion_r1626538233

atollena commented 4 months ago

There are many cases where we use context.Background because we don't use the context for cancellation:

+ git grep 'context.Background()' -- '*_test.go'
+ not grep -v 'context.WithTimeout(context.Background()'
+ grep -v 'context.WithTimeout(context.Background()'
benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   pCtx := context.Background()
clientconn_test.go: ctx, cancel := context.WithCancel(context.Background())
credentials/alts/internal/handshaker/handshaker_test.go:    hs, err := NewClientHandshaker(context.Background(), clientConn, conn, opts)
credentials/alts/internal/handshaker/handshaker_test.go:    hs, err := NewServerHandshaker(context.Background(), clientConn, conn, opts)
credentials/google/google_test.go:              ctx:     context.Background(),
credentials/google/google_test.go:              ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
credentials/google/google_test.go:              ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
credentials/google/google_test.go:              ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
credentials/google/google_test.go:              ctx: icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
credentials/google/xds_test.go:     return icredentials.NewClientHandshakeInfoContext(context.Background(), credentials.ClientHandshakeInfo{
credentials/google/xds_test.go:     {"not an xDS cluster", context.Background(), false},
gcp/observability/observability_test.go:    if err := Start(context.Background()); err == nil {
gcp/observability/observability_test.go:    if err := Start(context.Background()); err == nil {
gcp/observability/observability_test.go:    if err := Start(context.Background()); err == nil {
gcp/observability/observability_test.go:    if err := Start(context.Background()); err == nil {
gcp/observability/observability_test.go:    if err := Start(context.Background()); err == nil {
gcp/observability/observability_test.go:    if err := Start(context.Background()); err == nil {
internal/binarylog/method_logger_test.go:       ml.Log(context.Background(), tc.config)
internal/transport/handler_server_test.go:      context.Background(), func(s *Stream) { go handleStream(s) },
internal/transport/handler_server_test.go:      context.Background(), func(s *Stream) { go handleStream(s) },
internal/transport/handler_server_test.go:      context.Background(), func(s *Stream) { go runStream(s) },
internal/transport/handler_server_test.go:      context.Background(), func(s *Stream) { go handleStream(st, s) },
internal/transport/handler_server_test.go:      context.Background(), func(s *Stream) { go handleStream(s) },
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), h.handleStreamAndNotify)
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), func(*Stream) {})
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), func(s *Stream) {
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), func(s *Stream) {
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), func(s *Stream) {
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), func(s *Stream) {
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), func(s *Stream) {
internal/transport/transport_test.go:           go transport.HandleStreams(context.Background(), func(s *Stream) {
internal/transport/transport_test.go:   connectCtx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
internal/transport/transport_test.go:   ct, connErr := NewClientTransport(connectCtx, context.Background(), addr, copts, func(GoAwayReason) {})
internal/transport/transport_test.go:   connectCtx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
internal/transport/transport_test.go:   tr, err := NewClientTransport(connectCtx, context.Background(), resolver.Address{Addr: lis.Addr().String()}, copts, func(GoAwayReason) {})
internal/transport/transport_test.go:   ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Second*10))
internal/transport/transport_test.go:   ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Second*10))
internal/transport/transport_test.go:   pctx, cancel := context.WithCancel(context.Background())
internal/transport/transport_test.go:       ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(time.Second*10))
internal/transport/transport_test.go:   _, err = NewClientTransport(connectCtx, context.Background(), resolver.Address{Addr: lis.Addr().String()}, copts, func(GoAwayReason) {})
internal/transport/transport_test.go:   _, err = NewClientTransport(connectCtx, context.Background(), resolver.Address{Addr: lis.Addr().String()}, copts, func(GoAwayReason) {})
internal/transport/transport_test.go:   connectCtx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
internal/transport/transport_test.go:   ct, err := NewClientTransport(connectCtx, context.Background(), resolver.Address{Addr: lis.Addr().String()}, copts, func(GoAwayReason) {})
internal/transport/transport_test.go:       ctx:         context.Background(),
internal/transport/transport_test.go:   ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
internal/transport/transport_test.go:   tr, err := NewClientTransport(ctx, context.Background(), addr, copts, func(GoAwayReason) {})
internal/transport/transport_test.go:   ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(2*time.Second))
internal/transport/transport_test.go:   tr, err := NewClientTransport(ctx, context.Background(), addr, copts, func(GoAwayReason) {})
internal/transport/transport_test.go:   ct, err := NewClientTransport(ctx, context.Background(), resolver.Address{Addr: lis.Addr().String()}, ConnectOptions{}, func(GoAwayReason) {})
internal/xds/bootstrap/tlscreds/bundle_ext_test.go: if _, err = client.EmptyCall(context.Background(), &testpb.Empty{}); err != nil {
internal/xds/bootstrap/tlscreds/bundle_test.go: _, err = client.EmptyCall(context.Background(), &testpb.Empty{})
internal/xds/rbac/rbac_engine_test.go:                  ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md)
metadata/metadata_test.go:  ctx := NewIncomingContext(context.Background(), md)
metadata/metadata_test.go:  ctx := NewIncomingContext(context.Background(), md)
metadata/metadata_test.go:  ctx := NewIncomingContext(context.Background(), md)
metadata/metadata_test.go:  ctx := NewIncomingContext(context.Background(), md)
peer/peer_test.go:          ctx := NewContext(context.Background(), tc.peer)
picker_wrapper_test.go:         if tr, _, err := bp.pick(context.Background(), true, balancer.PickInfo{}); err != nil || tr != testT {
picker_wrapper_test.go:         if tr, _, err := bp.pick(context.Background(), true, balancer.PickInfo{}); err != nil || tr != testT {
picker_wrapper_test.go:         if tr, _, err := bp.pick(context.Background(), false, balancer.PickInfo{}); err != nil || tr != testT {
picker_wrapper_test.go:         if tr, _, err := bp.pick(context.Background(), true, balancer.PickInfo{}); err != nil || tr != testT {
resolver_balancer_ext_test.go:      ctx, cancel = context.WithCancel(context.Background())
security/advancedtls/advancedtls_test.go:           _, clientAuthInfo, handshakeErr := clientTLS.ClientHandshake(context.Background(),
server_test.go: ii(context.Background(), nil, nil, handler)
server_test.go: ctx := NewContextWithServerTransportStream(context.Background(), expectedStream)
server_test.go:             if _, err := s.opts.unaryInt(context.Background(), nil, nil,
test/gracefulstop_test.go:      _, err := clt.UnaryCall(context.Background(), &testpb.SimpleRequest{})
test/gracefulstop_test.go:      _, err := clt.UnaryCall(context.Background(), &testpb.SimpleRequest{})
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_1"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_2"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "notacluster"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_1"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_2"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:notacluster"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_1"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_2"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_3"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:notacluster"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_1"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_2"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:notacluster"),
xds/internal/balancer/clustermanager/clustermanager_test.go:        gotSCSt, err := p2.Pick(balancer.PickInfo{Ctx: SetPickedCluster(context.Background(), "cds:notacluster")})
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_1"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:cluster_2"),
xds/internal/balancer/clustermanager/clustermanager_test.go:                Ctx: SetPickedCluster(context.Background(), "cds:notacluster"),
xds/internal/balancer/clustermanager/clustermanager_test.go:        Ctx: SetPickedCluster(context.Background(), "csp:cluster"),
xds/internal/balancer/ringhash/picker_test.go:              Ctx: SetRequestHash(context.Background(), tt.hash),
xds/internal/balancer/ringhash/picker_test.go:  _, err := p.Pick(balancer.PickInfo{Ctx: SetRequestHash(context.Background(), 5)})
xds/internal/balancer/ringhash/picker_test.go:  pr, err := p.Pick(balancer.PickInfo{Ctx: SetRequestHash(context.Background(), 5)})
xds/internal/balancer/ringhash/picker_test.go:  _, err := p.Pick(balancer.PickInfo{Ctx: SetRequestHash(context.Background(), 5)})
xds/internal/balancer/ringhash/ringhash_test.go:    return SetRequestHash(context.Background(), h)
xds/internal/balancer/ringhash/ringhash_test.go:    p0.Pick(balancer.PickInfo{Ctx: context.Background()})
xds/internal/resolver/serviceconfig_test.go:                Context: metadata.NewOutgoingContext(context.Background(), metadata.Pairs(":path", "/products")),
xds/internal/resolver/serviceconfig_test.go:                Context: metadata.NewOutgoingContext(context.Background(), metadata.Pairs(":path", "abc")),
xds/internal/resolver/serviceconfig_test.go:                Context: metadata.NewOutgoingContext(context.Background(), metadata.Pairs("something-bin", "xyz")),
xds/internal/resolver/serviceconfig_test.go:                    metadata.NewOutgoingContext(context.Background(), metadata.Pairs("content-type", "user value")),
xds/internal/resolver/xds_resolver_test.go:                 _, err = res.Interceptor.NewStream(context.Background(), iresolver.RPCInfo{}, func() {}, func(ctx context.Context, done func()) (iresolver.ClientStream, error) {
xds/internal/xdsclient/xdsresource/filter_chain_test.go:                        errs = append(errs, int.AllowRPC(context.Background()).Error())
xds/internal/xdsclient/xdsresource/matcher_test.go:             Context: metadata.NewOutgoingContext(context.Background(), metadata.Pairs("th", "tv")),
xds/internal/xdsclient/xdsresource/matcher_test.go:             Context: metadata.NewOutgoingContext(context.Background(), metadata.Pairs("th", "tv")),
xds/internal/xdsclient/xdsresource/matcher_test.go:             Context: metadata.NewOutgoingContext(context.Background(), metadata.Pairs("th", "tv")),
xds/internal/xdsclient/xdsresource/matcher_test.go:             Context: metadata.NewOutgoingContext(context.Background(), metadata.Pairs("th", "tv")),
xds/internal/xdsclient/xdsresource/matcher_test.go:             Context: grpcutil.WithExtraMetadata(context.Background(), metadata.Pairs(
xds/internal/xdsclient/xdsresource/matcher_test.go:                 metadata.NewOutgoingContext(context.Background(), metadata.Pairs("t-bin", "123")), metadata.Pairs(
easwars commented 4 months ago

Thanks for filing this issue. That's an embarrassing number of usages of the background context.

hasson82 commented 3 months ago

@atollena I am willing to work on it if it is still vacant.

I will add here a question I have, to avoid ping pong later. What do we think about the benchmark tests for contexts. 

benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   ctx, cancel := context.WithCancel(context.Background())
benchmark/primitives/context_test.go:   pCtx := context.Background()

Do we want to add timeouts there? Can it affect the test?

atollena commented 3 months ago

No-one is working on this at the moment.

I think you should exclude benchmark/primitives/context_test from the linter rule.