From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 2435E46970E for ; Mon, 13 Jan 2020 15:07:00 +0300 (MSK) Date: Mon, 13 Jan 2020 15:04:45 +0300 From: Igor Munkin Message-ID: <20200113120445.GF31304@tarantool.org> References: <231f259ab03d9a1bbd4183857f1e6cd8c87a78bb.1576485551.git.avtikhon@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <231f259ab03d9a1bbd4183857f1e6cd8c87a78bb.1576485551.git.avtikhon@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 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: tarantool-patches@dev.tarantool.org Sasha, Thanks for the patch! I left several comments below, please consider them. On 16.12.19, Alexander V. Tikhonov wrote: > Added local definitions for the variables, added os.exit() > routine at the tests finish. Changed added information messages > to test:ok() calls. IMHO bullet list is a way more readable for listing the changes. > > 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 | 2 +- > 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, 33 insertions(+), 28 deletions(-) General comments: * Feel free to enhance the existing test names passed to tap.test: | $ grep -rF 'tap.test' | table_chain_bug_LuaJIT_494.test.lua:local test = tap.test("494") | fix_string_find_recording.test.lua:local test = | tap.test("fix-string-find-recording") | gh.test.lua:local test = tap.test("gh") | pairsmm_tarantool_4560.test.lua:local test = tap.test("PAIRSMM-is-set") | fold_bug_LuaJIT_505.test.lua:local test = tap.test("505") | fold_bug_LuaJIT_524.test.lua:local test = tap.test("LuaJIT 524") | unsink_64_kptr.test.lua:local test = tap.test("232") * It's a good idea to add more verbose description, but the new ones still don't make sense to me: | $ grep -rnF 'test:ok' | table_chain_bug_LuaJIT_494.test.lua:177:test:ok(true, "LuaJIT/LuaJIT gh-494 successfully checked commit a7dc9e8d23315217c9fe0029bc8ae12c03306b33") | fold_bug_LuaJIT_505.test.lua:19:test:ok(true, "LuaJIT/LuaJIT gh-505 assert not occured") | unsink_64_kptr.test.lua:43:test:ok(true, "openresty-lua/resty-core gh-232 allowed its address to be inlined") > > diff --git a/test/fix_string_find_recording.test.lua b/test/fix_string_find_recording.test.lua > index d3fc9e1..911e611 100755 > --- a/test/fix_string_find_recording.test.lua > +++ b/test/fix_string_find_recording.test.lua > @@ -1,8 +1,8 @@ > #!/usr/bin/env tarantool > > -tap = require('tap') > +local tap = require('tap') > > -test = tap.test("fix-string-find-recording") > +local test = tap.test("fix-string-find-recording") > test:plan(1) > > local err = [[module 'kit.1.10.3-136' not found: > @@ -76,4 +76,4 @@ until not e > > test:is(count_vm, count_jit) > > -test:check() > +os.exit(test:check() and 0 or 1) > diff --git a/test/fold_bug_LuaJIT_505.test.lua b/test/fold_bug_LuaJIT_505.test.lua > index 2fee069..433efb0 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("505") > 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") > > -test:ok("PASS") > +os.exit(test:check() and 0 or 1) > diff --git a/test/fold_bug_LuaJIT_524.test.lua b/test/fold_bug_LuaJIT_524.test.lua > index b056684..c8cc7b0 100755 > --- a/test/fold_bug_LuaJIT_524.test.lua > +++ b/test/fold_bug_LuaJIT_524.test.lua > @@ -21,4 +21,4 @@ end > > test:is(tonumber(sq), math.fmod(math.pow(42, 8), math.pow(2, 32))) > > -test:check() > +os.exit(test:check() and 0 or 1) > diff --git a/test/gh.test.lua b/test/gh.test.lua > index 00b71a6..2804d35 100755 > --- a/test/gh.test.lua > +++ b/test/gh.test.lua > @@ -1,17 +1,17 @@ > #!/usr/bin/env tarantool > > -- Miscellaneous test for LuaJIT bugs > -tap = require('tap') > +local tap = require('tap') > > -test = tap.test("gh") > +local test = tap.test("gh") > test:plan(2) > -- > -- gh-3196: incorrect string length if Lua hash returns 0 > -- > -h = "\x1F\x93\xE2\x1C\xCA\xDE\x28\x08\x26\x01\xED\x0A\x2F\xE4\x21\x02\x97\x77\xD9\x3E" > +local h = "\x1F\x93\xE2\x1C\xCA\xDE\x28\x08\x26\x01\xED\x0A\x2F\xE4\x21\x02\x97\x77\xD9\x3E" > test:is(h:len(), 20) > > h = "\x0F\x93\xE2\x1C\xCA\xDE\x28\x08\x26\x01\xED\x0A\x2F\xE4\x21\x02\x97\x77\xD9\x3E" > test:is(h:len(), 20) > > -test:check() > +os.exit(test:check() and 0 or 1) > diff --git a/test/pairsmm_tarantool_4560.test.lua b/test/pairsmm_tarantool_4560.test.lua > index bb9835d..f8cf0bb 100755 > --- a/test/pairsmm_tarantool_4560.test.lua > +++ b/test/pairsmm_tarantool_4560.test.lua > @@ -1,8 +1,8 @@ > #!/usr/bin/env tarantool > > -tap = require('tap') > +local tap = require('tap') > > -test = tap.test("PAIRSMM-is-set") > +local test = tap.test("PAIRSMM-is-set") > test:plan(2) > > -- There is no Lua way to detect whether LJ_PAIRSMM is enabled. However, in > @@ -46,5 +46,7 @@ for k, v in ipairs(t) do > ipairs_res = v + ipairs_res > end > test:is(pairs_res + ipairs_res, 0) > -os_exec_res = os.execute() > +local os_exec_res = os.execute() > test:is(os_exec_res, 1) > + > +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..85df8e0 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("494") > 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") > > -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..d01ddc6 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("232") > 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") > > -test:ok("PASS") > +os.exit(test:check() and 0 or 1) > -- > 2.17.1 > -- Best regards, IM