Re: [Tarantool-patches] [PATCH v4 4/4] test: add tests for truncation and deletion

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.