From f2e9b9df4ae0e31fcf8c34f1258937ef730e0ce8 Mon Sep 17 00:00:00 2001 From: Quang-Minh Nguyen Date: Mon, 14 Aug 2023 16:08:39 +0700 Subject: Correct context.Context and testing.T order in test This commit fixes some incorrect parameter ordering in tests. We always set testing.T before context.Context. --- internal/gitaly/server/auth_test.go | 18 +++---- internal/limiter/concurrency_limiter_test.go | 70 ++++++++++++++-------------- internal/testhelper/testserver/gitaly.go | 4 +- 3 files changed, 46 insertions(+), 46 deletions(-) diff --git a/internal/gitaly/server/auth_test.go b/internal/gitaly/server/auth_test.go index 99d9f498a..c845bd68f 100644 --- a/internal/gitaly/server/auth_test.go +++ b/internal/gitaly/server/auth_test.go @@ -44,7 +44,7 @@ func TestMain(m *testing.M) { } func TestSanity(t *testing.T) { - serverSocketPath := runServer(testhelper.Context(t), t, testcfg.Build(t)) + serverSocketPath := runServer(t, testhelper.Context(t), testcfg.Build(t)) conn, err := dial(serverSocketPath, []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}) require.NoError(t, err) @@ -56,7 +56,7 @@ func TestSanity(t *testing.T) { func TestTLSSanity(t *testing.T) { cfg := testcfg.Build(t) ctx := testhelper.Context(t) - addr := runSecureServer(ctx, t, cfg) + addr := runSecureServer(t, ctx, cfg) certPool, err := x509.SystemCertPool() require.NoError(t, err) @@ -103,7 +103,7 @@ func TestAuthFailures(t *testing.T) { Auth: auth.Config{Token: "quxbaz"}, })) - serverSocketPath := runServer(testhelper.Context(t), t, cfg) + serverSocketPath := runServer(t, testhelper.Context(t), cfg) connOpts := append(tc.opts, grpc.WithTransportCredentials(insecure.NewCredentials())) conn, err := dial(serverSocketPath, connOpts) require.NoError(t, err, tc.desc) @@ -146,7 +146,7 @@ func TestAuthSuccess(t *testing.T) { Auth: auth.Config{Token: tc.token, Transitioning: !tc.required}, })) - serverSocketPath := runServer(testhelper.Context(t), t, cfg) + serverSocketPath := runServer(t, testhelper.Context(t), cfg) connOpts := append(tc.opts, grpc.WithTransportCredentials(insecure.NewCredentials())) conn, err := dial(serverSocketPath, connOpts) require.NoError(t, err, tc.desc) @@ -187,7 +187,7 @@ func newOperationClient(t *testing.T, token, serverSocketPath string) (gitalypb. return gitalypb.NewOperationServiceClient(conn), conn } -func runServer(ctx context.Context, t *testing.T, cfg config.Cfg) string { +func runServer(t *testing.T, ctx context.Context, cfg config.Cfg) string { t.Helper() registry := backchannel.NewRegistry() @@ -229,7 +229,7 @@ func runServer(ctx context.Context, t *testing.T, cfg config.Cfg) string { } //go:generate openssl req -newkey rsa:4096 -new -nodes -x509 -days 3650 -out testdata/gitalycert.pem -keyout testdata/gitalykey.pem -subj "/C=US/ST=California/L=San Francisco/O=GitLab/OU=GitLab-Shell/CN=localhost" -addext "subjectAltName = IP:127.0.0.1, DNS:localhost" -func runSecureServer(ctx context.Context, t *testing.T, cfg config.Cfg) string { +func runSecureServer(t *testing.T, ctx context.Context, cfg config.Cfg) string { t.Helper() cfg.TLS = config.TLS{ @@ -262,7 +262,7 @@ func TestUnaryNoAuth(t *testing.T) { cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{Auth: auth.Config{Token: "testtoken"}})) ctx := testhelper.Context(t) - path := runServer(ctx, t, cfg) + path := runServer(t, ctx, cfg) conn, err := grpc.Dial(path, grpc.WithTransportCredentials(insecure.NewCredentials())) require.NoError(t, err) defer testhelper.MustClose(t, conn) @@ -283,7 +283,7 @@ func TestStreamingNoAuth(t *testing.T) { cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{Auth: auth.Config{Token: "testtoken"}})) ctx := testhelper.Context(t) - path := runServer(ctx, t, cfg) + path := runServer(t, ctx, cfg) conn, err := dial(path, []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}) require.NoError(t, err) t.Cleanup(func() { conn.Close() }) @@ -331,7 +331,7 @@ func TestAuthBeforeLimit(t *testing.T) { t.Cleanup(cleanup) cfg.Gitlab.URL = gitlabURL - serverSocketPath := runServer(ctx, t, cfg) + serverSocketPath := runServer(t, ctx, cfg) client, conn := newOperationClient(t, cfg.Auth.Token, serverSocketPath) t.Cleanup(func() { conn.Close() }) diff --git a/internal/limiter/concurrency_limiter_test.go b/internal/limiter/concurrency_limiter_test.go index 67ca38e1e..6ba48d243 100644 --- a/internal/limiter/concurrency_limiter_test.go +++ b/internal/limiter/concurrency_limiter_test.go @@ -241,14 +241,14 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 10, nil, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) // Update the limit to 7 limit.Update(7) // 2 more requests acquired the token. This proves the limit is expanded - release2, waitAfterRelease2 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 2) + release2, waitAfterRelease2 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 2) require.Equal(t, 7, gauge.enter) close(release1) @@ -265,7 +265,7 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 10, nil, gauge) // 3 requests acquired the tokens, 2 slots left - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 3) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 3) require.Equal(t, 3, gauge.enter) require.Equal(t, 3, gauge.queued) @@ -273,7 +273,7 @@ func TestLimiter_dynamic(t *testing.T) { limit.Update(3) // 2 requests are put in queue, meaning the limit shrinks down - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 2) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 2) require.Equal(t, 3, gauge.enter) require.Equal(t, 5, gauge.queued) @@ -300,11 +300,11 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 10, nil, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) // 2 requests waiting in the queue - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 2) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 2) require.Equal(t, 5, gauge.enter) require.Equal(t, 7, gauge.queued) @@ -329,16 +329,16 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 10, nil, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) // 2 requests waiting in the queue - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 2) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 2) require.Equal(t, 5, gauge.enter) require.Equal(t, 7, gauge.queued) // 5 more requests waiting in the queue - waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 5) + waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) require.Equal(t, 12, gauge.queued) @@ -369,14 +369,14 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 10, nil, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) // Update the limit to 3 limit.Update(3) // 3 requests are put in queue - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 3) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 3) require.Equal(t, 5, gauge.enter) require.Equal(t, 8, gauge.queued) @@ -390,7 +390,7 @@ func TestLimiter_dynamic(t *testing.T) { require.Equal(t, 8, gauge.enter) // 1 more request is put in queue, meaning the limit shrinks down to 3. - waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 1) + waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 1) require.Equal(t, 8, gauge.enter) require.Equal(t, 9, gauge.queued) @@ -419,14 +419,14 @@ func TestLimiter_dynamic(t *testing.T) { limit.Update(7) // 5 requests acquired the tokens - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) // Update the limit to 3 limit.Update(3) // 3 requests are put in queue - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 3) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 3) require.Equal(t, 5, gauge.enter) require.Equal(t, 8, gauge.queued) @@ -438,7 +438,7 @@ func TestLimiter_dynamic(t *testing.T) { require.Equal(t, 8, gauge.enter) // 2 more requests - release3, waitAfterRelease3 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 2) + release3, waitAfterRelease3 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 2) require.Equal(t, 10, gauge.enter) // Update the limit to 1 @@ -466,17 +466,17 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 5, nil, gauge) // 1 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 1) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 1) require.Equal(t, 1, gauge.enter) require.Equal(t, 1, gauge.queued) // 5 requests queuing for the tokens, the queue is full now - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 5) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 1, gauge.enter) require.Equal(t, 6, gauge.queued) // Limiter rejects new request - maximumQueueSizeReached(ctx, t, "1", limiter) + maximumQueueSizeReached(t, ctx, "1", limiter) // Update the limit limit.Update(6) @@ -484,12 +484,12 @@ func TestLimiter_dynamic(t *testing.T) { require.Equal(t, 6, gauge.enter) // 5 requests queuing for the tokens, the queue is full now - waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 5) + waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 6, gauge.enter) require.Equal(t, 11, gauge.queued) // Limiter rejects new request - maximumQueueSizeReached(ctx, t, "1", limiter) + maximumQueueSizeReached(t, ctx, "1", limiter) // Clean up close(release1) @@ -512,23 +512,23 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 3, nil, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) require.Equal(t, 5, gauge.queued) // 5 requests queuing for the tokens, the queue is full now - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 3) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 3) require.Equal(t, 5, gauge.enter) require.Equal(t, 8, gauge.queued) // Limiter rejects new request - maximumQueueSizeReached(ctx, t, "1", limiter) + maximumQueueSizeReached(t, ctx, "1", limiter) // Update the limit. limit.Update(3) // The queue is still full - maximumQueueSizeReached(ctx, t, "1", limiter) + maximumQueueSizeReached(t, ctx, "1", limiter) // Release first 5 requests and let the last 3 requests in close(release1) @@ -538,10 +538,10 @@ func TestLimiter_dynamic(t *testing.T) { require.Equal(t, 8, gauge.enter) // Another 5 requests in queue. The queue is still full, meaning the concurrency is 3 and the queue is still 5. - waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 3) + waitAcquired3, release3, waitAfterRelease3 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 3) require.Equal(t, 8, gauge.enter) require.Equal(t, 11, gauge.queued) - maximumQueueSizeReached(ctx, t, "1", limiter) + maximumQueueSizeReached(t, ctx, "1", limiter) // Clean up close(release2) @@ -562,12 +562,12 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 0, nil, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) require.Equal(t, 5, gauge.queued) // 5 more requests - waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(ctx, t, "1", limiter, gauge, 5) + waitAcquired2, release2, waitAfterRelease2 := spawnAndWaitQueued(t, ctx, "1", limiter, gauge, 5) // Update the limit. limit.Update(10) @@ -593,7 +593,7 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 0, func() helper.Ticker { return ticker }, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) require.Equal(t, 5, gauge.queued) @@ -629,7 +629,7 @@ func TestLimiter_dynamic(t *testing.T) { limiter := NewConcurrencyLimiter(ctx, limit, 0, nil, gauge) // 5 requests acquired the tokens, the limiter is full now - release1, waitAfterRelease1 := spawnAndWaitAcquired(ctx, t, "1", limiter, gauge, 5) + release1, waitAfterRelease1 := spawnAndWaitAcquired(t, ctx, "1", limiter, gauge, 5) require.Equal(t, 5, gauge.enter) require.Equal(t, 5, gauge.queued) @@ -667,7 +667,7 @@ func TestLimiter_dynamic(t *testing.T) { // 5 * 5 requests acquired tokens for i := 1; i <= 5; i++ { - release, waitAfterRelease := spawnAndWaitAcquired(ctx, t, fmt.Sprintf("%d", i), limiter, gauge, 5) + release, waitAfterRelease := spawnAndWaitAcquired(t, ctx, fmt.Sprintf("%d", i), limiter, gauge, 5) releaseChans = append(releaseChans, release) waitReleaseFuncs = append(waitReleaseFuncs, waitAfterRelease) } @@ -675,7 +675,7 @@ func TestLimiter_dynamic(t *testing.T) { // 1 + 2 + 3 + 4 + 5 requests are in queue for i := 1; i <= 5; i++ { - waitAcquired, release, waitAfterRelease := spawnAndWaitQueued(ctx, t, fmt.Sprintf("%d", i), limiter, gauge, i) + waitAcquired, release, waitAfterRelease := spawnAndWaitQueued(t, ctx, fmt.Sprintf("%d", i), limiter, gauge, i) waitAcquireFuncs = append(waitAcquireFuncs, waitAcquired) releaseChans = append(releaseChans, release) waitReleaseFuncs = append(waitReleaseFuncs, waitAfterRelease) @@ -706,7 +706,7 @@ func TestLimiter_dynamic(t *testing.T) { // spawnAndWaitAcquired spawns N goroutines that wait for the limiter. They wait until all of them acquire the limiter // token before exiting. This function returns a channel to control token release and a function to wait until all // goroutines finish. -func spawnAndWaitAcquired(ctx context.Context, t *testing.T, bucket string, limiter *ConcurrencyLimiter, gauge *blockingQueueCounter, n int) (chan struct{}, func()) { +func spawnAndWaitAcquired(t *testing.T, ctx context.Context, bucket string, limiter *ConcurrencyLimiter, gauge *blockingQueueCounter, n int) (chan struct{}, func()) { var acquireWg, releaseWg sync.WaitGroup release := make(chan struct{}) @@ -735,7 +735,7 @@ func spawnAndWaitAcquired(ctx context.Context, t *testing.T, bucket string, limi // spawnAndWaitQueued spawns N goroutines that wait for the limiter. They wait until all of them are queued. This // function returns a function to wait for channel to acquired the token, a channel to control token release, and a // function to wait until all goroutines finish. -func spawnAndWaitQueued(ctx context.Context, t *testing.T, bucket string, limiter *ConcurrencyLimiter, gauge *blockingQueueCounter, n int) (func(), chan struct{}, func()) { +func spawnAndWaitQueued(t *testing.T, ctx context.Context, bucket string, limiter *ConcurrencyLimiter, gauge *blockingQueueCounter, n int) (func(), chan struct{}, func()) { var acquireWg, releaseWg sync.WaitGroup release := make(chan struct{}) @@ -775,7 +775,7 @@ func spawnQueuedAndCollectErrors(ctx context.Context, bucket string, limiter *Co } } -func maximumQueueSizeReached(ctx context.Context, t *testing.T, bucket string, limiter *ConcurrencyLimiter) { +func maximumQueueSizeReached(t *testing.T, ctx context.Context, bucket string, limiter *ConcurrencyLimiter) { _, err := limiter.Limit(ctx, bucket, func() (interface{}, error) { return nil, fmt.Errorf("should not call") }) diff --git a/internal/testhelper/testserver/gitaly.go b/internal/testhelper/testserver/gitaly.go index 4f17f66c4..9873ff44a 100644 --- a/internal/testhelper/testserver/gitaly.go +++ b/internal/testhelper/testserver/gitaly.go @@ -175,7 +175,7 @@ func runGitaly(tb testing.TB, cfg config.Cfg, registrar func(srv *grpc.Server, d server.WithStreamInterceptor(StructErrStreamInterceptor), } - deps := gsd.createDependencies(ctx, tb, cfg) + deps := gsd.createDependencies(tb, ctx, cfg) tb.Cleanup(func() { gsd.conns.Close() }) serverFactory := server.NewGitalyServerFactory( @@ -276,7 +276,7 @@ type gitalyServerDeps struct { signingKey string } -func (gsd *gitalyServerDeps) createDependencies(ctx context.Context, tb testing.TB, cfg config.Cfg) *service.Dependencies { +func (gsd *gitalyServerDeps) createDependencies(tb testing.TB, ctx context.Context, cfg config.Cfg) *service.Dependencies { if gsd.logger == nil { gsd.logger = testhelper.NewGitalyServerLogger(tb) } -- cgit v1.2.3