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 154BE2BF95 for ; Wed, 24 Apr 2019 08:01:19 -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 d-k_XbZ6tG8k for ; Wed, 24 Apr 2019 08:01:19 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 9F1DA2BF1F for ; Wed, 24 Apr 2019 08:01:18 -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> <663571b2-87cc-2277-b291-d4b31f3db649@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 24 Apr 2019 15:01:15 +0300 MIME-Version: 1.0 In-Reply-To: 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, Kirill Yukhin LGTM. On 24/04/2019 14:56, Kirill Shcherbatov wrote: > On 24.04.2019 14:23, Vladislav Shpilevoy wrote: >> Hi! Thanks for the patch! See 6 comments below >> and some fixes on the branch. > Hi! Thank you for review. > >> 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. >> 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. >> 3. This line is excessive. Next 'if' will detect that >> type(got) ~= 'table' and will return 'got == expected'. >> 4. 'extra.strict == true' -> 'extra.strict' and it would fit >> one line, but up to you as I said. > Your changes are looking good for me. > >> 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. > Fixed. > >> 6. Great, I've just added one new test that >> {a = box.NULL} equals {a = box.NULL}. > Tnx! > > ======================================================= > > The tap:is_deeply call used to return inconsistent result > processing an empty tuple and tuple that has no values: > is_deeply({a = box.NULL}, {}) == true > is_deeply({}, {a = box.NULL}) == false > Fixed to return true by default in such case. You may also > set tap.strict = true to change this behaviour. > > @TarantoolBot document > Title: tap test new flag 'strict' > In some scenarios it is convenient to distinguish box.NULL and > nil in tap:is(), tap:isnt(), tap:is_deeply() tests. > For example, {result='Success'} and {result='Success', error=None} > are different HTTP responses. You may set t.strict = true to > behave such way. > Example: > t = require('tap').test('123') > t.strict = true > t:is_deeply({a = box.NULL}, {}) -- false > > Closes #4125 > --- > src/lua/tap.lua | 19 +++++--- > test/app-tap/tap.result | 95 ++++++++++++++++++++++++++++++++++++--- > test/app-tap/tap.test.lua | 50 ++++++++++++++++++++- > 3 files changed, 150 insertions(+), 14 deletions(-) > > diff --git a/src/lua/tap.lua b/src/lua/tap.lua > index 546d15392..94b080d5a 100644 > --- a/src/lua/tap.lua > +++ b/src/lua/tap.lua > @@ -91,15 +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 got == nil and expected == nil then return true 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 type(got) ~= 'table' then > + if type(got) ~= 'table' or type(expected) ~= 'table' then > extra.got = got > extra.expected = expected > return got == expected > @@ -117,8 +116,8 @@ 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 or v ~= box.NULL) then > extra.expected = 'key ' .. tostring(i) > extra.got = 'nil' > return false > @@ -148,14 +147,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 > > > @@ -163,6 +166,7 @@ local function is_deeply(test, got, expected, message, extra) > extra = extra or {} > extra.got = got > extra.expected = expected > + extra.strict = test.strict > return ok(test, cmpdeeply(got, expected, extra), message, extra) > end > > @@ -225,6 +229,7 @@ local function test(parent, name, fun, ...) > failed = 0; > planned = 0; > trace = parent == nil and true or parent.trace; > + strict = false; > }, test_mt) > if fun ~= nil then > test:diag('%s', test.name) > diff --git a/test/app-tap/tap.result b/test/app-tap/tap.result > index 3e7882331..12bf86ec2 100644 > --- a/test/app-tap/tap.result > +++ b/test/app-tap/tap.result > @@ -1,5 +1,5 @@ > TAP version 13 > -1..32 > +1..42 > ok - true > ok - extra information is not printed on success > not ok - extra printed using yaml only on failure > @@ -69,6 +69,24 @@ not ok - cdata type > expected: ctype > got: ctype > ... > +not ok - box.NULL == nil strict = true > + --- > + got: null > + ... > +not ok - nil == box.NULL strict = true > + --- > + expected: null > + ... > +ok - box.NULL == box.NULL strict = true > +ok - nil == nil strict = true > +ok - box.NULL != nil strict = true > +ok - nil != box.NULL strict = true > +not ok - box.NULL != box.NULL strict = true > + --- > + unexpected: null > + got: null > + ... > +not ok - nil != nil strict = true > # subtest 1 > 1..2 > ok - true > @@ -119,7 +137,7 @@ not ok - failed subtests > failed: 1 > ... > # is_deeply > - 1..6 > + 1..20 > ok - 1 and 1 > ok - abc and abc > ok - empty tables > @@ -127,20 +145,69 @@ not ok - failed subtests > not ok - {1} and {2} > --- > path: //1 > + strict: false > expected: 2 > got: 1 > ... > not ok - {1,2,{3,4}} and {1,2,{3,5}} > --- > path: //3/2 > + strict: false > expected: 5 > got: 4 > ... > + ok - {} and {a = box.NULL} strict = false > + ok - {a = box.NULL} and {} strict = false > + ok - {a = box.NULL} and {b = box.NULL} strict = false > + ok - {a = box.NULL} and {b = box.NULL, c = box.NULL} strict = false > + ok - nil and box.NULL strict = false > + ok - box.NULL and nil strict = false > + ok - {a = box.NULL} and {a = box.NULL} strict false > + not ok - {} and {a = box.NULL} strict = true > + --- > + strict: true > + expected: key a > + got: nil > + ... > + not ok - {a = box.NULL} and {} strict = true > + --- > + path: //a > + strict: true > + expected: nil > + got: cdata > + ... > + not ok - {a = box.NULL} and {b = box.NULL} strict = true > + --- > + path: //a > + strict: true > + expected: nil > + got: cdata > + ... > + not ok - {a = box.NULL} and {b = box.NULL, c = box.NULL} strict = true > + --- > + path: //a > + strict: true > + expected: nil > + got: cdata > + ... > + not ok - nil and box.NULL strict = true > + --- > + got: nil > + expected: cdata > + strict: true > + ... > + not ok - box.NULL and nil strict = true > + --- > + got: cdata > + expected: nil > + strict: true > + ... > + ok - {a = box.NULL} and {a = box.NULL} strict true > # is_deeply: end > not ok - failed subtests > --- > - planned: 6 > - failed: 2 > + planned: 20 > + failed: 8 > ... > # like > 1..2 > @@ -148,4 +215,22 @@ not ok - failed subtests > ok - unlike(abcde, acd) > # like: end > ok - like > -# failed subtest: 15 > +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 0e1de7f1c..e2a78f630 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(32) -- plan to run 3 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()' } > @@ -60,6 +60,19 @@ test:iscdata(10, 'int', 'cdata type') > test:iscdata(ffi.new('int', 10), 'int', 'cdata type') > test:iscdata(ffi.new('unsigned int', 10), 'int', 'cdata type') > > +-- > +-- gh-4125: Strict nulls comparisons. > +-- > +test.strict = true > +test:is(box.NULL, nil, "box.NULL == nil strict = true") > +test:is(nil, box.NULL, "nil == box.NULL strict = true") > +test:is(box.NULL, box.NULL, "box.NULL == box.NULL strict = true") > +test:is(nil, nil, "nil == nil strict = true") > +test:isnt(box.NULL, nil, "box.NULL != nil strict = true") > +test:isnt(nil, box.NULL, "nil != box.NULL strict = true") > +test:isnt(box.NULL, box.NULL, "box.NULL != box.NULL strict = true") > +test:isnt(nil, nil, "nil != nil strict = true") > +test.strict = false > -- > -- Any test also can create unlimited number of sub tests. > -- Subtest with callbacks (preferred). > @@ -118,7 +131,7 @@ end) > > > test:test('is_deeply', function(t) > - t:plan(6) > + t:plan(20) > > t:is_deeply(1, 1, '1 and 1') > t:is_deeply('abc', 'abc', 'abc and abc') > @@ -127,6 +140,32 @@ 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 = box.NULL} strict = false') > + t:is_deeply({a = box.NULL}, {}, '{a = box.NULL} and {} strict = false') > + t:is_deeply({a = box.NULL}, {b = box.NULL}, > + '{a = box.NULL} and {b = box.NULL} strict = false') > + t:is_deeply({a = box.NULL}, {b = box.NULL, c = box.NULL}, > + '{a = box.NULL} and {b = box.NULL, c = box.NULL} 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 = box.NULL} and {a = box.NULL} strict false') > + > + t.strict = true > + t:is_deeply({}, {a = box.NULL}, '{} and {a = box.NULL} strict = true') > + t:is_deeply({a = box.NULL}, {}, '{a = box.NULL} and {} strict = true') > + t:is_deeply({a = box.NULL}, {b = box.NULL}, > + '{a = box.NULL} and {b = box.NULL} strict = true') > + t:is_deeply({a = box.NULL}, {b = box.NULL, c = box.NULL}, > + '{a = box.NULL} and {b = box.NULL, c = box.NULL} 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 = box.NULL} and {a = box.NULL} strict true') > + t.strict = false > end) > > > @@ -136,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. > -- > 2.21.0 >