From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id B567B4696C3 for ; Sat, 2 May 2020 23:08:59 +0300 (MSK) References: <5dc5d2b0aaad4fcfc157c5a6e6bd13025ba85a25.1588292014.git.v.shpilevoy@tarantool.org> <6c622f6c-19d9-a64b-5ccb-8799561ef7cf@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sat, 2 May 2020 22:08:57 +0200 MIME-Version: 1.0 In-Reply-To: <6c622f6c-19d9-a64b-5ccb-8799561ef7cf@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH vshard 1/7] test: print errors in a portable way List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Oleg Babin , tarantool-patches@dev.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)