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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

I've fixed tests and wrong code style with tabs. 

From f9803f8d0b411abac02e35d5e4120ff0ce7e9341 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Mon, 16 Apr 2018 10:44:10 +0300
Subject: [PATCH] Fixed tests and spaces

---
 src/box/lua/tuple.c         | 10 +++---
 test/box/update.result      | 84 ---------------------------------------------
 test/box/update.test.lua    | 31 -----------------
 test/engine/update.result   | 61 ++++++++++++++++++++++++++++++++
 test/engine/update.test.lua | 24 +++++++++++++
 5 files changed, 90 insertions(+), 120 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index c40bdb7..566cbec 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -397,13 +397,13 @@ lbox_tuple_transform(struct lua_State *L)
 	size_t used = region_used(region);
 	struct tuple *new_tuple = NULL;
 	const char *new_data = tuple_update_execute(region_aligned_alloc_cb,
-	                                            region, buf->buf,
-	                                            buf->buf + ibuf_used(buf),
-	                                            old_data, old_data + bsize,
-	                                            &new_size, 1, NULL);
+						    region, buf->buf,
+						    buf->buf + ibuf_used(buf),
+						    old_data, old_data + bsize,
+						    &new_size, 1, NULL);
 	if (new_data != NULL)
 		new_tuple = tuple_new(box_tuple_format_default(),
-		                      new_data, new_data + new_size);
+				      new_data, new_data + new_size);
 	region_truncate(region, used);
 
 	if (new_tuple == NULL)
diff --git a/test/box/update.result b/test/box/update.result
index c71cbd8..a3f731b 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -849,87 +849,3 @@ s:update(1, {{'=', 3, map}})
 s:drop()
 ---
 ...
---
--- gh-3051 Lost format while tuple update
---
-test_run = require('test_run')
----
-...
-inspector = test_run.new()
----
-...
-engine = inspector:get_cfg('engine')
----
-...
-format = {}
----
-...
-format[1] = {name = 'KEY', type = 'unsigned'}
----
-...
-format[2] = {name = 'VAL', type = 'string'}
----
-...
-s = box.schema.space.create('tst_sample', {engine = engine, format = format})
----
-...
-pk = s:create_index('pk')
----
-...
-s:insert({1, 'sss'})
----
-- [1, 'sss']
-...
-aa = box.space.tst_sample:get(1)
----
-...
-aa.VAL
----
-- sss
-...
-aa = aa:update({{'=',2,'ssss'}})
----
-...
-aa.VAL
----
-- ssss
-...
-s:drop()
----
-...
-aa = nil
----
-...
--- test transform integrity
-space = box.schema.space.create('tweedledum')
----
-...
-tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
----
-...
-t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
----
-...
-t:transform(-1, 1)
----
-- ['1', '2', '3', '4', '5', '6']
-...
-n = 2000
----
-...
-tab = {}; for i=1,n,1 do table.insert(tab, i) end
----
-...
-t = box.tuple.new(tab)
----
-...
-t:transform(1, n - 1)
----
-- [2000]
-...
-t = nil
----
-...
-space:drop()
----
-...
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index b6271a6..c455bc1 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -269,34 +269,3 @@ t:update({{'=', 3, map}})
 s:update(1, {{'=', 3, map}})
 
 s:drop()
-
---
--- gh-3051 Lost format while tuple update
---
-test_run = require('test_run')
-inspector = test_run.new()
-engine = inspector:get_cfg('engine')
-format = {}
-format[1] = {name = 'KEY', type = 'unsigned'}
-format[2] = {name = 'VAL', type = 'string'}
-s = box.schema.space.create('tst_sample', {engine = engine, 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'}})
-aa.VAL
-s:drop()
-aa = nil
-
--- test transform integrity
-space = box.schema.space.create('tweedledum')
-tmp = space:create_index('primary', { type = 'hash', parts = {1, 'string'}, unique = true })
-t = space:insert{'1', '2', '3', '4', '5', '6', '7'}
-t:transform(-1, 1)
-n = 2000
-tab = {}; for i=1,n,1 do table.insert(tab, i) end
-t = box.tuple.new(tab)
-t:transform(1, n - 1)
-t = nil
-space:drop()
diff --git a/test/engine/update.result b/test/engine/update.result
index 653ebec..48398d2 100644
--- a/test/engine/update.result
+++ b/test/engine/update.result
@@ -683,3 +683,64 @@ space:select{}
 space:drop()
 ---
 ...
+--
+-- gh-3051 Lost format while tuple update
+--
+format = {}
+---
+...
+format[1] = {name = 'KEY', type = 'unsigned'}
+---
+...
+format[2] = {name = 'VAL', type = 'string'}
+---
+...
+s = box.schema.space.create('tst_sample', {engine = engine, format = format})
+---
+...
+pk = s:create_index('pk')
+---
+...
+s:insert({1, 'sss', '3', '4', '5', '6', '7'})
+---
+- [1, 'sss', '3', '4', '5', '6', '7']
+...
+aa = box.space.tst_sample:get(1)
+---
+...
+aa.VAL
+---
+- sss
+...
+aa = aa:update({{'=',2,'ssss'}})
+---
+...
+aa.VAL
+---
+- ssss
+...
+-- test transform integrity
+aa:transform(-1, 1)
+---
+- [1, 'ssss', '3', '4', '5', '6']
+...
+aa = nil
+---
+...
+s:upsert({2, 'wwwww'}, {{'=', 2, 'wwwww'}})
+---
+...
+box.space.tst_sample:get(2).VAL
+---
+- wwwww
+...
+s:upsert({2, 'wwwww2'}, {{'=', 2, 'wwwww2'}})
+---
+...
+box.space.tst_sample:get(2).VAL
+---
+- wwwww2
+...
+s:drop()
+---
+...
diff --git a/test/engine/update.test.lua b/test/engine/update.test.lua
index 0252b8a..c53e458 100644
--- a/test/engine/update.test.lua
+++ b/test/engine/update.test.lua
@@ -94,3 +94,27 @@ space:select{}
 space:update({2}, {})
 space:select{}
 space:drop()
+
+--
+-- gh-3051 Lost format while tuple update
+--
+format = {}
+format[1] = {name = 'KEY', type = 'unsigned'}
+format[2] = {name = 'VAL', type = 'string'}
+s = box.schema.space.create('tst_sample', {engine = engine, format = format})
+pk = s:create_index('pk')
+s:insert({1, 'sss', '3', '4', '5', '6', '7'})
+aa = box.space.tst_sample:get(1)
+aa.VAL
+aa = aa:update({{'=',2,'ssss'}})
+aa.VAL
+-- test transform integrity
+aa:transform(-1, 1)
+aa = nil
+
+s:upsert({2, 'wwwww'}, {{'=', 2, 'wwwww'}})
+box.space.tst_sample:get(2).VAL
+s:upsert({2, 'wwwww2'}, {{'=', 2, 'wwwww2'}})
+box.space.tst_sample:get(2).VAL
+
+s:drop()
-- 
2.7.4



On 15.04.2018 16:18, Vladislav Shpilevoy wrote:
> 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.
>