[tarantool-patches] Re: [PATCH v2 2/2] Fixed lost format on update and upsert operations.

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

Hello. See below 6 comments.

1. I fixed some of my comments, and pushed them with --force.
You can see them using diff your local and remote changes. After
this do pull. Please, try to investigate, why your text editor
uses spaces for multiline statements alignment, and fix it.

2. I do not see a test on upsert - please, add.

3. Please, add a test on incorrect update.

4. In the previous letter I have asked you to move the test into
engine/ suite. You can not get different engines on box/ suite:

[001] Test failed! Result content mismatch:
[001] --- box/update.result	Sun Apr 15 15:55:23 2018
[001] +++ box/update.reject	Sun Apr 15 16:07:40 2018
[001] @@ -861,6 +861,10 @@
[001]  engine = inspector:get_cfg('engine')
[001]  ---
[001]  ...
[001] +engine
[001] +---
[001] +- null
[001] +...
[001]  format = {}
[001]  ---
[001]  ...
[001]


As you can see, I printed engine variable in you test and 1) it is null,
2) the test is not run 2 times - with engine = memtx and engine = vinyl.
For this you must use engine/ suite, not box/.



> +test_run = require('test_run')
> +inspector = test_run.new()

5. It is not needed to create a new test run each time when you need to
get a variable from environment. Here the test run already is created.

> +engine = inspector:get_cfg('engine')
>   format = {}


> -s = box.schema.space.create('tst_sample', {engine = 'vinyl', format = format} > -pk = s:create_index('pk')
> -s:insert({1, 'sss'})
> -aa = box.space.tst_sample:get(1)
> -aa.VAL
> -aa = aa:update({{'=',2,'ssss'}})
> -collectgarbage()
> -aa.VAL
> -s:drop()
> -aa = nil
> -collectgarbage()
>   
>   -- test transform integrity
>   space = box.schema.space.create('tweedledum')

6. Try to do not create a new space to test each tuple. You can create
one space, and test on it transform, correct update, correct upsert,
incorrect update, incorrect upsert.

Too many actions for such simple test is not good, because test count
grows every day, and running all of them is already too long.