From 97b7ba90f3aa598973596bc09e246a4ef09792b5 Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Fri, 17 Jan 2025 11:36:35 -0800 Subject: [PATCH 1/2] [Access] Fix access connection tests for latest version of testing library --- .../access/rpc/connection/connection_test.go | 67 ++++++------------- .../grpc_compression_benchmark_test.go | 2 +- engine/access/rpc/connection/node_mock.go | 28 ++++++-- 3 files changed, 43 insertions(+), 54 deletions(-) diff --git a/engine/access/rpc/connection/connection_test.go b/engine/access/rpc/connection/connection_test.go index 33e993f0b8b..160be947508 100644 --- a/engine/access/rpc/connection/connection_test.go +++ b/engine/access/rpc/connection/connection_test.go @@ -34,7 +34,7 @@ func TestProxyAccessAPI(t *testing.T) { metrics := metrics.NewNoopCollector() // create a collection node - cn := new(collectionNode) + cn := newCollectionNode(t) cn.start(t) defer cn.stop(t) @@ -75,7 +75,7 @@ func TestProxyAccessAPI(t *testing.T) { // make the call to the collection node resp, err := client.Ping(ctx, req) assert.NoError(t, err) - assert.Equal(t, resp, expected) + assert.IsType(t, expected, resp) } func TestProxyExecutionAPI(t *testing.T) { @@ -83,7 +83,7 @@ func TestProxyExecutionAPI(t *testing.T) { metrics := metrics.NewNoopCollector() // create an execution node - en := new(executionNode) + en := newExecutionNode(t) en.start(t) defer en.stop(t) @@ -124,7 +124,7 @@ func TestProxyExecutionAPI(t *testing.T) { // make the call to the execution node resp, err := client.Ping(ctx, req) assert.NoError(t, err) - assert.Equal(t, resp, expected) + assert.IsType(t, expected, resp) } func TestProxyAccessAPIConnectionReuse(t *testing.T) { @@ -132,7 +132,7 @@ func TestProxyAccessAPIConnectionReuse(t *testing.T) { metrics := metrics.NewNoopCollector() // create a collection node - cn := new(collectionNode) + cn := newCollectionNode(t) cn.start(t) defer cn.stop(t) @@ -186,7 +186,7 @@ func TestProxyAccessAPIConnectionReuse(t *testing.T) { ctx := context.Background() resp, err := accessAPIClient.Ping(ctx, req) assert.NoError(t, err) - assert.Equal(t, resp, expected) + assert.IsType(t, expected, resp) } func TestProxyExecutionAPIConnectionReuse(t *testing.T) { @@ -194,7 +194,7 @@ func TestProxyExecutionAPIConnectionReuse(t *testing.T) { metrics := metrics.NewNoopCollector() // create an execution node - en := new(executionNode) + en := newExecutionNode(t) en.start(t) defer en.stop(t) @@ -248,7 +248,7 @@ func TestProxyExecutionAPIConnectionReuse(t *testing.T) { ctx := context.Background() resp, err := executionAPIClient.Ping(ctx, req) assert.NoError(t, err) - assert.Equal(t, resp, expected) + assert.IsType(t, expected, resp) } // TestExecutionNodeClientTimeout tests that the execution API client times out after the timeout duration @@ -259,7 +259,7 @@ func TestExecutionNodeClientTimeout(t *testing.T) { timeout := 10 * time.Millisecond // create an execution node - en := new(executionNode) + en := newExecutionNode(t) en.start(t) defer en.stop(t) @@ -316,7 +316,7 @@ func TestCollectionNodeClientTimeout(t *testing.T) { timeout := 10 * time.Millisecond // create a collection node - cn := new(collectionNode) + cn := newCollectionNode(t) cn.start(t) defer cn.stop(t) @@ -371,7 +371,7 @@ func TestConnectionPoolFull(t *testing.T) { metrics := metrics.NewNoopCollector() // create a collection node - cn1, cn2, cn3 := new(collectionNode), new(collectionNode), new(collectionNode) + cn1, cn2, cn3 := newCollectionNode(t), newCollectionNode(t), newCollectionNode(t) cn1.start(t) cn2.start(t) cn3.start(t) @@ -379,23 +379,6 @@ func TestConnectionPoolFull(t *testing.T) { defer cn2.stop(t) defer cn3.stop(t) - expected := &access.PingResponse{} - cn1.handler. - On("Ping", - testifymock.Anything, - testifymock.AnythingOfType("*access.PingRequest")). - Return(expected, nil) - cn2.handler. - On("Ping", - testifymock.Anything, - testifymock.AnythingOfType("*access.PingRequest")). - Return(expected, nil) - cn3.handler. - On("Ping", - testifymock.Anything, - testifymock.AnythingOfType("*access.PingRequest")). - Return(expected, nil) - // create the factory connectionFactory := new(ConnectionFactoryImpl) // set the collection grpc port @@ -467,7 +450,7 @@ func TestConnectionPoolStale(t *testing.T) { metrics := metrics.NewNoopCollector() // create a collection node - cn := new(collectionNode) + cn := newCollectionNode(t) cn.start(t) defer cn.stop(t) @@ -534,7 +517,7 @@ func TestConnectionPoolStale(t *testing.T) { ctx = context.Background() resp, err := accessAPIClient.Ping(ctx, req) assert.NoError(t, err) - assert.Equal(t, resp, expected) + assert.IsType(t, expected, resp) } // TestExecutionNodeClientClosedGracefully tests the scenario where the execution node client is closed gracefully. @@ -550,7 +533,7 @@ func TestExecutionNodeClientClosedGracefully(t *testing.T) { // Add createExecNode function to recreate it each time for rapid test createExecNode := func() (*executionNode, func()) { - en := new(executionNode) + en := newExecutionNode(t) en.start(t) return en, func() { en.stop(t) @@ -650,7 +633,7 @@ func TestEvictingCacheClients(t *testing.T) { metrics := metrics.NewNoopCollector() // Create a new collection node for testing - cn := new(collectionNode) + cn := newCollectionNode(t) cn.start(t) defer cn.stop(t) @@ -674,10 +657,6 @@ func TestEvictingCacheClients(t *testing.T) { func(context.Context, *access.PingRequest) error { return nil }, ) - netReq := &access.GetNetworkParametersRequest{} - netResp := &access.GetNetworkParametersResponse{} - cn.handler.On("GetNetworkParameters", testifymock.Anything, netReq).Return(netResp, nil) - // Create the connection factory connectionFactory := new(ConnectionFactoryImpl) // Set the gRPC port @@ -740,7 +719,7 @@ func TestEvictingCacheClients(t *testing.T) { }, 100*time.Millisecond, 10*time.Millisecond, "client timed out closing connection") // Call a gRPC method on the client, requests should be blocked since the connection is invalidated - resp, err := client.GetNetworkParameters(ctx, netReq) + resp, err := client.GetNetworkParameters(ctx, &access.GetNetworkParametersRequest{}) assert.Equal(t, status.Errorf(codes.Unavailable, "the connection to %s was closed", clientAddress), err) assert.Nil(t, resp) @@ -749,9 +728,7 @@ func TestEvictingCacheClients(t *testing.T) { // Call a gRPC method on the client _, err = client.Ping(ctx, pingReq) - // Check that Ping was called - cn.handler.AssertCalled(t, "Ping", testifymock.Anything, pingReq) - assert.NoError(t, err) + require.NoError(t, err) // Wait for the client connection to change state from "Ready" to "Shutdown" as connection was closed. require.Eventually(t, func() bool { @@ -770,7 +747,7 @@ func TestConcurrentConnections(t *testing.T) { // Add createExecNode function to recreate it each time for rapid test createExecNode := func() (*executionNode, func()) { - en := new(executionNode) + en := newExecutionNode(t) en.start(t) return en, func() { en.stop(t) @@ -886,7 +863,7 @@ func TestCircuitBreakerExecutionNode(t *testing.T) { circuitBreakerRestoreTimeout := 1500 * time.Millisecond // Create an execution node for testing. - en := new(executionNode) + en := newExecutionNode(t) en.start(t) defer en.stop(t) @@ -934,8 +911,6 @@ func TestCircuitBreakerExecutionNode(t *testing.T) { // Make the call to the execution node. _, err = client.Ping(ctx, req) - en.handler.AssertCalled(t, "Ping", testifymock.Anything, req) - return time.Since(start), err } @@ -1005,7 +980,7 @@ func TestCircuitBreakerCollectionNode(t *testing.T) { circuitBreakerRestoreTimeout := 1500 * time.Millisecond // Create a collection node for testing. - cn := new(collectionNode) + cn := newCollectionNode(t) cn.start(t) defer cn.stop(t) @@ -1053,8 +1028,6 @@ func TestCircuitBreakerCollectionNode(t *testing.T) { // Make the call to the collection node. _, err = client.Ping(ctx, req) - cn.handler.AssertCalled(t, "Ping", testifymock.Anything, req) - return time.Since(start), err } diff --git a/engine/access/rpc/connection/grpc_compression_benchmark_test.go b/engine/access/rpc/connection/grpc_compression_benchmark_test.go index 6ab86fa39a4..485d0f9c4b2 100644 --- a/engine/access/rpc/connection/grpc_compression_benchmark_test.go +++ b/engine/access/rpc/connection/grpc_compression_benchmark_test.go @@ -38,7 +38,7 @@ func BenchmarkWithDeflateCompression(b *testing.B) { // runBenchmark is a helper function that performs the benchmarking for different compressors. func runBenchmark(b *testing.B, compressorName string) { // create an execution node - en := new(executionNode) + en := newExecutionNode(b) en.start(b) defer en.stop(b) diff --git a/engine/access/rpc/connection/node_mock.go b/engine/access/rpc/connection/node_mock.go index 31a59fd490f..a4618184f83 100644 --- a/engine/access/rpc/connection/node_mock.go +++ b/engine/access/rpc/connection/node_mock.go @@ -64,11 +64,19 @@ type executionNode struct { handler *mock.ExecutionAPIServer } +func newExecutionNode(tb testing.TB) *executionNode { + return &executionNode{ + handler: mock.NewExecutionAPIServer(tb), + } +} + func (en *executionNode) start(tb testing.TB) { + if en.handler == nil { + tb.Fatalf("executionNode must be initialized using newExecutionNode") + } + en.setupNode(tb) - handler := new(mock.ExecutionAPIServer) - execution.RegisterExecutionAPIServer(en.server, handler) - en.handler = handler + execution.RegisterExecutionAPIServer(en.server, en.handler) en.node.start(tb) } @@ -81,11 +89,19 @@ type collectionNode struct { handler *mock.AccessAPIServer } +func newCollectionNode(tb testing.TB) *collectionNode { + return &collectionNode{ + handler: mock.NewAccessAPIServer(tb), + } +} + func (cn *collectionNode) start(t *testing.T) { + if cn.handler == nil { + t.Fatalf("collectionNode must be initialized using newCollectionNode") + } + cn.setupNode(t) - handler := new(mock.AccessAPIServer) - access.RegisterAccessAPIServer(cn.server, handler) - cn.handler = handler + access.RegisterAccessAPIServer(cn.server, cn.handler) cn.node.start(t) } From e075836a472449a57b7f48afbca01af4353357ef Mon Sep 17 00:00:00 2001 From: Peter Argue <89119817+peterargue@users.noreply.github.com> Date: Fri, 17 Jan 2025 11:43:45 -0800 Subject: [PATCH 2/2] update collectionNode mock to use testing.TB interface --- engine/access/rpc/connection/node_mock.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/engine/access/rpc/connection/node_mock.go b/engine/access/rpc/connection/node_mock.go index a4618184f83..af613f4ffc8 100644 --- a/engine/access/rpc/connection/node_mock.go +++ b/engine/access/rpc/connection/node_mock.go @@ -95,16 +95,16 @@ func newCollectionNode(tb testing.TB) *collectionNode { } } -func (cn *collectionNode) start(t *testing.T) { +func (cn *collectionNode) start(tb testing.TB) { if cn.handler == nil { - t.Fatalf("collectionNode must be initialized using newCollectionNode") + tb.Fatalf("collectionNode must be initialized using newCollectionNode") } - cn.setupNode(t) + cn.setupNode(tb) access.RegisterAccessAPIServer(cn.server, cn.handler) - cn.node.start(t) + cn.node.start(tb) } -func (cn *collectionNode) stop(t *testing.T) { - cn.node.stop(t) +func (cn *collectionNode) stop(tb testing.TB) { + cn.node.stop(tb) }