From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 3AFA0469719 for ; Fri, 28 Feb 2020 21:42:10 +0300 (MSK) Date: Fri, 28 Feb 2020 21:36:49 +0300 From: Igor Munkin Message-ID: <20200228183648.GR404@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 1/2] test: cleanup tests code List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Alexander V. Tikhonov" Cc: Oleg Piskunov , tarantool-patches@dev.tarantool.org Sasha, Thanks for the patch. I left some comments below, please consider them. On 27.02.20, Alexander V. Tikhonov wrote: > Cleaned up the tests code: > - added local definitions for the variables > - added os.exit() routine at the tests finish > - changed added information messages to test:ok() calls I propose to reword the commit message the following way: | test: cleanup tests code | | Cleaned up the tests code according to the Lua style guide: | - made scoped variables local | - added os.exit call at the end of the test chunk | - adjusted the messages in test:ok calls General comment: please adjust the test names regarding the changes I proposed in the second patch. > > Part of #4655 > --- > test/fix_string_find_recording.test.lua | 6 +++--- > test/fold_bug_LuaJIT_505.test.lua | 7 ++++--- > test/fold_bug_LuaJIT_524.test.lua | 4 ++-- > test/gh.test.lua | 8 ++++---- > test/pairsmm_tarantool_4560.test.lua | 8 +++++--- > test/table_chain_bug_LuaJIT_494.test.lua | 23 ++++++++++++----------- > test/unsink_64_kptr.test.lua | 7 ++++--- > 7 files changed, 34 insertions(+), 29 deletions(-) > > diff --git a/test/fold_bug_LuaJIT_505.test.lua b/test/fold_bug_LuaJIT_505.test.lua > index 2fee069..3b7d7df 100755 > --- a/test/fold_bug_LuaJIT_505.test.lua > +++ b/test/fold_bug_LuaJIT_505.test.lua > @@ -1,8 +1,8 @@ > #!/usr/bin/env tarantool > > -tap = require('tap') > +local tap = require('tap') > > -test = tap.test("505") > +local test = tap.test("lj-505-fold-icorrect-behavior") > test:plan(1) > > -- Test file to demonstrate Lua fold machinery icorrect behavior, details: > @@ -16,5 +16,6 @@ for _ = 1, 20 do > local pos_b = string.find(value2, "b", 2, true) > assert(pos_b == 2, "FAIL: position of 'b' is " .. pos_b) > end > +test:ok(true, "LuaJIT/LuaJIT gh-505 assert not occured") Message is absolutely non-informative. I propose to reword it: | "string.find offset aritmetics wasn't broken while recording" > > -test:ok("PASS") > +os.exit(test:check() and 0 or 1) > diff --git a/test/table_chain_bug_LuaJIT_494.test.lua b/test/table_chain_bug_LuaJIT_494.test.lua > index 06c0f0d..3007360 100755 > --- a/test/table_chain_bug_LuaJIT_494.test.lua > +++ b/test/table_chain_bug_LuaJIT_494.test.lua > @@ -1,8 +1,8 @@ > #!/usr/bin/env tarantool > > -tap = require('tap') > +local tap = require('tap') > > -test = tap.test("494") > +local test = tap.test("lj-494-table-chain-infinite-loop") > test:plan(1) > > -- Test file to demonstrate Lua table hash chain bugs discussed in > @@ -11,19 +11,19 @@ test:plan(1) > -- https://gist.github.com/corsix/1fc9b13a2dd5f3659417b62dd54d4500 > > --- Plumbing > -ffi = require"ffi" > -ffi.cdef"char* strstr(const char*, const char*)" > -strstr = ffi.C.strstr > -cast = ffi.cast > -str_hash_offset = cast("uint32_t*", strstr("*", ""))[-2] == 1 and 3 or 2 > +local ffi = require("ffi") > +ffi.cdef("char* strstr(const char*, const char*)") > +local strstr = ffi.C.strstr > +local cast = ffi.cast > +local str_hash_offset = cast("uint32_t*", strstr("*", ""))[-2] == 1 and 3 or 2 > function str_hash(s) > return cast("uint32_t*", strstr(s, "")) - str_hash_offset > end > -table_new = require"table.new" > +local table_new = require("table.new") > > --- Prepare some objects > -victims = {} > -orig_hash = {} > +local victims = {} > +local orig_hash = {} > for c in ("abcdef"):gmatch"." do > v = c .. "{09add58a-13a4-44e0-a52c-d44d0f9b2b95}" > victims[c] = v > @@ -174,5 +174,6 @@ collectgarbage() > for c, v in pairs(victims) do > str_hash(v)[0] = orig_hash[c] > end > +test:ok(true, "LuaJIT/LuaJIT gh-494 successfully checked commit a7dc9e8d23315217c9fe0029bc8ae12c03306b33") Message is absolutely non-informative. I propose to reword it: | "table keys collisions are resolved properly (no assertions failed)" > > -test:ok("PASS") > +os.exit(test:check() and 0 or 1) > diff --git a/test/unsink_64_kptr.test.lua b/test/unsink_64_kptr.test.lua > index 8995763..a618fd2 100755 > --- a/test/unsink_64_kptr.test.lua > +++ b/test/unsink_64_kptr.test.lua > @@ -1,8 +1,8 @@ > #!/usr/bin/env tarantool > > -tap = require('tap') > +local tap = require('tap') > > -test = tap.test("232") > +local test = tap.test("or-232-unsink-64-kptr") > test:plan(1) > > --- From: Thibault Charbonnier > @@ -40,5 +40,6 @@ end > for i = 1, 1000 do > fn(i) > end > +test:ok(true, "openresty-lua/resty-core gh-232 allowed its address to be inlined") Here is a mess with a message, it totally doesn't describe the check being made. I propose the following comment: | "allocation is unsunk at the trace exit (no platform failures)" > > -test:ok("PASS") > +os.exit(test:check() and 0 or 1) > -- > 2.17.1 > -- Best regards, IM