Re: [Tarantool-patches] [PATCH] lua/key_def: fix compare_with_key() part count check

[Alexander Turenko <alexander.turenko@tarantool.org>]

> diff --git a/test/app-tap/gh-5307-key_def-part-count-check.result b/test/app-tap/gh-5307-key_def-part-count-check.result
> new file mode 100644
> index 000000000..d47a905a5
> --- /dev/null
> +++ b/test/app-tap/gh-5307-key_def-part-count-check.result
> @@ -0,0 +1 @@
> +Failed (as it should), error message: Invalid key part count (expected [0..1], got 9)
> diff --git a/test/app-tap/gh-5307-key_def-part-count-check.test.lua b/test/app-tap/gh-5307-key_def-part-count-check.test.lua
> new file mode 100755
> index 000000000..8c8f6f34b
> --- /dev/null
> +++ b/test/app-tap/gh-5307-key_def-part-count-check.test.lua

box-tap fit better here, because key_def (C part and the Lua module) is
part of the database implementation ('box').

> @@ -0,0 +1,25 @@
> +#!/usr/bin/env tarantool
> +
> +key_def = require('key_def')
> +kd = key_def.new({{fieldno = 1, type = 'unsigned'}})

`x = 42` is equivalent of `_G.x = 42` if there is no definition of `x`
in the scope.

I would use 'local' for such code hunks. It does not change any
behaviour here, just for the sake of our usual style for Lua modules.

In a module it is undesirable to set a global variable, which accessible
for other modules. At least for a variable that is not intended to be
exported.

> +
> +-- Should succeed
> +if (kd:compare_with_key({1}, {1}) ~= 0) then
> +    print("Error: should be equal")
> +end

I would prefer to use 'tap' built-in module ([1]) for such tests. It
produces TAP13 output and does not require a result file. Most of tests
in the '*-tap' test suites procude TAP13 output.

[1]: https://www.tarantool.io/en/doc/latest/reference/reference_lua/tap/

> +
> +-- Should succeed
> +if (kd:compare_with_key({1}, {2}) >= 0) then
> +    print("Error: should be less")
> +end
> +
> +-- Should fail
> +ok, err = pcall(function()
> +	kd:compare_with_key({1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
> +    end)

The indentation is strange here. I would write it like so:

 | local exp_err = '<..expected error message..>'
 | local ok, err = pcall(kd.compare_with_key, kd, {1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
 | test:is_deeply({ok, tostring(err)}, {false, exp_err}, '<..short description..>')

tostring() is to convert a box.error object to a string, otherwise
is_deeply() fails to find them equal.

Or, with extra function:

 | local exp_err = '<..expected error message..>'
 | local ok, err = pcall(function()
 |     return kd:compare_with_key({1}, {1, 2, 3, 4, 5, 6, 7, 8, 9})
 | end)
 | test:is_deeply({ok, tostring(err)}, {false, exp_err}, '<..short description..>')

(Personally I prefer to avoid extra wrapping function, when we want to
just call one function under pcall).

> +if (ok) then
> +    print("Error: it should not have succeeded")
> +else
> +    print("Failed (as it should), error message: " .. err)
> +end

An if condition may be written without parentheses in Lua.

I would explicitly set an exit code:

 | os.exit(test:check() and 0 or 1)

This way we have more freedom in automation: say, we can run a subset of
tests without test-run and check their exit code without checks for
output. To be honest, I just find it more intuitive, when a failing test
process set a non-zero exit code.

It would be great to also open a PR to the tuple-keydef module with the
new test. I fixed the corresponding problem in the module, but don't add
any test for it.

https://github.com/tarantool/tuple-keydef/issues/7