-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
orc tests: add more cases #6801
Conversation
Signed-off-by: deepthi <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, some improvements suggested, not necessarily for this PR
err := curMaster.MysqlctlProcess.Stop() | ||
require.NoError(t, err) | ||
|
||
for _, tablet := range shard0.Vttablets { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where's the Sleep()
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is inside the call to checkMasterTablet
qr := runSQL(t, "select @@global.read_only", curMaster) | ||
require.NotNil(t, qr) | ||
require.Equal(t, 1, len(qr.Rows)) | ||
require.Equal(t, "[[INT64(0)]]", fmt.Sprintf("%s", qr.Rows), qr.Rows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
require.NotNil(t, qr) | ||
require.Equal(t, 1, len(qr.Rows)) | ||
require.Equal(t, "[[INT64(1)]]", fmt.Sprintf("%s", qr.Rows), qr.Rows) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// wait for repair | ||
time.Sleep(15 * time.Second) | ||
// check replication is setup correctly | ||
checkReplication(t, clusterInstance, curMaster, []*cluster.Vttablet{replica}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like these tests share a lot of code in common, the code should probably be refactored to provide a framework for setup-destroy-wait-expect cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved as much as possible into createCluster and checkReplication. What is left is slightly different in each test case.
As we add more tests we can revisit and refactor.
Partial fix for #6769
Signed-off-by: deepthi [email protected]