Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
atollena opened this issue Jun 6, 2024 · 4 comments
Open

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

atollena opened this issue Jun 6, 2024 · 4 comments
Assignees
@hasson82
Labels
Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. P2 Type: Internal Cleanup Refactors, etc

Comments

@atollena
Copy link
Collaborator

atollena commented Jun 6, 2024

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 #7271 (comment)
@atollena atollena added Type: Feature New features or improvements in behavior P2 labels Jun 6, 2024
@atollena
Copy link
Collaborator Author

atollena commented Jun 6, 2024

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(

@atollena atollena added Type: Internal Cleanup Refactors, etc and removed Type: Feature New features or improvements in behavior labels Jun 6, 2024
@atollena atollena mentioned this issue Jun 6, 2024
Merged
@easwars
Copy link
Contributor

easwars commented Jun 6, 2024

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

@hasson82
Copy link
Contributor

hasson82 commented Jun 19, 2024 •
edited
Loading

@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
Copy link
Collaborator Author

No-one is working on this at the moment.

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

@hasson82 hasson82 mentioned this issue Jun 22, 2024
Merged
@purnesh42H purnesh42H added Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. and removed Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. labels Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hasson82 hasson82

Labels
Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. P2 Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@atollena @easwars @purnesh42H @hasson82