From 9b4e1ddbe53ff7a4c4c4777dc133238d672d92f6 Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Wed, 22 May 2024 11:34:12 +0200 Subject: [PATCH 1/3] Refactor existing tests to make it easer to add more Signed-off-by: Rohit Nayak --- go/vt/topo/shard_test.go | 189 ++++++++++++++++++++++++++++----------- 1 file changed, 137 insertions(+), 52 deletions(-) diff --git a/go/vt/topo/shard_test.go b/go/vt/topo/shard_test.go index ccef80944a9..e4b5e3c1038 100644 --- a/go/vt/topo/shard_test.go +++ b/go/vt/topo/shard_test.go @@ -140,70 +140,155 @@ func TestUpdateSourcePrimaryDeniedTables(t *testing.T) { func TestUpdateSourceDeniedTables(t *testing.T) { si := NewShardInfo("ks", "sh", &topodatapb.Shard{}, nil) - - // check we enforce the keyspace lock ctx := context.Background() - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, nil, false, nil); err == nil || err.Error() != "keyspace ks is not locked (no locksInfo)" { - t.Fatalf("unlocked keyspace produced wrong error: %v", err) + ctxWithLock := lockedKeyspaceContext("ks") + + type testCase struct { + name string + ctx context.Context + tabletType topodatapb.TabletType + cells []string + remove bool + tables []string + + wantError string + wantTabletControl *topodatapb.Shard_TabletControl } - ctx = lockedKeyspaceContext("ks") - // add one cell - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ + testCases := []testCase{ { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first"}, - DeniedTables: []string{"t1", "t2"}, - }, - }) { - t.Fatalf("one cell add failed: %v", si) - } - - // remove that cell, going back - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, true, nil); err != nil || len(si.TabletControls) != 0 { - t.Fatalf("going back should have remove the record: %v", si) - } + name: "enforce keyspace lock", + ctx: ctx, + tabletType: topodatapb.TabletType_RDONLY, - // re-add a cell, then another with different table list to - // make sure it fails - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, false, []string{"t1", "t2"}); err != nil { - t.Fatalf("one cell add failed: %v", si) - } - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t2", "t3"}); err == nil || err.Error() != "trying to use two different sets of denied tables for shard ks/sh: [t1 t2] and [t2 t3]" { - t.Fatalf("different table list should fail: %v", err) - } - // add another cell, see the list grow - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ + wantError: "keyspace ks is not locked (no locksInfo)", + }, { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "second"}, - DeniedTables: []string{"t1", "t2"}, + name: "add one cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first"}, + DeniedTables: []string{"t1", "t2"}, + }, }, - }) { - t.Fatalf("second cell add failed: %v", si) - } - - // add all cells, see the list grow to all - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first", "second", "third"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "second", "third"}, - DeniedTables: []string{"t1", "t2"}, + name: "remove the only cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + remove: true, + }, + { + name: "re-add cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "re-add existing cell, different tables, should fail", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, + tables: []string{"t3"}, + wantError: "trying to use two different sets of denied tables for shard", }, - }) { - t.Fatalf("all cells add failed: %v", si) - } - - // remove one cell from the full list - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, true, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "third"}, - DeniedTables: []string{"t1", "t2"}, + name: "add all cells, see cell list grow to all", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first", "second", "third"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "second", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "remove one cell", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"second"}, + remove: true, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, }, - }) { - t.Fatalf("one cell removal from all failed: %v", si) } + + for _, tcase := range testCases { + t.Run(tcase.name, func(t *testing.T) { + if tcase.ctx == nil { + tcase.ctx = ctxWithLock + } + + err := si.UpdateDeniedTables(tcase.ctx, tcase.tabletType, tcase.cells, tcase.remove, tcase.tables) + if tcase.wantError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tcase.wantError) + return + } + require.NoError(t, err) + if tcase.wantTabletControl == nil { + require.Nil(t, si.GetTabletControl(tcase.tabletType)) + } else { + require.Truef(t, reflect.DeepEqual(tcase.wantTabletControl, si.GetTabletControl(tcase.tabletType)), + "want: %v, got: %v", tcase.wantTabletControl, si.GetTabletControl(tcase.tabletType)) + } + }) + } + /* + + + + // re-add a cell, then another with different table list to + // make sure it fails + if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, false, []string{"t1", "t2"}); err != nil { + t.Fatalf("one cell add failed: %v", si) + } + if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t2", "t3"}); err == nil || err.Error() != "trying to use two different sets of denied tables for shard ks/sh: [t1 t2] and [t2 t3]" { + t.Fatalf("different table list should fail: %v", err) + } + // add another cell, see the list grow + if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ + { + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "second"}, + DeniedTables: []string{"t1", "t2"}, + }, + }) { + t.Fatalf("second cell add failed: %v", si) + } + + // add all cells, see the list grow to all + if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first", "second", "third"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ + { + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "second", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, + }) { + t.Fatalf("all cells add failed: %v", si) + } + + // remove one cell from the full list + if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, true, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ + { + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, + }) { + t.Fatalf("one cell removal from all failed: %v", si) + } + */ } func TestValidateShardName(t *testing.T) { From d76d73c597bd8143f8a85cc02b5cb50be825b5fc Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Wed, 22 May 2024 14:19:59 +0200 Subject: [PATCH 2/3] Add more unit tests. Remove old tests kept previously in comments. Signed-off-by: Rohit Nayak --- go/vt/topo/shard_test.go | 88 +++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 47 deletions(-) diff --git a/go/vt/topo/shard_test.go b/go/vt/topo/shard_test.go index e4b5e3c1038..81dc563a740 100644 --- a/go/vt/topo/shard_test.go +++ b/go/vt/topo/shard_test.go @@ -155,11 +155,14 @@ func TestUpdateSourceDeniedTables(t *testing.T) { wantTabletControl *topodatapb.Shard_TabletControl } + // These tests update the state of the shard tablet controls, so subsequent tests + // depend on the cumulative state from the previous tests. testCases := []testCase{ { name: "enforce keyspace lock", ctx: ctx, tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first"}, wantError: "keyspace ks is not locked (no locksInfo)", }, @@ -221,6 +224,40 @@ func TestUpdateSourceDeniedTables(t *testing.T) { DeniedTables: []string{"t1", "t2"}, }, }, + { + name: "add replica tablet type", + tabletType: topodatapb.TabletType_REPLICA, + cells: []string{"first"}, + tables: []string{"t1", "t2"}, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_REPLICA, + Cells: []string{"first"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "confirm rdonly still stays the same, after replica was added", + tabletType: topodatapb.TabletType_RDONLY, + wantTabletControl: &topodatapb.Shard_TabletControl{ + TabletType: topodatapb.TabletType_RDONLY, + Cells: []string{"first", "third"}, + DeniedTables: []string{"t1", "t2"}, + }, + }, + { + name: "remove rdonly entry", + tabletType: topodatapb.TabletType_RDONLY, + cells: []string{"first", "third"}, + remove: true, + tables: []string{"t1", "t2"}, + }, + { + name: "remove replica entry", + tabletType: topodatapb.TabletType_REPLICA, + cells: []string{"first", "third"}, + remove: true, + tables: []string{"t1", "t2"}, + }, } for _, tcase := range testCases { @@ -228,8 +265,10 @@ func TestUpdateSourceDeniedTables(t *testing.T) { if tcase.ctx == nil { tcase.ctx = ctxWithLock } - - err := si.UpdateDeniedTables(tcase.ctx, tcase.tabletType, tcase.cells, tcase.remove, tcase.tables) + var err error + if tcase.tables != nil || tcase.cells != nil { + err = si.UpdateDeniedTables(tcase.ctx, tcase.tabletType, tcase.cells, tcase.remove, tcase.tables) + } if tcase.wantError != "" { require.Error(t, err) require.Contains(t, err.Error(), tcase.wantError) @@ -244,51 +283,6 @@ func TestUpdateSourceDeniedTables(t *testing.T) { } }) } - /* - - - - // re-add a cell, then another with different table list to - // make sure it fails - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first"}, false, []string{"t1", "t2"}); err != nil { - t.Fatalf("one cell add failed: %v", si) - } - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t2", "t3"}); err == nil || err.Error() != "trying to use two different sets of denied tables for shard ks/sh: [t1 t2] and [t2 t3]" { - t.Fatalf("different table list should fail: %v", err) - } - // add another cell, see the list grow - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ - { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "second"}, - DeniedTables: []string{"t1", "t2"}, - }, - }) { - t.Fatalf("second cell add failed: %v", si) - } - - // add all cells, see the list grow to all - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"first", "second", "third"}, false, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ - { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "second", "third"}, - DeniedTables: []string{"t1", "t2"}, - }, - }) { - t.Fatalf("all cells add failed: %v", si) - } - - // remove one cell from the full list - if err := si.UpdateDeniedTables(ctx, topodatapb.TabletType_RDONLY, []string{"second"}, true, []string{"t1", "t2"}); err != nil || !reflect.DeepEqual(si.TabletControls, []*topodatapb.Shard_TabletControl{ - { - TabletType: topodatapb.TabletType_RDONLY, - Cells: []string{"first", "third"}, - DeniedTables: []string{"t1", "t2"}, - }, - }) { - t.Fatalf("one cell removal from all failed: %v", si) - } - */ } func TestValidateShardName(t *testing.T) { From 152a2ae8c4d7239c378adabf6c9676be04625dbb Mon Sep 17 00:00:00 2001 From: Rohit Nayak Date: Mon, 27 May 2024 09:37:17 +0200 Subject: [PATCH 3/3] Address review comemnt Signed-off-by: Rohit Nayak --- go/vt/topo/shard_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/topo/shard_test.go b/go/vt/topo/shard_test.go index 81dc563a740..9afc8d0ea78 100644 --- a/go/vt/topo/shard_test.go +++ b/go/vt/topo/shard_test.go @@ -278,7 +278,7 @@ func TestUpdateSourceDeniedTables(t *testing.T) { if tcase.wantTabletControl == nil { require.Nil(t, si.GetTabletControl(tcase.tabletType)) } else { - require.Truef(t, reflect.DeepEqual(tcase.wantTabletControl, si.GetTabletControl(tcase.tabletType)), + require.EqualValuesf(t, tcase.wantTabletControl, si.GetTabletControl(tcase.tabletType), "want: %v, got: %v", tcase.wantTabletControl, si.GetTabletControl(tcase.tabletType)) } })