From: Igor Munkin <imun@tarantool.org> To: Alexander Turenko <alexander.turenko@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag Date: Wed, 27 Nov 2019 18:50:17 +0300 [thread overview] Message-ID: <20191127155017.GD16745@tarantool.org> (raw) In-Reply-To: <20191126165407.ncmd7hepnpifgv6v@tkn_work_nb> Sasha, Thanks for the review! On 26.11.19, Alexander Turenko wrote: > On Tue, Nov 26, 2019 at 06:47:38PM +0300, Igor Munkin wrote: > > The prior name doesn't respect the naming convention used by test-run > > thus this chunk was ignored by our testing machinery. > > > > Part-of tarantool/tarantool#4560 > > > > Signed-off-by: Igor Munkin <imun@tarantool.org> > > --- > > > > Branch: https://github.com/tarantool/luajit/tree/imun/test-name-fixup > > Issue: https://github.com/tarantool/tarantool/issues/4560 > > > > > > test/{mmpairs_test.lua => mmpairs-tarantool-4560.test.lua} | 0 > > 1 file changed, 0 insertions(+), 0 deletions(-) > > rename test/{mmpairs_test.lua => mmpairs-tarantool-4560.test.lua} (100%) > > > > diff --git a/test/mmpairs_test.lua b/test/mmpairs-tarantool-4560.test.lua > > similarity index 100% > > rename from test/mmpairs_test.lua > > rename to test/mmpairs-tarantool-4560.test.lua > > -- > > 2.24.0 > > > > Eagle eye! > > LGTM. > > Please, ensure that the test name will be updated in tarantool (with the > submodule update) only after we'll enable LUAJIT_ENABLE_PAIRSMM macro in > tarantool for luajit's build. Considering messages sent by Vlad and Kostja, we have some mess with the naming policy. Kostja's remark controverts with the policy proposed by Kirill some time ago. As Vlad mentioned, we agreed on the following naming pattern: | gh-####-description.test.lua However, since we backport bugs related to the issues from other queues, the `gh-####` prefix is ambigious. Thereby I guess the following pattern is the most convenient one for LuaJIT related tests: | <origin-issue-repo>-####-description.test.lua Here are some examples: * LuaJIT-505-fold-bug-in-string-find.test.lua * tarantool-3196-bug-with-zero-hash-strings.test.lua We don't have any documented policy, so I filed a ticket[1] to tarantool/doc queue with the naming described above and mentioned you, Kirill, Vlad and Kostja. I can write the corresponding message to our discussion list on demand. I'll send a v2 for this changeset regarding the common naming for the existing tests in LuaJIT suite. > > Several comments around are below. > > ---- > > Re test itself: > > * Use `local` for variables (`tap`, `test`). > * Use `os.exit(test:check() and 0 or 1)` at the end. > As I see all tests violate both comments you left above. Therefore I've created an issue[2] for the corresponding follow-up activity. > The test depends on tarantool indirectly: we (will) set > LUAJIT_ENABLE_PAIRSMM macro in tarantool when building LuaJIT, so the > test formally may not be applicable for luajit itself (despite that it > is placed in the luajit's repo). > > I don't see real problems here, to be honest; just feel it a bit less > strict then it maybe should be. > > We discussed possible ways to skip the test when it will not be > appropriate with Igor and they are the following. > > * mmpairs-tarantool-4560.skipcond Python script that look into some > generated header (don't know whether luajit has it) to determine > options with which luajit was built. > * mmpairs-tarantool-4560.skipcond that calls, say, `nm` to deduce build > flags from symbols in luajit executable. > * Look at _TARANTOOL global variable from the test and do a couple of > test:skip(). > > The latter is simplest. > > Anyway, it is not very important now and surely should not be part of > this patch. Let's skip all those thoughts or add the simplest check > separately if you wish. > > WBR, Alexander Turenko. [1]: https://github.com/tarantool/doc/issues/1004 [2]: https://github.com/tarantool/tarantool/issues/4655 -- Best regards, IM
prev parent reply other threads:[~2019-11-27 15:52 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-26 15:47 Igor Munkin 2019-11-26 16:54 ` Alexander Turenko 2019-11-26 20:58 ` Konstantin Osipov 2019-11-26 21:32 ` Igor Munkin 2019-11-27 8:32 ` Konstantin Osipov 2019-11-26 21:03 ` Vladislav Shpilevoy 2019-11-26 21:10 ` Igor Munkin 2019-11-27 15:50 ` Igor Munkin [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191127155017.GD16745@tarantool.org \ --to=imun@tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox