[Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Igor Munkin]

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

Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Alexander Turenko]

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.

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.

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.

Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [19/11/26 19:57]:
> > rename from test/mmpairs_test.lua
> > rename to test/mmpairs-tarantool-4560.test.lua

*All* of the source code uses underscores, not dashes for file
names. 


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Vladislav Shpilevoy]

Hi!

On 26/11/2019 17:54, 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

We have already agreed on, and use a different test name pattern:

    gh-####-description.test.lua

Not

    <description>-####.test.lua.

Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Igor Munkin]

Vlad,

Thanks for your remark. On second thought I totally agree with you, so
I'll move this chunk one more time. Furthermore, I guess we ought to
move all results of this discussion to Lua style guide[1]:

On 26.11.19, Vladislav Shpilevoy wrote:
> Hi!
> 
> On 26/11/2019 17:54, 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
> 
> We have already agreed on, and use a different test name pattern:
> 
>     gh-####-description.test.lua
> 
> Not
> 
>     <description>-####.test.lua.

[1]: https://www.tarantool.io/en/doc/2.2/dev_guide/lua_style_guide/#testing

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Igor Munkin]

Kostja,

Thanks for your remark. I've seen no mention related to the test naming
in our contribution guide[1]. However, your point is valid: *all* other
chunks uses underscores, but not dashes, and the name used in this patch
differs from already existing ones.

Besides, there is another convention mentioned by Vlad and successfully
missed by me sometime.

I see the most convenient solution is adjusting the related test name to
a common style and further naming refactoring within some follow-up
activity.

On 26.11.19, Konstantin Osipov wrote:
> * Alexander Turenko <alexander.turenko@tarantool.org> [19/11/26 19:57]:
> > > rename from test/mmpairs_test.lua
> > > rename to test/mmpairs-tarantool-4560.test.lua
> 
> *All* of the source code uses underscores, not dashes for file
> names. 
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

[1]: https://www.tarantool.io/en/doc/2.2/dev_guide/lua_style_guide/#testing

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Konstantin Osipov]

* Igor Munkin <imun@tarantool.org> [19/11/27 11:15]:

> Thanks for your remark. I've seen no mention related to the test naming
> in our contribution guide[1]. However, your point is valid: *all* other
> chunks uses underscores, but not dashes, and the name used in this patch
> differs from already existing ones.

Well, I guess the contribution guidelines need to be updated then.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH] test: adjust the test name related to PAIRSMM flag

[Igor Munkin]

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