integration tests: wait for index creation #407
Conversation
There was a problem hiding this comment.
I reviewed only last commit 3f4bdd7, and LGTM.
(the comment 👇 is not blocker)
cmdtests/sql_test.go
Outdated
| visibleValue := strings.TrimSpace(strings.Split(indexLine, "|")[14]) | ||
| var visibleValue string | ||
| // building/loading index takes some time, wait maximum 15s | ||
| for i := 0; i < 15; i++ { |
There was a problem hiding this comment.
For the simple index we are creating in this testcase, 15s can be enough.
with bigger datasets (as could be needed in the future), it would not be enough.
There was a problem hiding this comment.
15s is a lot. I would prefer it to fail when we add such a big dataset. And increase timeout/reduce dataset depends on the case.
Yup the only other place is still unmerged in #363. |
|
Ok. I'll wait for it to be merged and then rebase. |
Signed-off-by: Maxim Sukharev <[email protected]>
|
I have reworked the PR due to different check in regression test. Please another pass. |
cmdtests/sql_index_util.go
Outdated
| return visibleValue | ||
| } | ||
|
|
||
| func hasIndex(s commandSuite, table, name string) string { |
There was a problem hiding this comment.
Does the visible column have other values than "YES" and "NO"?
There was a problem hiding this comment.
according to mysql documentation no. At least there is no mention of any other value.
There was a problem hiding this comment.
I'd then change the return value to bool so that the handling of "YES" and "NO" values is done only within this function.
Signed-off-by: Maxim Sukharev <[email protected]>
|
LGTM 👍 |
| s.Require().NoError(r.Error, r.Combined()) | ||
|
|
||
| // parse result and check that correct index was built and it is visiable | ||
| lines := strings.Split(r.Stdout(), "\n") |
There was a problem hiding this comment.
Maybe we could add an example output. This way if the format changes in the future we can compare it and fix the code easier.
There was a problem hiding this comment.
I added a link instead of output itself (it's huge). The link contains version of mysql so it won't change.
Signed-off-by: Maxim Sukharev <[email protected]>
Fix: #378