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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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<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.