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

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

LGTM.

On 23/04/2019 17:12, Kirill Shcherbatov wrote:
> explicit is better than implicit, yep
> 
> - if visited_keys[i] ~= true and v ~= nil then
> + if visited_keys[i] ~= true and v ~= box.NULL then
> 
> ===========================================
> 
> 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 in such case.
> 
> We shouldn't return false in such case because of marshaling
> features covered by tests:
> is_deeply(decode(encode(t)), t) must be true for all t,
> including t = nil; decode(encode(nil)) == box.NULL.
> 
> Closes #4125
> ---
>  src/lua/tap.lua           |  4 ++--
>  test/app-tap/tap.result   |  8 ++++++--
>  test/app-tap/tap.test.lua | 10 +++++++++-
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/src/lua/tap.lua b/src/lua/tap.lua
> index 546d15392..e48adbbab 100644
> --- a/src/lua/tap.lua
> +++ b/src/lua/tap.lua
> @@ -117,8 +117,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 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 3e7882331..dced9a5a2 100644
> --- a/test/app-tap/tap.result
> +++ b/test/app-tap/tap.result
> @@ -119,7 +119,7 @@ not ok - failed subtests
>    failed: 1
>    ...
>      # is_deeply
> -    1..6
> +    1..10
>      ok - 1 and 1
>      ok - abc and abc
>      ok - empty tables
> @@ -136,10 +136,14 @@ not ok - failed subtests
>        expected: 5
>        got: 4
>        ...
> +    ok - {} and {a = nil}
> +    ok - {a = nil} and {}
> +    ok - {a = nil} and {b = nil}
> +    ok - {a = nil} and {b = nil, c = nil}
>      # is_deeply: end
>  not ok - failed subtests
>    ---
> -  planned: 6
> +  planned: 10
>    failed: 2
>    ...
>      # like
> diff --git a/test/app-tap/tap.test.lua b/test/app-tap/tap.test.lua
> index 0e1de7f1c..c0593db46 100755
> --- a/test/app-tap/tap.test.lua
> +++ b/test/app-tap/tap.test.lua
> @@ -118,7 +118,7 @@ end)
>  
>  
>  test:test('is_deeply', function(t)
> -    t:plan(6)
> +    t:plan(10)
>  
>      t:is_deeply(1, 1, '1 and 1')
>      t:is_deeply('abc', 'abc', 'abc and abc')
> @@ -127,6 +127,14 @@ 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}')
> +    t:is_deeply({a = box.NULL}, {}, '{a = nil} and {}')
> +    t:is_deeply({a = box.NULL}, {b = box.NULL}, '{a = nil} and {b = nil}')
> +    t:is_deeply({a = box.NULL}, {b = box.NULL, c = box.NULL},
> +                '{a = nil} and {b = nil, c = nil}')
>  end)
>  
>  
> -- 
> 2.21.0
>