Re: [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way

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

Hi! Thanks for the review!

> On 01/05/2020 03:16, Vladislav Shpilevoy wrote:
>>   +-- Portable representation of an error, not depending on Tarantool
>> +-- version and on any additional fields it can add. Trace is also
>> +-- trimmed in order for the tests not to depend on line numbers of
>> +-- the source files, which may slip into a .result file.
>> +local function portable_error(err)
>> +    return {code = err.code, type = err.type, message = err.message}
>> +end
> 
> I propose to add "nil" check here. At first it allows to use this hepler not only when we expect error. Secondly this approach eliminates confusing error messages if tests break for some reasons.

But this function is supposed to be used only on errors. So
it looks strange to put here any workarounds for non-errors.

>> diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
>> index 6c15707..2ba7da6 100644
>> --- a/test/unit/error.test.lua
>> +++ b/test/unit/error.test.lua
>> @@ -8,7 +8,11 @@ lerror = vshard.error
>>   --
>>   ok, err = pcall(box.error, box.error.TIMEOUT)
>>   box_error = lerror.box(err)
> 
> Why do you use pcall(box.error, box.error.TIMEOUT)? Is it needed for promote an error to box.error.last()? Why not simply `box.error.new(box.error.TIMEOUT)?` I doesn't expect that it will promote an error to box.error.last() (even if https://github.com/tarantool/tarantool/issues/4778 still works in 1.10/2.2/2.3), but I think that this isn't really needed if you return `nil, err`.

The lines you are referring to were written for 1.9 version.
box.error.new() does not exist in 1.9. Since then there was
no reason to change these particular test code lines.

>> -tostring(box_error)
>> +str = tostring(box_error)
>> +-- Base_type appears in 2.4.1. Simple nullification of base_type
>> +-- won't work, since order of the old fields becomes different.
>> +assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
>> +       str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
>>
> 
> I assume that we don't have a guarantee that our error will be printed as `{"type":"ClientError","code":78 ... }` and not as `{"code":78, "type":"ClientError" ...}`.

Actually we have. If you insert the same keys in the same order,
resulting iteration order will be the same everywhere (at least
this is what I always observed). Unless luajit will change the
way how it places values into tables, or if a new key is added.
The latter is exactly what happened because of 'base_type'
in 2.4.1.

> Also there is confusing "line" parameter. Is it stable for different versions? I propose to check it as
> ```
> err.type == "ClientError"
> err.code == 78
> ...
> ```

The 'line' is stable. It is UINT32_MAX when unknown.

The test is exactly about string representation. That it is json.
Removing check of how the string looks would make the test useless.

However I found another workaround:

====================
diff --git a/test/unit/error.result b/test/unit/error.result
index 144df44..8552d91 100644
--- a/test/unit/error.result
+++ b/test/unit/error.result
@@ -7,6 +7,9 @@ vshard = require('vshard')
 util = require('util')
 ---
 ...
+json = require('json')
+---
+...
 lerror = vshard.error
 ---
 ...
@@ -22,12 +25,11 @@ box_error = lerror.box(err)
 str = tostring(box_error)
 ---
 ...
--- Base_type appears in 2.4.1. Simple nullification of base_type
--- won't work, since order of the old fields becomes different.
-assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
-       str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
+util.portable_error(json.decode(str))
 ---
-- true
+- type: ClientError
+  code: 78
+  message: Timeout exceeded
 ...
 vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
 ---
diff --git a/test/unit/error.test.lua b/test/unit/error.test.lua
index 2ba7da6..859414e 100644
--- a/test/unit/error.test.lua
+++ b/test/unit/error.test.lua
@@ -1,6 +1,7 @@
 test_run = require('test_run').new()
 vshard = require('vshard')
 util = require('util')
+json = require('json')
 lerror = vshard.error
 
 --
@@ -9,10 +10,7 @@ lerror = vshard.error
 ok, err = pcall(box.error, box.error.TIMEOUT)
 box_error = lerror.box(err)
 str = tostring(box_error)
--- Base_type appears in 2.4.1. Simple nullification of base_type
--- won't work, since order of the old fields becomes different.
-assert(str == '{"type":"ClientError","code":78,"message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}' or \
-       str == '{"code":78,"base_type":"ClientError","type":"ClientError","message":"Timeout exceeded","trace":[{"file":"[C]","line":4294967295}]}')
+util.portable_error(json.decode(str))
 
 vshard_error = lerror.vshard(lerror.code.UNREACHABLE_MASTER, 'uuid', 'reason')
 tostring(vshard_error)