[Tarantool-patches] [PATCH v4 4/4] test: add tests for truncation and deletion
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat Feb 15 02:50:05 MSK 2020
Thanks for the patch!
Please, put tests to the same commit as the patch.
See 10 comments below.
On 14/02/2020 20:39, Ilya Kosarev wrote:
> Trying to perform space:truncate() and space:delete() while reaching
> memtx_memory limit we could experience slab allocator failure. Now it
> is solved through quota overuse tech. This commit introduces
> corresponding tests.
>
> Part of #3807
> ---
> src/box/memtx_space.c | 15 +++-
> src/lib/core/errinj.h | 1 +
> test/box/errinj.result | 1 +
> test/engine/engine.cfg | 6 ++
> test/engine/low_memory.lua | 8 ++
> test/engine/stress_delete.result | 111 +++++++++++++++++++++++++++
> test/engine/stress_delete.test.lua | 55 +++++++++++++
> test/engine/stress_truncate.result | 103 +++++++++++++++++++++++++
> test/engine/stress_truncate.test.lua | 52 +++++++++++++
> 9 files changed, 349 insertions(+), 3 deletions(-)
> create mode 100644 test/engine/low_memory.lua
> create mode 100644 test/engine/stress_delete.result
> create mode 100644 test/engine/stress_delete.test.lua
> create mode 100644 test/engine/stress_truncate.result
> create mode 100644 test/engine/stress_truncate.test.lua
>
> diff --git a/test/engine/engine.cfg b/test/engine/engine.cfg
> index f1e7de274..c8c6fb130 100644
> --- a/test/engine/engine.cfg
> +++ b/test/engine/engine.cfg
> @@ -5,6 +5,12 @@
> },
> "func_index.test.lua": {
> "memtx": {"engine": "memtx"}
> + },
> + "stress_delete.test.lua": {
> + "memtx": {"engine": "memtx"}
> + },
> + "stress_truncate.test.lua": {
> + "memtx": {"engine": "memtx"}
> }
1. This folder (engine/) is supposed to have only
tests running on both engines. It is purpose of this
folder. If you need something to run for one engine only,
please, add it to a different folder. For you case the
box/ folder is ok.
> }
>
> diff --git a/test/engine/low_memory.lua b/test/engine/low_memory.lua
> new file mode 100644
> index 000000000..46fd26d1b
> --- /dev/null
> +++ b/test/engine/low_memory.lua
> @@ -0,0 +1,8 @@
> +#!/usr/bin/env tarantool
> +os = require('os')
> +box.cfg({
> + listen = os.getenv("LISTEN"),
> + memtx_memory = 32 * 1024 * 1024,
> +})
> +
> +require('console').listen(os.getenv('ADMIN'))
> diff --git a/test/engine/stress_delete.result b/test/engine/stress_delete.result
> new file mode 100644
> index 000000000..bf92c780b
> --- /dev/null
> +++ b/test/engine/stress_delete.result
2. Since this is a test for a bug, and I would say quite a
big test, it should follow the convention and have the
corresponding name pattern: 'gh-####-short-description.test.lua'.
3. Please, don't split test of that bug into separate files.
You just duplicated lots of the code.
4. The test uses errinj, but is not added to release_disabled.
Please, add.
> @@ -0,0 +1,111 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
5. When a test is related to a ticket (which is nearly always),
you need to write a comment saying ticket number and its
description. See examples in other test files.
> +
> +
> +test_run:cmd("create server master with script='engine/low_memory.lua'")
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server master')
> + | ---
> + | - true
> + | ...
> +test_run:cmd("switch master")
> + | ---
> + | - true
> + | ...
> +
> +
> +test_run:cmd("setopt delimiter ';'")
> + | ---
> + | - true
> + | ...
> +function create_space(name)
> + local space = box.schema.create_space(name)
> + space:format({
> + { name = "id", type = "unsigned" },
> + { name = "val", type = "str" }
> + })
> + space:create_index('primary', { parts = { 'id' } })
> + return space
> +end;
> + | ---
> + | ...
> +
> +function insert(space, i)
> + space:insert({ i, string.rep(string.char(32 + math.random(127-32)), math.random(1024)) })
> +end;
6. Why does it matter whether data is random? IMO, on the
contrary, it reduces reproducibility.
> + | ---
> + | ...
> +
> +function fill_space(space, start)
> + local _, err = nil
> + local i = start
> + while err == nil do _, err = pcall(insert, space, i) i = i + 1 end
> +end;
> + | ---
> + | ...
> +
> +function stress_deletion(i, spaces)
> + local res, space = pcall(create_space, 'test' .. tostring(i))
> + if res then spaces[i] = space return end
> + fill_space(box.space.test, box.space.test:len())
> + for _, s in pairs(spaces) do fill_space(s, s:len()) end
> + box.space.test:delete(box.space.test:len() - 1)
> +end;
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | - true
> + | ...
> +
> +
> +_ = create_space('test')
> + | ---
> + | ...
> +for i = 0, 27000 do insert(box.space.test, i) end
7. Why exactly 27k?
8. Seems like drop of this space 'test' does not change
anything. I dropped it and the test passed.
> + | ---
> + | ...
> +
> +spaces = {}
> + | ---
> + | ...
> +counter = 0
> + | ---
> + | ...
> +status = true
> + | ---
> + | ...
> +res = nil
> + | ---
> + | ...
> +errinj = box.error.injection
> + | ---
> + | ...
> +errinj.set('ERRINJ_RESERVE_EXTENTS_BEFORE_DELETE', true)
9. I dropped that error injection and nothing changed in the
output. What do you need it for?
> + | ---
> + | - ok
> + | ...
> +while counter < 1400 do status, res = pcall(stress_deletion, counter, spaces) counter = counter + 1 end
10. Why 1400?
> + | ---
> + | ...
> +status
> + | ---
> + | - true
> + | ...
> +res
> + | ---
> + | - null
> + | ...
I have more comments, but I will keep them until I understand what is
happening in these tests.
More information about the Tarantool-patches
mailing list