From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E96AC4304B4 for ; Thu, 17 Dec 2020 15:37:23 +0300 (MSK) Date: Thu, 17 Dec 2020 15:37:32 +0300 From: Alexander Turenko Message-ID: <20201217123732.rnaz2eu6752qk42f@tkn_work_nb> References: <20201214153527.451373-1-void@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20201214153527.451373-1-void@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] lua/key_def: fix compare_with_key() part count check List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Nikiforov Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy > 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