[tarantool-patches] Re: [PATCH v2 1/1] tap: fix is_deeply box.NULL corner cases

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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<int>
>    got: ctype<unsigned int>
>    ...
> +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
>