From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 785B02ADDF7; Mon, 27 Feb 2023 12:18:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 785B02ADDF7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1677489530; bh=0QKJB4QMDsiTVTZI7OV4VN0V7qA0p6uBls7SEhZCoto=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=YDSaFcEGTKkdkaUSqlY++XkQZKqr8B8+SIHGSvuoMqDURjlJ7Qp5gN2KQYW4rmLhT WujV336idSLcTHbPMtSXtpPKs2ewwD5XKXrcK5m2Ue6d2hZGoTScaOCXNUCBM3pCvT 9BC+GuIT0VNylPtBjzHqF1pcckrvWActFDYkTars= Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [95.163.41.79]) (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 2BA8E2ADDF4 for ; Mon, 27 Feb 2023 12:18:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2BA8E2ADDF4 Received: by smtp38.i.mail.ru with esmtpa (envelope-from ) id 1pWZeh-000MxS-Qb; Mon, 27 Feb 2023 12:18:48 +0300 Date: Mon, 27 Feb 2023 09:16:08 +0000 To: Sergey Kaplun Message-ID: References: <73073fb24db65f59c8629f4f377faa9e52cb54af.1676304797.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Clacks-Overhead: GNU Terry Pratchett X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD9806C989EC2150E33C13C456498AA74328729F4FEF7F411FF182A05F5380850404458065AF786F5C47A153711B3BDFBDE62CBBD8C8096F062D2018693BA073F39 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE721AF84DC1D70954DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371AA4FDB8B3812E678638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8B77E2EB759D0EE4CB94E2B6F73862EE2117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF80095D1E17F4578A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520B28585415E75ADA9F04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EECCD848CCB6FE560C040F9FF01DFDA4A84AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3E1AB39E79EA2BE51BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE7D151390FFDBF6399731C566533BA786AA5CC5B56E945C8DA X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D342C0B628602DFD0BC6933780F557E752A283D9358D7ECECF4E10D99BA32672362C5C9D1BB738314321D7E09C32AA3244CB885A8D24B869852A1A2F413694099355A1673A01BA68E40927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojY+HhiuGRi+osoYIAkUqv7w== X-Mailru-Sender: 2FEBA92C8E508479FE7B9A1DF348D531569EBBEE27F44552EDC6575C5E1378A8F2D242151ECE40F42326FE6F2A341ACE0FB9F97486540B4CD9E8847AB8CFED4D9ABF8A61C016C2CFB0DAF586E7D11B3E67EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 3/7] test: stop using utils.selfrun in tests X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, Thanks for your review! See my answers below. On 15.02.23, Sergey Kaplun wrote: > Hi, Igor! > Thanks for the patch! > Generally LGTM, but please consider my entreaties about comments below. > > On 13.02.23, Igor Munkin wrote: > > Unfortunately, is too complex to be maintained, so the > > corresponding tests are split into two files: the test itself and the > > script to be run by the test. As a result of the patch > > helper is introduced: it inherits some approaches from , > > but it's considered for more general use. > > > > Relates to tarantool/tarantool#8252 > > > > Signed-off-by: Igor Munkin > > --- > > > > > diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua > > index dd02130c..f4795db0 100644 > > --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua > > +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua > > @@ -3,52 +3,43 @@ local utils = require('utils') > > > > > --- Save the current coroutine and set the value to trigger > > --- call the Lua routine instead of C implementation. > > -local sandwich = require('libsandwich')(cfg.trigger) > > +local script = utils.makecmd(arg, { > > It will be nice to drop a comment here and below, that `makecmd()` > searches for <%testname%/script.lua>. > > > + -- TODO: Leave another toxic comment regarding SIP on macOS. > > Minor: This TODO is unnecessary, so feel free to delete this line. > > > + env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') }, > > + redirect = '2>&1', > > +}) Fixed both cases above. You can find the diff below: ================================================================================ diff --git a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua index ff3eaf01..ad06c329 100644 --- a/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua +++ b/test/tarantool-tests/gh-4427-ffi-sandwich.test.lua @@ -6,8 +6,23 @@ local test = tap.test('gh-4427-ffi-sandwich'):skipcond({ test:plan(2) +-- runs %testname%/script.lua by +-- with the given environment, launch options and CLI arguments. local script = require('utils').makecmd(arg, { - -- TODO: Leave another toxic comment regarding SIP on macOS. + -- XXX: Apple tries their best to "protect their users from + -- malware". As a result SIP (see the link[1] below) has been + -- designed and released. Now, Apple developers are so + -- protected, that they can load nothing being not installed in + -- the system, since the environment is sanitized before the + -- child process is launched. In particular, environment + -- variables starting with DYLD_ and LD_ are unset for child + -- process. For more info, see the docs[2] below. + -- + -- The environment variable below is used by FFI machinery to + -- find the proper shared library. + -- + -- [1]: https://support.apple.com/en-us/HT204899 + -- [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') }, redirect = '2>&1', }) ================================================================================ > > > > -- Depending on trigger and hotloop values the following contexts > > -- are possible: > > -- * if trigger <= hotloop -> trace recording is aborted > > -- * if trigger > hotloop -> trace is recorded but execution > > -- leads to panic > > -jit.opt.start("3", string.format("hotloop=%d", cfg.hotloop)) > > +local hotloop = 1 > > +local cases = { > > + abort = { > > + trigger = hotloop, > > + expected = '#4427 still works', > > Side note: We may provide this message from here to avoid it copy-pasting in > , but this reduces the test > readability... At least, we need to call format twice in the test's > payload, so for better good is not to do it. Yeah, there is no silver bullet for this, unfortunately. This is lesser evil in our case, IMHO. > > > + test = 'is', > > + message = 'Trace is aborted', > > + }, > > + panic = { > > + trigger = hotloop + 1, > > + expected = 'Lua VM re%-entrancy is detected while executing the trace', > > + test = 'like', > > + message = 'Trace is compiled', > > + }, > > +} > > diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua > > index c46b93f0..cf92757c 100644 > > --- a/test/tarantool-tests/lj-flush-on-trace.test.lua > > +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua > > > > > --- Save the current coroutine and set the value to trigger > > --- call the Lua routine instead of C implementation. > > -local flush = require('libflush')(cfg.trigger) > > +local script = utils.makecmd(arg, { > > + -- TODO: Leave another toxic comment regarding SIP on macOS. > > Minor: This TODO is unnecessary, so feel free to delete this line. Added the same comments as for the case above. The diff is below: ================================================================================ diff --git a/test/tarantool-tests/lj-flush-on-trace.test.lua b/test/tarantool-tests/lj-flush-on-trace.test.lua index e157622a..a355c688 100644 --- a/test/tarantool-tests/lj-flush-on-trace.test.lua +++ b/test/tarantool-tests/lj-flush-on-trace.test.lua @@ -6,8 +6,23 @@ local test = tap.test('lj-flush-on-trace'):skipcond({ test:plan(2) +-- runs %testname%/script.lua by +-- with the given environment, launch options and CLI arguments. local script = require('utils').makecmd(arg, { - -- TODO: Leave another toxic comment regarding SIP on macOS. + -- XXX: Apple tries their best to "protect their users from + -- malware". As a result SIP (see the link[1] below) has been + -- designed and released. Now, Apple developers are so + -- protected, that they can load nothing being not installed in + -- the system, since the environment is sanitized before the + -- child process is launched. In particular, environment + -- variables starting with DYLD_ and LD_ are unset for child + -- process. For more info, see the docs[2] below. + -- + -- The environment variable below is used by FFI machinery to + -- find the proper shared library. + -- + -- [1]: https://support.apple.com/en-us/HT204899 + -- [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/RuntimeProtections/RuntimeProtections.html env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') }, redirect = '2>&1', }) ================================================================================ > > > + env = { DYLD_LIBRARY_PATH = os.getenv('DYLD_LIBRARY_PATH') }, > > + redirect = '2>&1', > > +}) > > diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua > > index eb11d40d..41a7c22a 100644 > > --- a/test/tarantool-tests/utils.lua > > +++ b/test/tarantool-tests/utils.lua > > > > > +function M.makecmd(arg, opts) > > Please, add the comment about script location, it's good thing to > commment, IMHO. Added the comment to clear the usage. The diff is below: ================================================================================ diff --git a/test/tarantool-tests/utils.lua b/test/tarantool-tests/utils.lua index 4fb66600..6a9645ca 100644 --- a/test/tarantool-tests/utils.lua +++ b/test/tarantool-tests/utils.lua @@ -52,6 +52,11 @@ local function makeenv(tabenv) return table.concat(flatenv, ' ') end +-- creates a command that runs %testname%/script.lua by +-- with the given environment, launch options +-- and CLI arguments. The function yields an object (i.e. table) +-- with the aforementioned parameters. To launch the command just +-- call the object. function M.makecmd(arg, opts) return setmetatable({ LUABIN = M.luacmd(arg), ================================================================================ > > > + return setmetatable({ > > + LUABIN = M.luacmd(arg), > > + SCRIPT = opts and opts.script or arg[0]:gsub('%.test%.lua$', '/script.lua'), > > + ENV = opts and makeenv(opts.env) or '', > > Is this option is used only for DYLD_LIBRARY_PATH (as I can see for now)? > Should we drop the comment about that here (that this is the main > reason)? Not really. This is just a general approach to tweak the child process environment. And yes, it's used only for DYLD_LIBRARY_PATH for now, but I want to create a general interface to run commands in tests. > > > + REDIRECT = opts and opts.redirect or '', > > + }, { > > + __call = function(self, ...) > > + local cmd = ('