From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Kaplun <skaplun@tarantool.org>
Cc: Sergey Bronnikov <estetus@gmail.com>,
tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH luajit 2/2] test: skip sysprof tests with LUAJIT_DISABLE_SYSPROF
Date: Fri, 6 Dec 2024 13:31:24 +0300 [thread overview]
Message-ID: <Z1LSfI1ZFtlUDdEs@pony> (raw)
In-Reply-To: <Z1KxGeslhn3-gKFy@root>
Hi, Sergey,
fixes applied and force-pushed.
On 11:08 Fri 06 Dec , Sergey Kaplun wrote:
>Hi, Sergey!
>Thanks for the patch!
>LGTM, with a minor comment below.
>
>On 06.12.24, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> LuaJIT has a macro LUAJIT_DISABLE_SYSPROF that disable
>
>Typo: s/disable/disables/
Fixed.
>
>> sysprof support. Sysprof tests doesn't respect this macro
>
>Typo: s/doesn't/don't/
>Typo: s/macro/macro,/
Fixed.
>
>> and therefore some of them failed when macro is enabled.
>
>Typo: /macro/macro,/
Fixed as /macro/the macro/.
>
>>
>> The proposed patch:
>
>This isn't proposed patch, but it is just patch, so:
>s/proposed //
>
>>
>> - skip sysprof testcases in a suite tarantool-c-tests
>
>Typo: /skip/Skips/
>Missed dot at the end of the sentence.
Fixed.
>
>> - introduce an environment variable LUAJIT_DISABLE_SYSPROF in
>
>Typo: s/introduce/Introduces/
Fixed.
>
>> a suite tarantool-tests that set to 1 when sysprof support is
>
>Typo: s/set/is set/
Fixed.
>
>> disabled
>
>Missed dot at the end of the sentence.
Fixed.
>
>> - propagate a status of sysprof support to Lua tests and skip
>
>Typo: s/propagate/Propagates
Fixed.
>
>> testing when sysprof is disabled
>
>Missed dot at the end of the sentence.
Fixed.
>
>> ---
>> test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c | 4 ++++
>> test/tarantool-c-tests/misclib-sysprof-capi.test.c | 4 ++++
>> test/tarantool-tests/CMakeLists.txt | 4 ++++
>> test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua | 2 ++
>> .../profilers/gh-5813-resolving-of-c-symbols.test.lua | 2 ++
>> .../gh-7264-add-proto-trace-sysprof-default.test.lua | 2 ++
>> .../profilers/gh-9217-profile-parsers-error-handling.test.lua | 2 ++
>> test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua | 2 ++
>> 8 files changed, 22 insertions(+)
>>
>> diff --git a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
>> index a8de3358..de4fb2c4 100644
>> --- a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
>> +++ b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
>> @@ -277,8 +277,12 @@ static int test_tostring_call(void *ctx)
>>
>> int main(void)
>> {
>> +#if LUAJIT_DISABLE_SYSPROF
>> + return skip_all("Sysprof is disabled (LUAJIT_DISABLE_SYSPROF)");
>
>Minor: I suppose we can drop the LUAJIT_DISABLE_SYSPROF mentioning since
>it becomes "maslo maslyanoe", for my taste.
>Feel free to ignore.
the intention was to describe a reason explicitly.
Update skip message:
diff --git a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
index de4fb2c4..a83a0921 100644
--- a/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
+++ b/test/tarantool-c-tests/gh-8594-sysprof-ffunc-crash.test.c
@@ -278,7 +278,7 @@ static int test_tostring_call(void *ctx)
int main(void)
{
#if LUAJIT_DISABLE_SYSPROF
- return skip_all("Sysprof is disabled (LUAJIT_DISABLE_SYSPROF)");
+ return skip_all("Sysprof is disabled");
#else /* LUAJIT_DISABLE_SYSPROF */
const struct test_unit tgroup[] = {
test_unit_def(test_tostring_call),
diff --git a/test/tarantool-c-tests/misclib-sysprof-capi.test.c b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
index ec3354cc..21e40d22 100644
--- a/test/tarantool-c-tests/misclib-sysprof-capi.test.c
+++ b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
@@ -302,7 +302,7 @@ static int profile_func_jiton(void *test_state)
int main(void)
{
#if LUAJIT_DISABLE_SYSPROF
- return skip_all("Sysprof is disabled (LUAJIT_DISABLE_SYSPROF)");
+ return skip_all("Sysprof is disabled");
#else /* LUAJIT_DISABLE_SYSPROF */
if (LUAJIT_OS != LUAJIT_OS_LINUX)
return skip_all("Sysprof is implemented for Linux only");
diff --git a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
index 379ccf8e..e3cb86c0 100644
--- a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
+++ b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
@@ -1,5 +1,4 @@
local tap = require('tap')
-local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
jit.arch ~= 'x64',
@@ -8,7 +7,7 @@ local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
['No profile tools CLI option integration'] = _TARANTOOL,
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
- ['Disabled due to LUAJIT_DISABLE_SYSPROF'] = no_sysprof,
+ ['Sysprof is disabled'] = os.getenv('LUAJIT_DISABLE_SYSPROF'),
})
test:plan(3)
diff --git a/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua b/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua
index ff745225..4af1cf79 100644
--- a/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua
+++ b/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua
@@ -1,12 +1,11 @@
local tap = require("tap")
-local no_sysprof = os.getenv("LUAJIT_DISABLE_SYSPROF")
local test = tap.test("gh-5813-resolving-of-c-symbols"):skipcond({
["Memprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
jit.arch ~= "x64",
["Memprof is implemented for Linux only"] = jit.os ~= "Linux",
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
- ["Disabled due to LUAJIT_DISABLE_SYSPROF"] = no_sysprof,
+ ["Sysprof is disabled"] = os.getenv("LUAJIT_DISABLE_SYSPROF")
})
test:plan(5)
diff --git a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
index e09016e7..0a909819 100644
--- a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
+++ b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
@@ -1,5 +1,4 @@
local tap = require('tap')
-local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
['Test requires JIT enabled'] = not jit.status(),
['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
@@ -7,7 +6,7 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
- ['Disabled due to LUAJIT_DISABLE_SYSPROF'] = no_sysprof,
+ ['Sysprof is disabled'] = os.getenv('LUAJIT_DISABLE_SYSPROF'),
})
test:plan(2)
diff --git a/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua b/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua
index 012b1501..68c1b726 100644
--- a/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua
+++ b/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua
@@ -1,5 +1,4 @@
local tap = require('tap')
-local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({
['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
jit.arch ~= 'x64',
@@ -8,7 +7,7 @@ local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({
['No profile tools CLI option integration'] = _TARANTOOL,
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
- ['Disabled due to LUAJIT_DISABLE_SYSPROF'] = no_sysprof,
+ ['Sysprof is disabled'] = os.getenv('LUAJIT_DISABLE_SYSPROF'),
})
jit.off()
diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
index ea5ca9a9..711ca646 100644
--- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
+++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
@@ -1,12 +1,11 @@
local tap = require("tap")
-local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
local test = tap.test("misc-sysprof-lapi"):skipcond({
["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
jit.arch ~= "x64",
["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
-- See also https://github.com/LuaJIT/LuaJIT/issues/606.
["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
- ["Disabled due to LUAJIT_DISABLE_SYSPROF"] = no_sysprof,
+ ["Sysprof is disabled"] = os.getenv('LUAJIT_DISABLE_SYSPROF'),
})
test:plan(19)
>> +#else /* LUAJIT_DISABLE_SYSPROF */
>
><snipped>
>
>> diff --git a/test/tarantool-c-tests/misclib-sysprof-capi.test.c b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
>> index 6f53d488..ec3354cc 100644
>> --- a/test/tarantool-c-tests/misclib-sysprof-capi.test.c
>> +++ b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
>> @@ -301,6 +301,9 @@ static int profile_func_jiton(void *test_state)
>>
>> int main(void)
>> {
>> +#if LUAJIT_DISABLE_SYSPROF
>> + return skip_all("Sysprof is disabled (LUAJIT_DISABLE_SYSPROF)");
>
>Ditto.
>
>> +#else /* LUAJIT_DISABLE_SYSPROF */
>> if (LUAJIT_OS != LUAJIT_OS_LINUX)
>> return skip_all("Sysprof is implemented for Linux only");
>> if (LUAJIT_TARGET != LUAJIT_ARCH_X86
>> @@ -322,4 +325,5 @@ int main(void)
>> const int test_result = test_run_group(tgroup, L);
>> utils_lua_close(L);
>> return test_result;
>> +#endif /* LUAJIT_DISABLE_SYSPROF */
>> }
>> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>> index 8bdb2cf3..a6e39f6c 100644
>> --- a/test/tarantool-tests/CMakeLists.txt
>> +++ b/test/tarantool-tests/CMakeLists.txt
>> @@ -89,6 +89,10 @@ if(LUAJIT_ENABLE_TABLE_BUMP)
>> list(APPEND LUA_TEST_ENV_MORE LUAJIT_TABLE_BUMP=1)
>> endif()
>>
>> +if(LUAJIT_DISABLE_SYSPROF)
>> + list(APPEND LUA_TEST_ENV_MORE LUAJIT_DISABLE_SYSPROF=1)
>> +endif()
>> +
>> set(TEST_SUITE_NAME "tarantool-tests")
>>
>> # XXX: The call produces both test and target
>> diff --git a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
>> index f935dc5b..379ccf8e 100644
>> --- a/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
>> +++ b/test/tarantool-tests/profilers/gh-5688-tool-cli-flag.test.lua
>> @@ -1,4 +1,5 @@
>> local tap = require('tap')
>> +local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
>> local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
>> ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
>> jit.arch ~= 'x64',
>> @@ -7,6 +8,7 @@ local test = tap.test('gh-5688-tool-cli-flag'):skipcond({
>> ['No profile tools CLI option integration'] = _TARANTOOL,
>> -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
>> ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
>> + ['Disabled due to LUAJIT_DISABLE_SYSPROF'] = no_sysprof,
>
>Minor: "disabled due to disable" sounds like "maslo maslyanoe" for my
>taste. I suppose "Sysprof is disabled" is just enough. As a minor bonus,
>we may avoid the declaration of the `no_sysprof` variable since we can
>just use one-line `os.getenv()` that fits into this line now.
>
>Same for the other Lua tests.
Updated.
>
>> })
>>
>> test:plan(3)
>> diff --git a/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua b/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua
>> index 74bcd9e8..ff745225 100644
>> --- a/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua
>> +++ b/test/tarantool-tests/profilers/gh-5813-resolving-of-c-symbols.test.lua
>> @@ -1,10 +1,12 @@
>> local tap = require("tap")
>> +local no_sysprof = os.getenv("LUAJIT_DISABLE_SYSPROF")
>> local test = tap.test("gh-5813-resolving-of-c-symbols"):skipcond({
>> ["Memprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
>> jit.arch ~= "x64",
>> ["Memprof is implemented for Linux only"] = jit.os ~= "Linux",
>> -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
>> ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
>> + ["Disabled due to LUAJIT_DISABLE_SYSPROF"] = no_sysprof,
>> })
>>
>> test:plan(5)
>> diff --git a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
>> index 176c1a15..e09016e7 100644
>> --- a/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
>> +++ b/test/tarantool-tests/profilers/gh-7264-add-proto-trace-sysprof-default.test.lua
>> @@ -1,4 +1,5 @@
>> local tap = require('tap')
>> +local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
>> local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
>> ['Test requires JIT enabled'] = not jit.status(),
>> ['Sysprof is implemented for x86_64 only'] = jit.arch ~= 'x86' and
>> @@ -6,6 +7,7 @@ local test = tap.test('gh-7264-add-proto-trace-sysprof-default'):skipcond({
>> ['Sysprof is implemented for Linux only'] = jit.os ~= 'Linux',
>> -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
>> ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
>> + ['Disabled due to LUAJIT_DISABLE_SYSPROF'] = no_sysprof,
>> })
>>
>> test:plan(2)
>> diff --git a/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua b/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua
>> index 65c51198..012b1501 100644
>> --- a/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua
>> +++ b/test/tarantool-tests/profilers/gh-9217-profile-parsers-error-handling.test.lua
>> @@ -1,4 +1,5 @@
>> local tap = require('tap')
>> +local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
>> local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({
>> ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and
>> jit.arch ~= 'x64',
>> @@ -7,6 +8,7 @@ local test = tap.test('gh-9217-profile-parsers-error-handling'):skipcond({
>> ['No profile tools CLI option integration'] = _TARANTOOL,
>> -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
>> ['Disabled due to LuaJIT/LuaJIT#606'] = os.getenv('LUAJIT_TABLE_BUMP'),
>> + ['Disabled due to LUAJIT_DISABLE_SYSPROF'] = no_sysprof,
>> })
>>
>> jit.off()
>> diff --git a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> index 26c277cd..ea5ca9a9 100644
>> --- a/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> +++ b/test/tarantool-tests/profilers/misclib-sysprof-lapi.test.lua
>> @@ -1,10 +1,12 @@
>> local tap = require("tap")
>> +local no_sysprof = os.getenv('LUAJIT_DISABLE_SYSPROF')
>> local test = tap.test("misc-sysprof-lapi"):skipcond({
>> ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
>> jit.arch ~= "x64",
>> ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
>> -- See also https://github.com/LuaJIT/LuaJIT/issues/606.
>> ["Disabled due to LuaJIT/LuaJIT#606"] = os.getenv("LUAJIT_TABLE_BUMP"),
>> + ["Disabled due to LUAJIT_DISABLE_SYSPROF"] = no_sysprof,
>> })
>>
>> test:plan(19)
>> --
>> 2.34.1
>>
>
>--
>Best regards,
>Sergey Kaplun
--
sergeyb@
next prev parent reply other threads:[~2024-12-06 10:31 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-06 7:05 [Tarantool-patches] [PATCH luajit 0/2] Respect LUAJIT_DISABLE_SYSPROF in sysprof tests Sergey Bronnikov via Tarantool-patches
2024-12-06 7:06 ` [Tarantool-patches] [PATCH luajit 1/2] test: small fixes in gh-8594-sysprof-ffunc-crash Sergey Bronnikov via Tarantool-patches
2024-12-06 7:55 ` Sergey Kaplun via Tarantool-patches
2024-12-06 10:16 ` Sergey Bronnikov via Tarantool-patches
2024-12-11 12:24 ` Maxim Kokryashkin via Tarantool-patches
2024-12-06 7:06 ` [Tarantool-patches] [PATCH luajit 2/2] test: skip sysprof tests with LUAJIT_DISABLE_SYSPROF Sergey Bronnikov via Tarantool-patches
2024-12-06 8:08 ` Sergey Kaplun via Tarantool-patches
2024-12-06 10:31 ` Sergey Bronnikov via Tarantool-patches [this message]
2024-12-11 12:25 ` Maxim Kokryashkin via Tarantool-patches
2024-12-17 12:23 ` [Tarantool-patches] [PATCH luajit 0/2] Respect LUAJIT_DISABLE_SYSPROF in sysprof tests Sergey Kaplun via Tarantool-patches
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=Z1LSfI1ZFtlUDdEs@pony \
--to=tarantool-patches@dev.tarantool.org \
--cc=estetus@gmail.com \
--cc=skaplun@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH luajit 2/2] test: skip sysprof tests with LUAJIT_DISABLE_SYSPROF' \
/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