Conversation
18456d7 to
5e324a4
Compare
Totktonada
left a comment
There was a problem hiding this comment.
Thanks! This is a nice code health activity.
95ebd8c to
e05da81
Compare
|
@ImeevMA, I've update a couple of SQL tests, please take a look on changes. |
ImeevMA
left a comment
There was a problem hiding this comment.
I think it makes sense to rename the test files to map_binding_test.lua and array_binding_test.lua to indicate that binding is tested separately. I think it would otherwise be quite odd to have two test-files with the same name implementing complementary rather than replacing tests.
As discussed verbally with you, I've updated testcase names as in patch below: diff --git a/test/sql-luatest/array_test.lua b/test/sql-luatest/array_test.lua
index fe40e3d12b..bbb566de66 100644
--- a/test/sql-luatest/array_test.lua
+++ b/test/sql-luatest/array_test.lua
@@ -13,7 +13,7 @@ g.after_all(function()
end)
-- Make sure that ARRAY values can be used as bound variable.
-g.test_builtins_14_1 = function()
+g.test_array_binding_local = function()
g.server:exec(function()
local sql = [[SELECT #a;]]
local arg = {{['#a'] = {1, 2, 3}}}
@@ -21,7 +21,7 @@ g.test_builtins_14_1 = function()
end)
end
-g.test_builtins_14_2 = function()
+g.test_array_binding_remote = function()
local conn = g.server.net_box
local ok, res = pcall(conn.execute, conn, [[SELECT #a;]],
{{['#a'] = {1, 2, 3}}})
diff --git a/test/sql-luatest/map_test.lua b/test/sql-luatest/map_test.lua
index fefa4e7255..679907dcc9 100644
--- a/test/sql-luatest/map_test.lua
+++ b/test/sql-luatest/map_test.lua
@@ -322,7 +322,7 @@ g.test_map_9 = function()
end
-- Make sure that MAP values can be used as a bound variable.
-g.test_builtins_13_1 = function()
+g.test_map_binding_local = function()
g.server:exec(function()
local sql = [[SELECT #a;]]
local arg = {{['#a'] = {abc = 2, [1] = 3}}}
@@ -331,7 +331,7 @@ g.test_builtins_13_1 = function()
end)
end
-g.test_builtins_13_2 = function()
+g.test_map_binding_remote = function()
local conn = g.server.net_box
local ok, res = pcall(conn.execute, conn, [[SELECT #a;]],
{{['#a'] = {abc = 2, [1] = 3}}})
|
e05da81 to
8bb6196
Compare
Buristan
left a comment
There was a problem hiding this comment.
Hi, Sergey!
Thanks for the patchset!
It is nice to see that someone dusts off these old tests.
Generally LGTM, except a few minor nits and comments below:
[PATCH 1/8] cmake: remove duplicate definition
perf/CMakeLists.txt, the patch removes duplicate definition.
Typo: s/duplicate/the duplicate/
[PATCH 2/8] test: remove test in suite.ini
used error injection anymore, so it was removed in suite.ini.
Typo: s/error injection/the error injection/
[PATCH 3/8] test: remove feedback_daemon.skipcond
because depends on environment variable
TRAVIS_JOB_ID.
Typo: s/depends/it depends/
Typo: s/environment/the environment/
[PATCH 4/8] test: fix test filename
triggered with patch for CTest support and breaks test generation:
Typo:s/patch/the patch/
[PATCH 5/8] test: use built tarantool instead a system one
The test gh_5747_crash_multiple_args_test.lua use a tarantool
Typo: s/use/uses/
Typo: s/tarantool/Tarantool/
In some cases, PATH may point to a system tarantool binary and
Typo: s/tarantool binary/Tarantool binary,/
set a path to tarantool binary derived from a command-line
Typo: s/tarantool binary/the Tarantool binary/
And the comment below.
[PATCH 6/8] test: fix minimal.test.lua
The patch set a patch to tarantool binary explicitly in the
Typo: s/patch/path/
Typo: s/tarantool/the Tarantool/
[PATCH 7/8] test: remove testcase for tarantoolctl
but this tools has been deprecated an removed for a long time.
Typo: s/tools/tool/
Typo: s/an/and/
[PATCH 8/8] test: replace testcase by luatest's testcases
The tests
array.test.luaandmap.test.luarequires remote
Typo: s/requires/require/
Typo: s/remote/the remote/
Tarantool instance for running testcases. When test are executed
Typo: s/test/tests/
by test-run.py it runs this Tarantool instance. The testcases that
Typo: s/test-run.py/test-run.py,/
require remote instance were ported to luatest to make tests able
Typo: s/remote/the remote/
`TARANTOOL_BIN` is already defined in a parent `perf/CMakeLists.txt`, the patch removes the duplicate definition. NO_CHANGELOG=codehealth NO_DOC=codehealth NO_TEST=codehealth
The test has been renamed in commit 16d6e9d ("console: remove ERRINJ_STDIN_ISATTY injection") and it is not used the error injection anymore, so it was removed in suite.ini. NO_CHANGELOG=codehealth NO_DOC=codehealth NO_TEST=codehealth
The aforementioned skipcond file is specific for Travis CI because it depends on the environment variable `TRAVIS_JOB_ID`. We have moved to GHA for a long time, it seems this skipcond is not useful anymore. NO_CHANGELOG=codehealth NO_DOC=codehealth NO_TEST=codehealth
According to CMake policy CMP0110 whitespaces and other special
characters are forbidden before CMake 3.19. This policy is
triggered with the patch for CTest support and breaks test
generation:
NO_WRAP
The following name given to add_test() is invalid if CMP0110 is not set or
set to OLD:
`test/box-luatest/gh_7217_repeatable_{in, up}sert_memtx_tx_conflict_test.lua´
NO_WRAP
The patch renames file without using whitespaces and special
characters.
Required for CTest support. Follows up commit 654cf49
("memtx: fix story delete statement list").
1. https://cmake.org/cmake/help/latest/policy/CMP0110.html
NO_CHANGELOG=codehealth
NO_DOC=codehealth
NO_TEST=codehealth
The test gh_5747_crash_multiple_args_test.lua uses a `tarantool` binary that is available in a current PATH environment variable. In some cases, PATH may point to a system `tarantool` executable and this could be unexpected for those who run the test. The patch set a path to `tarantool` executable derived from a command-line used to run the test. NO_CHANGELOG=codehealth NO_DOC=codehealth
The patch set a path to `tarantool` executable binary explicitly in the aforementioned test. NO_CHANGELOG=codehealth NO_DOC=codehealth
The testcase `test_tarantoolctl_connect` tests `tarantoolctl`, but this tool has been deprecated and removed for a long time. The patch removes testcases as useless. Follows up commit 216b624 ("tools: remove tarantoolctl utility"). NO_CHANGELOG=codehealth NO_DOC=codehealth
The tests `array.test.lua` and `map.test.lua` require the remote Tarantool instance for running testcases. When tests are executed by test-run.py, it runs this Tarantool instance. The testcases that require the remote instance were ported to luatest to make tests able to run with ctest. The testcases `builtins-13.1` and `builtins-14.1` are companion testcases and were moved as well. Required by patches for CTest support, because allows to execute aforementioned tests without test-run.py. NO_CHANGELOG=codehealth NO_DOC=codehealth NO_TEST=codehealth
8bb6196 to
77dabd5
Compare
Needed by #10216