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

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

[Kirill Shcherbatov]

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
---
Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4125-inconsistent-isdeeply-results
Issue: https://github.com/tarantool/tarantool/issues/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..4f214ba52 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 ~= nil 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

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

[Kirill Shcherbatov]

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

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

[Vladislav Shpilevoy]

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
> 

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

[Kirill Shcherbatov]

After a discussion that took place on github after I submitted this patch, it was decided to introduce
the ability to change the behavior when comparing box.NULL and nil in is, isnt, is_deeply tests using
tap object flag  'strict'.

=======================================================================

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
---
Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4125-inconsistent-isdeeply-results
Issue: https://github.com/tarantool/tarantool/issues/4125

 src/lua/tap.lua           | 22 +++++++++---
 test/app-tap/tap.result   | 75 ++++++++++++++++++++++++++++++++++++---
 test/app-tap/tap.test.lua | 39 ++++++++++++++++++--
 3 files changed, 124 insertions(+), 12 deletions(-)

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
+        -- 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
         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
         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
             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
 
 
@@ -163,6 +173,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 +236,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..13fa4fbcd 100644
--- a/test/app-tap/tap.result
+++ b/test/app-tap/tap.result
@@ -1,5 +1,5 @@
 TAP version 13
-1..32
+1..40
 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
+  ---
+  got: null
+  ...
+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
@@ -119,7 +137,7 @@ not ok - failed subtests
   failed: 1
   ...
     # is_deeply
-    1..6
+    1..18
     ok - 1 and 1
     ok - abc and abc
     ok - empty tables
@@ -127,20 +145,67 @@ 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 = nil} strict = false
+    ok - {a = nil} and {} strict = false
+    ok - {a = nil} and {b = nil} strict = false
+    ok - {a = nil} and {b = nil, c = nil} strict = false
+    ok - nil and box.NULL strict = false
+    ok - box.NULL and nil strict = false
+    not ok - {} and {a = nil} strict = true
+      ---
+      strict: true
+      expected: key a
+      got: nil
+      ...
+    not ok - {a = nil} and {} strict = true
+      ---
+      path: //a
+      strict: true
+      expected: nil
+      got: cdata
+      ...
+    not ok - {a = nil} and {b = nil} strict = true
+      ---
+      path: //a
+      strict: true
+      expected: nil
+      got: cdata
+      ...
+    not ok - {a = nil} and {b = nil, c = nil} 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
+      ...
     # is_deeply: end
 not ok - failed subtests
   ---
-  planned: 6
-  failed: 2
+  planned: 18
+  failed: 8
   ...
     # like
     1..2
@@ -148,4 +213,4 @@ not ok - failed subtests
     ok - unlike(abcde, acd)
     # like: end
 ok - like
-# failed subtest: 15
+# failed subtest: 19
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
@@ -20,7 +20,7 @@ test.trace = false
 -- ok, fail and skip predicates
 --
 
-test:plan(32) -- plan to run 3 test
+test:plan(40) -- plan to run 40 test
 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")
+test:is(nil, box.NULL, "nil != box.NULL")
+test:is(box.NULL, box.NULL, "box.NULL == box.NULL")
+test:is(nil, nil, "nil == nil")
+test:isnt(box.NULL, nil, "box.NULL != nil")
+test:isnt(nil, box.NULL, "nil != box.NULL")
+test:isnt(box.NULL, box.NULL, "box.NULL == box.NULL")
+test:isnt(nil, nil, "nil == nil")
+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(18)
 
     t:is_deeply(1, 1, '1 and 1')
     t:is_deeply('abc', 'abc', 'abc and abc')
@@ -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
 end)
 
 
-- 
2.21.0

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

[Vladislav Shpilevoy]

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

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

[Kirill Shcherbatov]

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

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

[Vladislav Shpilevoy]

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
> 

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

[Alexander Turenko]

> @@ -163,6 +173,7 @@ local function is_deeply(test, got, expected, message, extra)
>      extra = extra or {}
>      extra.got = got
>      extra.expected = expected
> +    extra.strict = test.strict

The 'extra' parameter is needed to provide a user more information in
case of a fail. Here we use it to push options for cmpdeeply(). I would
add 'opts' parameter instead.

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

[Vladislav Shpilevoy]

On 24/04/2019 14:57, Alexander Turenko wrote:
>> @@ -163,6 +173,7 @@ local function is_deeply(test, got, expected, message, extra)
>>      extra = extra or {}
>>      extra.got = got
>>      extra.expected = expected
>> +    extra.strict = test.strict
> 
> The 'extra' parameter is needed to provide a user more information in
> case of a fail. Here we use it to push options for cmpdeeply(). I would
> add 'opts' parameter instead.
> 

If you do not like extra output about 'strict' value, then we
could make it 'nil' by default, not 'false', and it will not
be printed until someone starts to use it.

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

[Alexander Turenko]

On Wed, Apr 24, 2019 at 03:02:32PM +0300, Vladislav Shpilevoy wrote:
> 
> 
> On 24/04/2019 14:57, Alexander Turenko wrote:
> >> @@ -163,6 +173,7 @@ local function is_deeply(test, got, expected, message, extra)
> >>      extra = extra or {}
> >>      extra.got = got
> >>      extra.expected = expected
> >> +    extra.strict = test.strict
> > 
> > The 'extra' parameter is needed to provide a user more information in
> > case of a fail. Here we use it to push options for cmpdeeply(). I would
> > add 'opts' parameter instead.
> > 
> 
> If you do not like extra output about 'strict' value, then we
> could make it 'nil' by default, not 'false', and it will not
> be printed until someone starts to use it.

I'm ok if it is consistent with other such parameters. Kirill said it
is, so ok.

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

[Kirill Yukhin]

Hello,

On 23 Apr 15:55, Kirill Shcherbatov wrote:
> 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
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4125-inconsistent-isdeeply-results
> Issue: https://github.com/tarantool/tarantool/issues/4125

I've checked your patch into master and 2.1 branch.

--
Regards, Kirill Yukhin