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 CD1A96EC56; Tue, 16 Mar 2021 13:51:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CD1A96EC56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1615891912; bh=JagHEb8GRcbxKzcdWAFcmsGEaJrvSOv1SB570au4kBI=; 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=ykGSxMFVZCOUT45MTFiesZZvo74yJXjN5hTiVh5lirL+E8eRh7QYkA/Rdd9enrgA4 Rq7hmfDasKOw4DvDF2wNaNHlc/LA/kzSy5vZVsLG9UftlALv1dzXmWFaFCc9JCqGdw RtLiTQUJVIjhvug/JvxXpcE2RBZPsZENwvY0v0NU= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 1B7766EC56 for ; Tue, 16 Mar 2021 13:51:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1B7766EC56 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lM7Ij-0005hN-Tw; Tue, 16 Mar 2021 13:51:50 +0300 Date: Tue, 16 Mar 2021 13:51:42 +0300 To: Sergey Kaplun Message-ID: <20210316105142.GK9042@tarantool.org> References: <31d2b8f880693c4af3d47d7c6693acfd79459052.1615819534.git.skaplun@tarantool.org> <20210315174421.GG9042@tarantool.org> <20210316060142.GD16737@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210316060142.GD16737@root> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9D3134714A9BDB69BA93610A9EE4F09178597EFC5743F8DB100894C459B0CD1B91F60EEC8EC7205409CA03779A7219BD0A6AD1EDCE578EDE879C523755529B03C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE749E89BD568380EECC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7D77100FFB2844417EA1F7E6F0F101C67CDEEF6D7F21E0D1D174C73DBBBFC7664A2FCA2C37D531C93362E428BFE464E2EE9082D77DB9943AE389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0A29E2F051442AF778941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6A0EE70D6C4970CA7A471835C12D1D977C4224003CC836476EC64975D915A344093EC92FD9297F6718AA50765F7900637B7457ED07BBA7E3EA7F4EDE966BC389F395957E7521B51C24C7702A67D5C33162DBA43225CD8A89FC0F9454058DFE53CA91E23F1B6B78B78B5C8C57E37DE458B4C7702A67D5C3316FA3894348FB808DB48C21F01D89DB561574AF45C6390F7469DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5346745A2DA2C206A01A9B529CFB3DA752275B065E8D61E75D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F04B387B5D7535DE410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D349F4E6BC39AAD02BBDCCFE9EBDB8276753544BCC56947D3240CCA8C90452A4D57EB03D1265A088D571D7E09C32AA3244C6FBA35A69197DDA2B5AEE1D91E03BB2BE8FBBEFAE1C4874C927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojmD8gqPF9k0lBdZVQXig8Uw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822A42E5DA8EAC53C2ACA3E22D93CB62D6BA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 luajit 3/5] test: adjust lua-Harness test suite for Tarantool 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, On 16.03.21, Sergey Kaplun wrote: > Igor, > > Thanks for the review! > > On 15.03.21, Igor Munkin wrote: > > Sergey, > > > > This is ridiculous: you split two similar renames required by the one > > issue into *two* separate commits, but leaving the changes *totally > > unrelated to each other* within a single commit. Please, split this > > patch into two: one for the test directory tweak and another with > > mocking environment variable in CMake. > > Sorry, but why then you said nothing about commits separation/joining > for this [1] draft series, as I asked for? I mentioned it here[1] and you decided to left everything intact (that's OK for me). This commit is a new one and you created it from two separate commits in the previous version. So it's just a new feedback for the new changes. > > Offline we came to the agreement that we should use 3 commits for each > test suite. The first to onboard it, the second to adjust for LuaJIT, > the third to adjust it for Tarantool. I remember only 3 bullets on which we have explicitly agreed: * The suite should be taken intact in the first commit despite the fact tests can be broken. * LuaJIT related changes should be separated from Tarantool related ones to ease their further maintenance. Furthermore, every change should be mentioned in our GitHub queue. * All tests should be run in a unified way via both LuaJIT and Tarantool testing machinery. It means test chunk or test case is either run or not despite the testing environment. Regarding everything else I was open to discuss it on review, since it's too hard (at least for me) to consider all issues faced while adopting these suites and create the most unified way to handle them. I believe, this is the purpose of review process: to see what is done and think whether it can be done in a better way (better doesn't mean *you* made it wrong, but *we* chose a bad solution). > Later your asked me to send Mergens changes as is. > > I specially send draft to discuss the commits content, order and so on. > You said only about separating patchset for different test suites and > merged LuaJIT test suite, so I thought that commit order is OK for you. > I'd merged changes with Tarantool-related one. I hope you remember that I've asked a couple of times to polish and send LuaJIT-test-cleanup patchset. The last time I was asking was right before you sent WIP series. LuaJIT related patches per se were ready to be merged; the only issue was disabling strict for it. Additionally I've adjusted tarantool-tests runner to use LUAJIT_TEST_COMMAND to make all tests be run by unified way. Then I asked you to split two remaining suites into two series so I can look on them separately. Hence, I was OK with the general approach, but was afraid to miss some details. > > If it is not comfortable for you getting WIP series and discussing them > let's decline this practise. I'm totally fine with WIP series: e.g. LUAJIT_TEST_INIT has been done after I've glanced all the patches with tests you've sent. > > Back to business, I've split the patch into two, considering your > proposal, see them below (their order is the same). Nice, thanks a lot! > > > Patch for test directory tweak. Adjusted considering [2]. > > =================================================================== > commit a84980334dce27243a25508e3bb8fd491689e552 > Author: Sergey Kaplun > Date: Mon Mar 15 16:24:07 2021 +0300 > > test: adjust lua-Harness tests that using dofile > > This patch makes out-of-source execution lua-Harness suite tests that > using `dofile()` correct. Minor: There are other tests using , but they are fine. The root problem relates to the approach used in the patched tests: the .t files are kinda wrapper, containing the basic checks (or not, e.g. UTF-8 test chunk), and some specific checks are moved to the auxiliary chunks "dofiled" in .t script. > > There are the following files that used `dofile()` function > on file in test sources directory: > * 101-boolean.t > * 102-function.t > * 103-nil.t > * 104-number.t > * 105-string.t > * 106-table.t > * 107-thread.t > * 108-userdata.t > * 203-lexico.t > * 231-metatable.t > * 301-basic.t > * 305-utf8.t > * 404-ext.t > > `dofile()` looks for files to execute in the current working directory, > that might be not the same as the test source directory. > > This patch introduces the new function `dofile_fullpath()` that What does "fullpath" mean? If this is absolute path, then naming is ambiguous; and I guess nothing stops one from using relative paths (but not symlink, I think) here. Furthermore, the naming doesn't represent the purpose: as I mentioned above this dofile is needed to implement specific checks, so you can adjust the naming to more verbose one: or . Thoughts? > evaluates full path to file considering arg[0] (i.e. test filename) It's worth to check and mention the caveat with symlinks. If the issue exists, then the paths should be resolved in CMake the way similar to the one used for luacheck target. > value. > > Part of tarantool/tarantool#5844 > Part of tarantool/tarantool#4473 > > =================================================================== > > Patch for mocking environment variable in CMake. > > =================================================================== > commit 483508b0a7863efabcde6d232ab9af2033e0011f > Author: Sergey Kaplun > Date: Mon Mar 15 22:08:07 2021 +0300 > > test: forcify set USERNAME env var for lua-Harness Strictly saying, you do not force, but just set. Furthermore, I doubt about word formation you applied to "force". > > 309-os.t checks `os.getenv()` function by examining of Typo: examine , not examine of [2]. > USERNAME or LOGNAME environment variable. > These variables might not be set in the environment, that leads to test > failure. > > This patchs sets manually USERNAME environment variable for Typo: s/manually/explicitly/. > lua-Harness-tests target. > > Part of tarantool/tarantool#5844 > Part of tarantool/tarantool#4473 > > diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt > index f8611ce..b844788 100644 > --- a/test/lua-Harness-tests/CMakeLists.txt > +++ b/test/lua-Harness-tests/CMakeLists.txt > @@ -34,6 +34,11 @@ add_custom_command(TARGET lua-Harness-tests > env > LUA_PATH="${LUA_PATH}\;" > LUA_CPATH="${LUA_CPATH}\;" > + # XXX: 309-os.t checks os.getenv() function by examining of Typo: examine , not examine of [2]. > + # USERNAME or LOGNAME environment variable. > + # These variables might not be set in the environment, so > + # set one of them manually. > + USERNAME="fperrad" > ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR} > --exec '${LUAJIT_TEST_COMMAND} -l profile_luajit21' > ${LUA_TEST_FLAGS} > =================================================================== > > [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-March/022563.html > [2]: https://lists.tarantool.org/pipermail/tarantool-patches/2021-March/022710.html > > -- > Best regards, > Sergey Kaplun [1]: https://lists.tarantool.org/tarantool-patches/16E39C08-397B-4A38-953A-B9EB85CD9C8B@tarantool.org/T/#m9de07af72e06ac22b7540741b9e1c4ac485688bb [2]: https://dictionary.cambridge.org/dictionary/english/examine -- Best regards, IM