From 89060c80363860fdba2c4d3f2261df185be5ded7 Mon Sep 17 00:00:00 2001 From: Florent Poinsard Date: Thu, 20 Feb 2025 12:01:58 -0600 Subject: [PATCH] review suggestions: more unit tests Signed-off-by: Florent Poinsard --- .../newfeaturetest/reparent_in_tx_test.go | 13 ++- go/test/endtoend/reparent/utils/utils.go | 7 +- go/test/endtoend/tabletgateway/vtgate_test.go | 1 + go/vt/discovery/healthcheck.go | 1 - go/vt/vttablet/tabletconn/grpc_error_test.go | 86 ++++++++++++++++++- 5 files changed, 95 insertions(+), 13 deletions(-) diff --git a/go/test/endtoend/reparent/newfeaturetest/reparent_in_tx_test.go b/go/test/endtoend/reparent/newfeaturetest/reparent_in_tx_test.go index 1ab0fe6dcc3..31426092198 100644 --- a/go/test/endtoend/reparent/newfeaturetest/reparent_in_tx_test.go +++ b/go/test/endtoend/reparent/newfeaturetest/reparent_in_tx_test.go @@ -27,7 +27,6 @@ import ( "vitess.io/vitess/go/test/endtoend/cluster" "vitess.io/vitess/go/test/endtoend/reparent/utils" "vitess.io/vitess/go/vt/vtctl/reparentutil/policy" - "vitess.io/vitess/go/vt/vterrors" ) var primary int @@ -43,7 +42,7 @@ func testCommitError(t *testing.T, conn *mysql.Conn, clusterInstance *cluster.Lo go func() { <-tabletStopped _, err := conn.ExecuteFetch("commit", 0, false) - require.ErrorContains(t, err, vterrors.VT15001(0).ID) + require.ErrorContains(t, err, "VT15001") commitDone <- true }() @@ -63,11 +62,11 @@ func testExecuteError(t *testing.T, conn *mysql.Conn, clusterInstance *cluster.L idx += 5 <-tabletStopped _, err := conn.ExecuteFetch(utils.GetInsertMultipleValuesQuery(idx, idx+1, idx+2, idx+3), 0, false) - require.ErrorContains(t, err, vterrors.VT15001(0).ID) + require.ErrorContains(t, err, "VT15001") // Subsequent queries after a VT15001 should start returning a VT15002 error until we issue a ROLLBACK _, err = conn.ExecuteFetch("select * from vt_insert_test", 1, false) - require.ErrorContains(t, err, vterrors.VT15002().ID) + require.ErrorContains(t, err, "VT15002") _, err = conn.ExecuteFetch("rollback", 0, false) require.NoError(t, err) @@ -123,9 +122,9 @@ func reparent(t *testing.T, clusterInstance *cluster.LocalProcessCluster, tablet } func TestErrorsInTransaction(t *testing.T) { - clusterInstance := utils.SetupShardedReparentCluster(t, policy.DurabilitySemiSync, map[string]string{ - "--queryserver-config-transaction-timeout": "5m", - "--queryserver-config-query-timeout": "5m", + clusterInstance := utils.SetupShardedReparentCluster(t, policy.DurabilitySemiSync, []string{ + "--queryserver-config-transaction-timeout", "5m", + "--queryserver-config-query-timeout", "5m", }) defer utils.TeardownCluster(clusterInstance) diff --git a/go/test/endtoend/reparent/utils/utils.go b/go/test/endtoend/reparent/utils/utils.go index 7a975507dd2..08a97a91ed7 100644 --- a/go/test/endtoend/reparent/utils/utils.go +++ b/go/test/endtoend/reparent/utils/utils.go @@ -77,7 +77,7 @@ func SetupRangeBasedCluster(ctx context.Context, t *testing.T) *cluster.LocalPro } // SetupShardedReparentCluster is used to setup a sharded cluster for testing -func SetupShardedReparentCluster(t *testing.T, durability string, extraVttabletFlags map[string]string) *cluster.LocalProcessCluster { +func SetupShardedReparentCluster(t *testing.T, durability string, extraVttabletFlags []string) *cluster.LocalProcessCluster { clusterInstance := cluster.NewCluster(cell1, Hostname) // Start topo server err := clusterInstance.StartTopo() @@ -89,8 +89,9 @@ func SetupShardedReparentCluster(t *testing.T, durability string, extraVttabletF "--health_check_interval", "1s", "--track_schema_versions=true", "--queryserver_enable_online_ddl=false") - for flag, val := range extraVttabletFlags { - clusterInstance.VtTabletExtraArgs = append(clusterInstance.VtTabletExtraArgs, flag, val) + + if len(extraVttabletFlags) > 0 { + clusterInstance.VtTabletExtraArgs = append(clusterInstance.VtTabletExtraArgs, extraVttabletFlags...) } clusterInstance.VtGateExtraArgs = append(clusterInstance.VtGateExtraArgs, diff --git a/go/test/endtoend/tabletgateway/vtgate_test.go b/go/test/endtoend/tabletgateway/vtgate_test.go index 62973e5285f..6e69ef958ed 100644 --- a/go/test/endtoend/tabletgateway/vtgate_test.go +++ b/go/test/endtoend/tabletgateway/vtgate_test.go @@ -271,6 +271,7 @@ func TestReplicaTransactions(t *testing.T) { require.NoError(t, err) serving := replicaTablet.VttabletProcess.WaitForStatus("SERVING", 60*time.Second) assert.Equal(t, serving, true, "Tablet did not become ready within a reasonable time") + utils.Exec(t, readConn, fetchAllCustomers) // create a new connection, should be able to query again readConn, err = mysql.Connect(ctx, &vtParams) diff --git a/go/vt/discovery/healthcheck.go b/go/vt/discovery/healthcheck.go index 2f270bd7518..3fe3e93f491 100644 --- a/go/vt/discovery/healthcheck.go +++ b/go/vt/discovery/healthcheck.go @@ -871,7 +871,6 @@ func (hc *HealthCheckImpl) TabletConnection(ctx context.Context, alias *topodata thc := hc.healthByAlias[tabletAliasString(topoproto.TabletAliasString(alias))] hc.mu.Unlock() if thc == nil || thc.Conn == nil { - // TODO: test that throws this error return nil, vterrors.Errorf(vtrpc.Code_NOT_FOUND, "tablet: %v is either down or nonexistent", alias) } return thc.Connection(ctx), nil diff --git a/go/vt/vttablet/tabletconn/grpc_error_test.go b/go/vt/vttablet/tabletconn/grpc_error_test.go index daf3ee3306c..8bdc09d0e2d 100644 --- a/go/vt/vttablet/tabletconn/grpc_error_test.go +++ b/go/vt/vttablet/tabletconn/grpc_error_test.go @@ -19,6 +19,10 @@ package tabletconn import ( "testing" + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc" "vitess.io/vitess/go/vt/vterrors" ) @@ -48,8 +52,86 @@ func TestTabletErrorFromRPCError(t *testing.T) { }} for _, tcase := range testcases { got := vterrors.Code(ErrorFromVTRPC(tcase.in)) - if got != tcase.want { - t.Errorf("FromVtRPCError(%v):\n%v, want\n%v", tcase.in, got, tcase.want) + require.Equal(t, tcase.want, got) + } +} + +func TestGRPCErrorToVT15001(t *testing.T) { + msg := "connection error: desc = \"transport: Error while dialing: dial tcp 127.0.0.1:7108: connect: connection refused\"" + testcases := []struct { + in error + inTx bool + wrap bool + }{{ + in: status.Error(codes.Unavailable, msg), + inTx: true, + wrap: true, + }, { + in: status.Error(codes.Unavailable, msg), + inTx: false, + wrap: false, + }, { + in: status.Error(codes.Unavailable, "unavailable"), + inTx: true, + wrap: false, + }, { + in: status.Error(codes.FailedPrecondition, msg), + inTx: true, + wrap: false, + }} + for _, tcase := range testcases { + got := ErrorsFromGRPCWrapTransientTxError(tcase.in, tcase.inTx) + require.Error(t, got) + if tcase.wrap { + require.ErrorContains(t, got, "VT15001") + } else { + require.NotContains(t, got.Error(), "VT15001") + } + } +} + +func TestVTRPCErrorToVT15001(t *testing.T) { + msg := "connection error: desc = \"transport: Error while dialing: dial tcp 127.0.0.1:7108: connect: connection refused\"" + testcases := []struct { + in *vtrpcpb.RPCError + inTx bool + wrap bool + }{{ + in: &vtrpcpb.RPCError{ + Code: vtrpcpb.Code_UNAVAILABLE, + Message: msg, + }, + inTx: true, + wrap: true, + }, { + in: &vtrpcpb.RPCError{ + Code: vtrpcpb.Code_UNAVAILABLE, + Message: msg, + }, + inTx: false, + wrap: false, + }, { + in: &vtrpcpb.RPCError{ + Code: vtrpcpb.Code_UNAVAILABLE, + Message: "unavailable", + }, + inTx: true, + wrap: false, + }, { + in: &vtrpcpb.RPCError{ + Code: vtrpcpb.Code_FAILED_PRECONDITION, + Message: msg, + }, + inTx: true, + wrap: false, + }} + for _, tcase := range testcases { + got := ErrorsFromVTRPCWrapTransientTxError(tcase.in, tcase.inTx) + require.Error(t, got) + if tcase.wrap { + require.ErrorContains(t, got, "VT15001") + } else { + require.NotContains(t, got.Error(), "VT15001") } } }