From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0FC782BD54 for ; Wed, 24 Apr 2019 07:23:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id L3me_zrffeo5 for ; Wed, 24 Apr 2019 07:23:58 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 9A5A02BCC5 for ; Wed, 24 Apr 2019 07:23:58 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] tap: fix is_deeply box.NULL corner cases References: <140e8187-8f84-c89d-10d8-57891e8f9353@tarantool.org> From: Vladislav Shpilevoy Message-ID: <663571b2-87cc-2277-b291-d4b31f3db649@tarantool.org> Date: Wed, 24 Apr 2019 14:23:56 +0300 MIME-Version: 1.0 In-Reply-To: <140e8187-8f84-c89d-10d8-57891e8f9353@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Kirill Shcherbatov , tarantool-patches@freelists.org Hi! Thanks for the patch! See 6 comments below and some fixes on the branch. > diff --git a/src/lua/tap.lua b/src/lua/tap.lua > index 546d15392..866786e6f 100644 > --- a/src/lua/tap.lua > +++ b/src/lua/tap.lua > @@ -91,13 +91,18 @@ local function cmpdeeply(got, expected, extra) > > if ffi.istype('bool', got) then got = (got == 1) end > if ffi.istype('bool', expected) then expected = (expected == 1) end > - if got == nil and expected == nil then return true end > + if extra.strict == false then 1. I would rather use 'not extra.strict' instead of explicit comparison with true/false here and in other places, but up to you, we do not have such strict rules for Lua. > + -- Convert all box.NULLs to LUA nil representation. > + if got == box.NULL then got = nil end > + if expected == box.NULL then expected = nil end 2. These conversions would not be needed, if you just added 'and extra.strict' to the check below. Just like you did for 'is' and 'isnt' - compare types only when strict is set. > + end > > if type(got) ~= type(expected) then > extra.got = type(got) > extra.expected = type(expected) > return false > end > + if got == nil and expected == nil then return true end 3. This line is excessive. Next 'if' will detect that type(got) ~= 'table' and will return 'got == expected'. > > if type(got) ~= 'table' then > extra.got = got > @@ -117,8 +122,9 @@ local function cmpdeeply(got, expected, extra) > end > > -- check if expected contains more keys then got > - for i, _ in pairs(expected) do > - if visited_keys[i] ~= true then > + for i, v in pairs(expected) do > + if visited_keys[i] ~= true and > + (extra.strict == true or v ~= box.NULL) then 4. 'extra.strict == true' -> 'extra.strict' and it would fit one line, but up to you as I said. > extra.expected = 'key ' .. tostring(i) > extra.got = 'nil' > return false > @@ -148,14 +154,18 @@ local function is(test, got, expected, message, extra) > extra = extra or {} > extra.got = got > extra.expected = expected > - return ok(test, got == expected, message, extra) > + local rc = (test.strict == false or type(got) == type(expected)) and > + got == expected > + return ok(test, rc, message, extra) > end > > local function isnt(test, got, unexpected, message, extra) > extra = extra or {} > extra.got = got > extra.unexpected = unexpected > - return ok(test, got ~= unexpected, message, extra) > + local rc = (test.strict == true and type(got) ~= type(unexpected)) or > + got ~= unexpected > + return ok(test, rc, message, extra) > end > diff --git a/test/app-tap/tap.result b/test/app-tap/tap.result > index 3e7882331..13fa4fbcd 100644 > --- a/test/app-tap/tap.result > +++ b/test/app-tap/tap.result > @@ -69,6 +69,24 @@ not ok - cdata type > expected: ctype > got: ctype > ... > +not ok - box.NULL != nil > + --- > + got: null 5. The result confuses. Looks like you checked box.NULL != nil with strict = true and got 'not ok' on that -> box.NULL == nil in strict mode. Lets better for all 'test:is' use '==' in comments instead of '!='. And vice versa for 'test:isnt'. In such a case you will see the output not ok - box.NULL == nil Then it will be obvious that you tried to compare box.NULL and nil, and failed at it. > + ... > +not ok - nil != box.NULL > + --- > + expected: null > + ... > +ok - box.NULL == box.NULL > +ok - nil == nil > +ok - box.NULL != nil > +ok - nil != box.NULL > +not ok - box.NULL == box.NULL > + --- > + unexpected: null > + got: null > + ... > +not ok - nil == nil > # subtest 1 > 1..2 > ok - true > diff --git a/test/app-tap/tap.test.lua b/test/app-tap/tap.test.lua > index 0e1de7f1c..8a70e1c5d 100755 > --- a/test/app-tap/tap.test.lua > +++ b/test/app-tap/tap.test.lua > @@ -127,6 +140,28 @@ test:test('is_deeply', function(t) > t:is_deeply({1}, {2}, '{1} and {2}') > t:is_deeply({1, 2, { 3, 4 }}, {1, 2, { 3, 5 }}, '{1,2,{3,4}} and {1,2,{3,5}}') > > + -- > + -- gh-4125: is_deeply inconsistently works with box.NULL. > + -- > + t:is_deeply({}, {a = box.NULL}, '{} and {a = nil} strict = false') > + t:is_deeply({a = box.NULL}, {}, '{a = nil} and {} strict = false') > + t:is_deeply({a = box.NULL}, {b = box.NULL}, > + '{a = nil} and {b = nil} strict = false') > + t:is_deeply({a = box.NULL}, {b = box.NULL, c = box.NULL}, > + '{a = nil} and {b = nil, c = nil} strict = false') > + t:is_deeply(nil, box.NULL, 'nil and box.NULL strict = false') > + t:is_deeply(box.NULL, nil, 'box.NULL and nil strict = false') > + > + t.strict = true > + t:is_deeply({}, {a = box.NULL}, '{} and {a = nil} strict = true') > + t:is_deeply({a = box.NULL}, {}, '{a = nil} and {} strict = true') > + t:is_deeply({a = box.NULL}, {b = box.NULL}, > + '{a = nil} and {b = nil} strict = true') > + t:is_deeply({a = box.NULL}, {b = box.NULL, c = box.NULL}, > + '{a = nil} and {b = nil, c = nil} strict = true') > + t:is_deeply(nil, box.NULL, 'nil and box.NULL strict = true') > + t:is_deeply(box.NULL, nil, 'box.NULL and nil strict = true') > + t.strict = false 6. Great, I've just added one new test that {a = box.NULL} equals {a = box.NULL}. > end) > > > -- > 2.21.0 > Please, find my review fixes pasted below and on the branch: ========================================================================= diff --git a/src/lua/tap.lua b/src/lua/tap.lua index 866786e6f..94b080d5a 100644 --- a/src/lua/tap.lua +++ b/src/lua/tap.lua @@ -91,20 +91,14 @@ local function cmpdeeply(got, expected, extra) if ffi.istype('bool', got) then got = (got == 1) end if ffi.istype('bool', expected) then expected = (expected == 1) end - if extra.strict == false then - -- Convert all box.NULLs to LUA nil representation. - if got == box.NULL then got = nil end - if expected == box.NULL then expected = nil end - end - if type(got) ~= type(expected) then + if extra.strict and type(got) ~= type(expected) then extra.got = type(got) extra.expected = type(expected) return false end - if got == nil and expected == nil then return true end - if type(got) ~= 'table' then + if type(got) ~= 'table' or type(expected) ~= 'table' then extra.got = got extra.expected = expected return got == expected @@ -123,8 +117,7 @@ local function cmpdeeply(got, expected, extra) -- check if expected contains more keys then got for i, v in pairs(expected) do - if visited_keys[i] ~= true and - (extra.strict == true or v ~= box.NULL) then + if visited_keys[i] ~= true and (extra.strict or v ~= box.NULL) then extra.expected = 'key ' .. tostring(i) extra.got = 'nil' return false diff --git a/test/app-tap/tap.result b/test/app-tap/tap.result index 13fa4fbcd..db56fd720 100644 --- a/test/app-tap/tap.result +++ b/test/app-tap/tap.result @@ -1,5 +1,5 @@ TAP version 13 -1..40 +1..42 ok - true ok - extra information is not printed on success not ok - extra printed using yaml only on failure @@ -137,7 +137,7 @@ not ok - failed subtests failed: 1 ... # is_deeply - 1..18 + 1..20 ok - 1 and 1 ok - abc and abc ok - empty tables @@ -162,6 +162,7 @@ not ok - failed subtests ok - {a = nil} and {b = nil, c = nil} strict = false ok - nil and box.NULL strict = false ok - box.NULL and nil strict = false + ok - {a = nil} and {a = nil} strict false not ok - {} and {a = nil} strict = true --- strict: true @@ -201,10 +202,11 @@ not ok - failed subtests expected: nil strict: true ... + ok - {a = nil} and {a = nil} strict true # is_deeply: end not ok - failed subtests --- - planned: 18 + planned: 20 failed: 8 ... # like @@ -213,4 +215,22 @@ not ok - failed subtests ok - unlike(abcde, acd) # like: end ok - like -# failed subtest: 19 +not ok - compare {1, 2, 3} and '200' + --- + strict: false + expected: '200' + got: + - 1 + - 2 + - 3 + ... +not ok - compare '200' and {1, 2, 3} + --- + strict: false + expected: + - 1 + - 2 + - 3 + got: '200' + ... +# failed subtest: 21 diff --git a/test/app-tap/tap.test.lua b/test/app-tap/tap.test.lua index 8a70e1c5d..7c8948f4d 100755 --- a/test/app-tap/tap.test.lua +++ b/test/app-tap/tap.test.lua @@ -20,7 +20,7 @@ test.trace = false -- ok, fail and skip predicates -- -test:plan(40) -- plan to run 40 test +test:plan(42) test:ok(true, 'true') -- basic function local extra = { state = 'some userful information to debug on failure', details = 'a table argument formatted using yaml.encode()' } @@ -131,7 +131,7 @@ end) test:test('is_deeply', function(t) - t:plan(18) + t:plan(20) t:is_deeply(1, 1, '1 and 1') t:is_deeply('abc', 'abc', 'abc and abc') @@ -151,6 +151,8 @@ test:test('is_deeply', function(t) '{a = nil} and {b = nil, c = nil} strict = false') t:is_deeply(nil, box.NULL, 'nil and box.NULL strict = false') t:is_deeply(box.NULL, nil, 'box.NULL and nil strict = false') + t:is_deeply({a = box.NULL}, {a = box.NULL}, + '{a = nil} and {a = nil} strict false') t.strict = true t:is_deeply({}, {a = box.NULL}, '{} and {a = nil} strict = true') @@ -161,6 +163,8 @@ test:test('is_deeply', function(t) '{a = nil} and {b = nil, c = nil} strict = true') t:is_deeply(nil, box.NULL, 'nil and box.NULL strict = true') t:is_deeply(box.NULL, nil, 'box.NULL and nil strict = true') + t:is_deeply({a = box.NULL}, {a = box.NULL}, + '{a = nil} and {a = nil} strict true') t.strict = false end) @@ -171,6 +175,13 @@ test:test('like', function(t) t:unlike('abcde', 'acd', 'unlike(abcde, acd)') end) +-- +-- Test, that in case of not strict comparison the order of +-- arguments does not matter. +-- +test:is_deeply({1, 2, 3}, '200', "compare {1, 2, 3} and '200'") +test:is_deeply('200', {1, 2, 3}, "compare '200' and {1, 2, 3}") + -- -- Finish root test. Since we used non-callback variant, we have to -- call check explicitly.