* [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
@ 2023-05-18 20:44 ` Sergey Kaplun via Tarantool-patches
2023-05-19 11:23 ` Maxim Kokryashkin via Tarantool-patches
2023-05-22 11:03 ` Sergey Bronnikov via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests Sergey Kaplun via Tarantool-patches
` (5 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-18 20:44 UTC (permalink / raw)
To: Igor Munkin, Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
When we set `LUA_TEST_ENV_MORE` variable to be used in the additional
env command for run testing if `"` is used to wrap the `LD_LIBRARY_PATH`
value the content of this environment variable is literally
`"/abs/path1:/abs/path2:...:"`. So, the first entry is treated as the
relative path starting with `"`. In that case if we need the library to
be loaded via FFI for this particular test, that loading fails with the
error "cannot open shared object file", since the path to it is
incorrect.
This patch removes `"` wrapping for the aforementioned variables.
---
test/tarantool-tests/CMakeLists.txt | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index a428d009..38d6ae49 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -102,6 +102,11 @@ endif()
# loaded modules on MacOS instead of shared libraries as it is
# done on Linux and BSD, another environment variable should be
# used to guide <ffi.load> while searching the extension.
+# XXX: Be noticed that we shouldn't use `"` here to wrap
+# the variable's content. If we do this, the variable value will
+# contain `"` at the beginning and the end, so this `"` at the
+# beginning will be treated as the directory for the first entry
+# (the last subdirectory added).
if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
# XXX: Apple tries their best to "protect their users from
# malware". As a result SIP (see the link[1] below) has been
@@ -122,9 +127,9 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
#
# [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
- list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
+ list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH=${LD_LIBRARY_PATH})
else()
- list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
+ list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
endif()
# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
--
2.34.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables Sergey Kaplun via Tarantool-patches
@ 2023-05-19 11:23 ` Maxim Kokryashkin via Tarantool-patches
2023-05-22 11:03 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 0 replies; 29+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-19 11:23 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]
Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
>Четверг, 18 мая 2023, 23:49 +03:00 от Sergey Kaplun <skaplun@tarantool.org>:
>
>When we set `LUA_TEST_ENV_MORE` variable to be used in the additional
>env command for run testing if `"` is used to wrap the `LD_LIBRARY_PATH`
>value the content of this environment variable is literally
>`"/abs/path1:/abs/path2:...:"`. So, the first entry is treated as the
>relative path starting with `"`. In that case if we need the library to
>be loaded via FFI for this particular test, that loading fails with the
>error "cannot open shared object file", since the path to it is
>incorrect.
>
>This patch removes `"` wrapping for the aforementioned variables.
>---
> test/tarantool-tests/CMakeLists.txt | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>index a428d009..38d6ae49 100644
>--- a/test/tarantool-tests/CMakeLists.txt
>+++ b/test/tarantool-tests/CMakeLists.txt
>@@ -102,6 +102,11 @@ endif()
> # loaded modules on MacOS instead of shared libraries as it is
> # done on Linux and BSD, another environment variable should be
> # used to guide <ffi.load> while searching the extension.
>+# XXX: Be noticed that we shouldn't use `"` here to wrap
>+# the variable's content. If we do this, the variable value will
>+# contain `"` at the beginning and the end, so this `"` at the
>+# beginning will be treated as the directory for the first entry
>+# (the last subdirectory added).
> if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> # XXX: Apple tries their best to "protect their users from
> # malware". As a result SIP (see the link[1] below) has been
>@@ -122,9 +127,9 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> #
> # [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
>- list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
>+ list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH=${LD_LIBRARY_PATH})
> else()
>- list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
>+ list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
> endif()
>
> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
>--
>2.34.1
[-- Attachment #2: Type: text/html, Size: 3342 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables Sergey Kaplun via Tarantool-patches
2023-05-19 11:23 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-22 11:03 ` Sergey Bronnikov via Tarantool-patches
2023-05-23 6:47 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 29+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-05-22 11:03 UTC (permalink / raw)
To: Sergey Kaplun, Igor Munkin, Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the patch. See one comment below.
On 5/18/23 23:44, Sergey Kaplun wrote:
> When we set `LUA_TEST_ENV_MORE` variable to be used in the additional
> env command for run testing if `"` is used to wrap the `LD_LIBRARY_PATH`
> value the content of this environment variable is literally
> `"/abs/path1:/abs/path2:...:"`. So, the first entry is treated as the
> relative path starting with `"`. In that case if we need the library to
> be loaded via FFI for this particular test, that loading fails with the
> error "cannot open shared object file", since the path to it is
> incorrect.
>
> This patch removes `"` wrapping for the aforementioned variables.
> ---
> test/tarantool-tests/CMakeLists.txt | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index a428d009..38d6ae49 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -102,6 +102,11 @@ endif()
> # loaded modules on MacOS instead of shared libraries as it is
> # done on Linux and BSD, another environment variable should be
> # used to guide <ffi.load> while searching the extension.
> +# XXX: Be noticed that we shouldn't use `"` here to wrap
> +# the variable's content. If we do this, the variable value will
> +# contain `"` at the beginning and the end, so this `"` at the
> +# beginning will be treated as the directory for the first entry
> +# (the last subdirectory added).
> if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> # XXX: Apple tries their best to "protect their users from
> # malware". As a result SIP (see the link[1] below) has been
> @@ -122,9 +127,9 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> #
> # [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
> - list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> + list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH=${LD_LIBRARY_PATH})
> else()
> - list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> + list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
LUA_TEST_ENV_MORE then will be passed to a shell for execution.
I suspect that command line execution will be broken when env variable
will contain non-escaped whitespaces.
It should be quoted or whitespaces should be escaped. So I propose to
escape whitespaces with backward slashes, see [1].
1.
https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#how-can-i-get-quoting-and-escapes-to-work-properly
> endif()
>
> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables
2023-05-22 11:03 ` Sergey Bronnikov via Tarantool-patches
@ 2023-05-23 6:47 ` Sergey Kaplun via Tarantool-patches
2023-05-29 14:37 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-23 6:47 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the review!
On 22.05.23, Sergey Bronnikov wrote:
> Hi, Sergey!
>
> Thanks for the patch. See one comment below.
>
> On 5/18/23 23:44, Sergey Kaplun wrote:
> > When we set `LUA_TEST_ENV_MORE` variable to be used in the additional
> > env command for run testing if `"` is used to wrap the `LD_LIBRARY_PATH`
> > value the content of this environment variable is literally
> > `"/abs/path1:/abs/path2:...:"`. So, the first entry is treated as the
> > relative path starting with `"`. In that case if we need the library to
> > be loaded via FFI for this particular test, that loading fails with the
> > error "cannot open shared object file", since the path to it is
> > incorrect.
> >
> > This patch removes `"` wrapping for the aforementioned variables.
> > ---
> > test/tarantool-tests/CMakeLists.txt | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> > index a428d009..38d6ae49 100644
> > --- a/test/tarantool-tests/CMakeLists.txt
> > +++ b/test/tarantool-tests/CMakeLists.txt
> > @@ -102,6 +102,11 @@ endif()
> > # loaded modules on MacOS instead of shared libraries as it is
> > # done on Linux and BSD, another environment variable should be
> > # used to guide <ffi.load> while searching the extension.
> > +# XXX: Be noticed that we shouldn't use `"` here to wrap
> > +# the variable's content. If we do this, the variable value will
> > +# contain `"` at the beginning and the end, so this `"` at the
> > +# beginning will be treated as the directory for the first entry
> > +# (the last subdirectory added).
> > if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> > # XXX: Apple tries their best to "protect their users from
> > # malware". As a result SIP (see the link[1] below) has been
> > @@ -122,9 +127,9 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> > #
> > # [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
> > - list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> > + list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH=${LD_LIBRARY_PATH})
> > else()
> > - list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> > + list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
>
> LUA_TEST_ENV_MORE then will be passed to a shell for execution.
>
> I suspect that command line execution will be broken when env variable
> will contain non-escaped whitespaces.
>
> It should be quoted or whitespaces should be escaped. So I propose to
> escape whitespaces with backward slashes, see [1].
Yes, but we have some other problems with whitespaces already:
| Could not execute (/home/burii/reviews/luajit/dir\ space/src/luajit -e dofile[[/home/burii/reviews/luajit/dir;space/test/luajit-test-init.lua]] /home/burii/rev
| iews/luajit/dir): open3: exec of /home/burii/reviews/luajit/dir\ space/src/luajit -e dofile[[/home/burii/reviews/luajit/dir;space/test/luajit-test-init.lua]] /
| home/burii/reviews/luajit/dir failed: No such file or directory at /usr/lib64/perl5/5.34/TAP/Parser/Iterator/Process.pm line 165.
I suppose we can cover all such cases in the separate patchset.
>
>
> 1.
> https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#how-can-i-get-quoting-and-escapes-to-work-properly
>
>
> > endif()
> >
> > # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables
2023-05-23 6:47 ` Sergey Kaplun via Tarantool-patches
@ 2023-05-29 14:37 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 0 replies; 29+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-05-29 14:37 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
This patch is LGTM.
On 5/23/23 09:47, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> On 22.05.23, Sergey Bronnikov wrote:
>> Hi, Sergey!
>>
>> Thanks for the patch. See one comment below.
>>
>> On 5/18/23 23:44, Sergey Kaplun wrote:
>>> When we set `LUA_TEST_ENV_MORE` variable to be used in the additional
>>> env command for run testing if `"` is used to wrap the `LD_LIBRARY_PATH`
>>> value the content of this environment variable is literally
>>> `"/abs/path1:/abs/path2:...:"`. So, the first entry is treated as the
>>> relative path starting with `"`. In that case if we need the library to
>>> be loaded via FFI for this particular test, that loading fails with the
>>> error "cannot open shared object file", since the path to it is
>>> incorrect.
>>>
>>> This patch removes `"` wrapping for the aforementioned variables.
>>> ---
>>> test/tarantool-tests/CMakeLists.txt | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>>> index a428d009..38d6ae49 100644
>>> --- a/test/tarantool-tests/CMakeLists.txt
>>> +++ b/test/tarantool-tests/CMakeLists.txt
>>> @@ -102,6 +102,11 @@ endif()
>>> # loaded modules on MacOS instead of shared libraries as it is
>>> # done on Linux and BSD, another environment variable should be
>>> # used to guide <ffi.load> while searching the extension.
>>> +# XXX: Be noticed that we shouldn't use `"` here to wrap
>>> +# the variable's content. If we do this, the variable value will
>>> +# contain `"` at the beginning and the end, so this `"` at the
>>> +# beginning will be treated as the directory for the first entry
>>> +# (the last subdirectory added).
>>> if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
>>> # XXX: Apple tries their best to "protect their users from
>>> # malware". As a result SIP (see the link[1] below) has been
>>> @@ -122,9 +127,9 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
>>> #
>>> # [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
>>> - list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
>>> + list(APPEND LUA_TEST_ENV_MORE DYLD_LIBRARY_PATH=${LD_LIBRARY_PATH})
>>> else()
>>> - list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
>>> + list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
>> LUA_TEST_ENV_MORE then will be passed to a shell for execution.
>>
>> I suspect that command line execution will be broken when env variable
>> will contain non-escaped whitespaces.
>>
>> It should be quoted or whitespaces should be escaped. So I propose to
>> escape whitespaces with backward slashes, see [1].
> Yes, but we have some other problems with whitespaces already:
>
> | Could not execute (/home/burii/reviews/luajit/dir\ space/src/luajit -e dofile[[/home/burii/reviews/luajit/dir;space/test/luajit-test-init.lua]] /home/burii/rev
> | iews/luajit/dir): open3: exec of /home/burii/reviews/luajit/dir\ space/src/luajit -e dofile[[/home/burii/reviews/luajit/dir;space/test/luajit-test-init.lua]] /
> | home/burii/reviews/luajit/dir failed: No such file or directory at /usr/lib64/perl5/5.34/TAP/Parser/Iterator/Process.pm line 165.
>
> I suppose we can cover all such cases in the separate patchset.
>
>>
>> 1.
>> https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#how-can-i-get-quoting-and-escapes-to-work-properly
>>
>>
>>> endif()
>>>
>>> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables Sergey Kaplun via Tarantool-patches
@ 2023-05-18 20:44 ` Sergey Kaplun via Tarantool-patches
2023-05-19 11:46 ` Maxim Kokryashkin via Tarantool-patches
2023-05-22 12:33 ` Sergey Bronnikov via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper " Sergey Kaplun via Tarantool-patches
` (4 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-18 20:44 UTC (permalink / raw)
To: Igor Munkin, Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
We need an instrument to write tests in plain C for LuaJIT, to be able:
* easily test LuaC API
* test patches without usage of plain Lua
* write unit tests
* startup LuaJIT with custom memory allocator, to test some GC issues
* maybe, in future, use custom hashing function to test a behavior
of LuaJIT tables
and so on.
The <test.c> module serves to achieve these goals without too fancy
features.
It's functionality inspired by cmoka API [1], but only TAP14 [2]
protocol is supported (Version of TAP set to 13 to be compatible with
old TAP13 harnesses).
The group of unit tests is declared like the following:
| void *t_state = NULL;
| const struct test_unit tgroup[] = {
| test_unit_new(test_base),
| test_unit_new(test_subtest),
| };
| return test_run_group(tgroup, t_state);
`test_run_group()` runs the whole group of tests, returns
`TEST_EXIT_SUCCESS` or `TEST_EXIT_FAILURE`.
If a similar group is declared inside unit test, this group will be
considered as a subtest.
This library provides an API similar to glibc (3) `assert()` to use
inside unit tests. `assert_[true,false]()` are useful for condition
checks and `assert_{type}_[not_,]_equal()` are useful for value
comparisons. If some assertion fails diagnostic is set, all test
considered as failing and finished via `longjmp()`, so these assertions
can be used inside custom subroutines.
Also, this module provides ability to skip one test or all tests, mark
test as todo, bail out all tests. Just use `return skip()`, `skip_all()`
or `todo()` for early return. They should be used only in the test body
to make skipping clear. `skip_all()` may be used both for the parent
test and for a subtest. `bail_out()` prints an error message and exits
the process.
As a part of this commit, tarantool-c-tests directory is created with
the corresponding CMakeLists.txt file to build this test library.
Tests to be rewritten in C with this library in the next commit and
placed as unit tests are:
* misclib-getmetrics-capi.test.lua
* misclib-sysprof-capi.test.lua
For now the tarantool-c-tests target just build the test library without
new tests to run.
[1]: https://github.com/clibs/cmocka
[2]: https://testanything.org/tap-version-14-specification.html
Part of tarantool/tarantool#7900
---
test/CMakeLists.txt | 2 +
test/tarantool-c-tests/CMakeLists.txt | 41 +++++
test/tarantool-c-tests/test.c | 251 ++++++++++++++++++++++++++
test/tarantool-c-tests/test.h | 217 ++++++++++++++++++++++
4 files changed, 511 insertions(+)
create mode 100644 test/tarantool-c-tests/CMakeLists.txt
create mode 100644 test/tarantool-c-tests/test.c
create mode 100644 test/tarantool-c-tests/test.h
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index a8262b12..47296a22 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -48,12 +48,14 @@ separate_arguments(LUAJIT_TEST_COMMAND)
add_subdirectory(LuaJIT-tests)
add_subdirectory(PUC-Rio-Lua-5.1-tests)
add_subdirectory(lua-Harness-tests)
+add_subdirectory(tarantool-c-tests)
add_subdirectory(tarantool-tests)
add_custom_target(${PROJECT_NAME}-test DEPENDS
LuaJIT-tests
PUC-Rio-Lua-5.1-tests
lua-Harness-tests
+ tarantool-c-tests
tarantool-tests
)
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
new file mode 100644
index 00000000..c6b7cd30
--- /dev/null
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -0,0 +1,41 @@
+find_program(PROVE prove)
+if(NOT PROVE)
+ message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
+ return()
+endif()
+
+set(C_TEST_SUFFIX .c_test)
+set(C_TEST_FLAGS --failures --shuffle)
+
+if(CMAKE_VERBOSE_MAKEFILE)
+ list(APPEND C_TEST_FLAGS --verbose)
+endif()
+
+# Build libtest.
+
+set(TEST_LIB_NAME "test")
+add_library(libtest STATIC EXCLUDE_FROM_ALL ${CMAKE_CURRENT_SOURCE_DIR}/test.c)
+target_include_directories(libtest PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
+set_target_properties(libtest PROPERTIES
+ COMPILE_FLAGS "-Wall -Wextra"
+ OUTPUT_NAME "${TEST_LIB_NAME}"
+ LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+)
+
+# XXX: For now, just build libtest. The tests to be depended on
+# will be added in the next commit.
+add_custom_target(tarantool-c-tests
+ DEPENDS libluajit libtest
+)
+
+# XXX: For now, run 0 tests. Just verify that libtest was built.
+add_custom_command(TARGET tarantool-c-tests
+ COMMENT "Running Tarantool C tests"
+ COMMAND
+ ${PROVE}
+ ${CMAKE_CURRENT_BINARY_DIR}
+ --ext ${C_TEST_SUFFIX}
+ --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
+ ${C_TEST_FLAGS}
+ WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+)
diff --git a/test/tarantool-c-tests/test.c b/test/tarantool-c-tests/test.c
new file mode 100644
index 00000000..74cba3a3
--- /dev/null
+++ b/test/tarantool-c-tests/test.c
@@ -0,0 +1,251 @@
+#include "test.h"
+
+/*
+ * Test module, based on TAP 14 specification [1].
+ * [1]: https://testanything.org/tap-version-14-specification.html
+ */
+
+/* Need for `PATH_MAX` in diagnostic definition. */
+#include <limits.h>
+#include <setjmp.h>
+#include <stdarg.h>
+/* Need for `strchr()` in diagnostic parsing. */
+#include <string.h>
+
+/*
+ * Test level: 0 for the parent test, >0 for any subtests.
+ */
+static int level = -1;
+
+/*
+ * The last diagnostic data to be used in the YAML Diagnostic
+ * block.
+ *
+ * Contains filename, line number and failed expression or assert
+ * name and "got" and "expected" fields. All entries are separated
+ * by \n.
+ * The longest field is filename here, so PATH_MAX * 3 as
+ * the diagnostic string length should be enough.
+ *
+ * The first \0 means the end of diagnostic data.
+ *
+ * As far as `strchr()` searches until \0, all previous entries
+ * are suppressed by the last one. If the first byte is \0 --
+ * diagnostic is empty.
+ */
+#define TEST_DIAG_DATA_MAX (PATH_MAX * 3)
+char test_diag_buf[TEST_DIAG_DATA_MAX] = {0};
+
+const char *skip_reason = NULL;
+const char *todo_reason = NULL;
+
+/* Indent for the TAP. 4 spaces is default for subtest. */
+static void indent(void)
+{
+ int i;
+ for (i = 0; i < level; i++)
+ printf(" ");
+}
+
+void test_message(const char *fmt, ...)
+{
+ va_list ap;
+ indent();
+ va_start(ap, fmt);
+ vprintf(fmt, ap);
+ printf("\n");
+ va_end(ap);
+}
+
+static void test_print_tap_version(void)
+{
+ /*
+ * Since several TAP13 parsers in popular usage treat
+ * a repeated Version declaration as an error, even if the
+ * Version is indented, Subtests _should not_ include a
+ * Version, if TAP13 Harness compatibility is
+ * desirable [1].
+ */
+ if (level == 0)
+ test_message("TAP version %d", TAP_VERSION);
+}
+
+static void test_start_comment(const char *t_name)
+{
+ if (level > -1)
+ /*
+ * Inform about starting subtest, easier for
+ * humans to read.
+ * Subtest with a name must be terminated by a
+ * Test Point with a matching Description [1].
+ */
+ test_comment("Subtest: %s", t_name);
+}
+
+void _test_print_skip_all(const char *group_name, const char *reason)
+{
+ test_start_comment(group_name);
+ /*
+ * XXX: This test isn't started yet, so set indent level
+ * manually.
+ */
+ level++;
+ test_print_tap_version();
+ /*
+ * XXX: `SKIP_DIRECTIVE` is not necessary here according
+ * to the TAP14 specification [1], but some harnesses may
+ * fail to parse the output without it.
+ */
+ test_message("1..0" SKIP_DIRECTIVE "%s", reason);
+ level--;
+}
+
+/* Just inform TAP parser how many tests we want to run. */
+static void test_plan(size_t planned)
+{
+ test_message("1..%lu", planned);
+}
+
+/* Human-readable output how many tests/subtests are failed. */
+static void test_finish(size_t planned, size_t failed)
+{
+ const char *t_type = level == 0 ? "tests" : "subtests";
+ if (failed > 0)
+ test_comment("Failed %lu %s out of %lu",
+ failed, t_type, planned);
+ fflush(stdout);
+}
+
+void test_set_skip_reason(const char *reason)
+{
+ skip_reason = reason;
+}
+
+void test_set_todo_reason(const char *reason)
+{
+ todo_reason = reason;
+}
+
+void test_save_diag_data(const char *fmt, ...)
+{
+ va_list ap;
+ va_start(ap, fmt);
+ vsnprintf(test_diag_buf, TEST_DIAG_DATA_MAX, fmt, ap);
+ va_end(ap);
+}
+
+static void test_clear_diag_data(void)
+{
+ /*
+ * Limit buffer with zero byte to show that there is no
+ * any entry.
+ */
+ test_diag_buf[0] = '\0';
+}
+
+static int test_diagnostic_is_set(void)
+{
+ return test_diag_buf[0] != '\0';
+}
+
+/*
+ * Parse the last diagnostic data entry and print it in YAML
+ * format with the corresponding additional half-indent in TAP
+ * (2 spaces).
+ * Clear diagnostic message to be sure that it's printed once.
+ * XXX: \n separators are changed to \0 during parsing and
+ * printing output for convenience in usage.
+ */
+static void test_diagnostic(void)
+{
+ test_message(" ---");
+ char *ent = test_diag_buf;
+ char *ent_end = NULL;
+ while ((ent_end = strchr(ent, '\n')) != NULL) {
+ char *next_ent = ent_end + 1;
+ /*
+ * Limit string with with the zero byte for
+ * formatted output. Anyway, don't need this \n
+ * anymore.
+ */
+ *ent_end = '\0';
+ test_message(" %s", ent);
+ ent = next_ent;
+ }
+ test_message(" ...");
+ test_clear_diag_data();
+}
+
+static jmp_buf test_run_env;
+
+TEST_NORET void _test_exit(int status)
+{
+ longjmp(test_run_env, status);
+}
+
+static int test_run(const struct test_unit *test, size_t test_number,
+ void *test_state)
+{
+ int status = TEST_EXIT_SUCCESS;
+ /*
+ * Run unit test. Diagnostic in case of failure setup by
+ * helpers assert macros defined in the header.
+ */
+ int jmp_status;
+ if ((jmp_status = setjmp(test_run_env)) == 0) {
+ if (test->f(test_state) != TEST_EXIT_SUCCESS)
+ status = TEST_EXIT_FAILURE;
+ } else {
+ status = jmp_status - TEST_JMP_STATUS_SHIFT;
+ }
+ const char *result = status == TEST_EXIT_SUCCESS ? "ok" : "not ok";
+
+ /*
+ * Format suffix of the test message for SKIP or TODO
+ * directives.
+ */
+#define SUFFIX_SZ 1024
+ char suffix[SUFFIX_SZ] = {0};
+ if (skip_reason) {
+ snprintf(suffix, SUFFIX_SZ, SKIP_DIRECTIVE "%s", skip_reason);
+ skip_reason = NULL;
+ } else if (todo_reason) {
+ /* Prevent count this test as failed. */
+ status = TEST_EXIT_SUCCESS;
+ snprintf(suffix, SUFFIX_SZ, TODO_DIRECTIVE "%s", todo_reason);
+ todo_reason = NULL;
+ }
+#undef SUFFIX_SZ
+
+ test_message("%s %lu - %s%s", result, test_number, test->name,
+ suffix);
+
+ if (status && test_diagnostic_is_set())
+ test_diagnostic();
+ return status;
+}
+
+int _test_run_group(const char *group_name, const struct test_unit tests[],
+ size_t n_tests, void *test_state)
+{
+ test_start_comment(group_name);
+
+ level++;
+ test_print_tap_version();
+
+ test_plan(n_tests);
+
+ size_t n_failed = 0;
+
+ size_t i;
+ for (i = 0; i < n_tests; i++) {
+ size_t test_number = i + 1;
+ /* Return 1 on failure, 0 on success. */
+ n_failed += test_run(&tests[i], test_number, test_state);
+ }
+
+ test_finish(n_tests, n_failed);
+
+ level--;
+ return n_failed > 0 ? TEST_EXIT_FAILURE : TEST_EXIT_SUCCESS;
+}
diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
new file mode 100644
index 00000000..28df9daf
--- /dev/null
+++ b/test/tarantool-c-tests/test.h
@@ -0,0 +1,217 @@
+#ifndef TEST_H
+#define TEST_H
+
+#include <stdio.h>
+#include <stdlib.h>
+
+/*
+ * Test module, based on TAP 14 specification [1].
+ * [1]: https://testanything.org/tap-version-14-specification.html
+ * Version 13 is set for better compatibility on old machines.
+ *
+ * TODO:
+ * * Helpers assert macros:
+ * - assert_uint_equal if needed
+ * - assert_uint_not_equal if needed
+ * - assert_str_equal if needed
+ * - assert_str_not_equal if needed
+ * - assert_memory_equal if needed
+ * - assert_memory_not_equal if needed
+ * * Pragmas.
+ */
+
+#define TAP_VERSION 13
+
+#define TEST_EXIT_SUCCESS 0
+#define TEST_EXIT_FAILURE 1
+
+#define TEST_JMP_STATUS_SHIFT 2
+#define TEST_LJMP_EXIT_SUCCESS (TEST_EXIT_SUCCESS + TEST_JMP_STATUS_SHIFT)
+#define TEST_LJMP_EXIT_FAILURE (TEST_EXIT_FAILURE + TEST_JMP_STATUS_SHIFT)
+
+#define TEST_NORET __attribute__((noreturn))
+
+typedef int (*test_func)(void *test_state);
+struct test_unit {
+ const char *name;
+ test_func f;
+};
+
+/* API declaration. */
+
+/*
+ * Print formatted message with the corresponding indent.
+ * If you want to leave a comment, use `test_comment()` instead.
+ */
+void test_message(const char *fmt, ...);
+
+/* Need for `skip_all()`, please, don't use it. */
+void _test_print_skip_all(const char *group_name, const char *reason);
+/* End test via `longjmp()`, please, don't use it. */
+TEST_NORET void _test_exit(int status);
+
+void test_set_skip_reason(const char *reason);
+void test_set_todo_reason(const char *reason);
+/*
+ * Save formatted diagnostic data. Each entry separated with \n.
+ */
+void test_save_diag_data(const char *fmt, ...);
+
+/* Internal, it is better to use `test_run_group()` instead. */
+int _test_run_group(const char *group_name, const struct test_unit tests[],
+ size_t n_tests, void *test_state);
+
+/* Initialize `test_unit` structure. */
+#define test_unit_new(f) {#f, f}
+
+#define lengthof(arr) (sizeof(arr) / sizeof((arr)[0]))
+
+/*
+ * __func__ is the name for a test group, "main" for the parent
+ * test.
+ */
+#define test_run_group(t_arr, t_state) \
+ _test_run_group(__func__, t_arr, lengthof(t_arr), t_state)
+
+#define SKIP_DIRECTIVE " # SKIP "
+#define TODO_DIRECTIVE " # TODO "
+
+#define skip_all(reason) ({ \
+ _test_print_skip_all(__func__, reason); \
+ TEST_EXIT_SUCCESS; \
+})
+
+static inline int skip(const char *reason)
+{
+ test_set_skip_reason(reason);
+ return TEST_EXIT_SUCCESS;
+}
+
+static inline int todo(const char *reason)
+{
+ test_set_todo_reason(reason);
+ return TEST_EXIT_FAILURE;
+}
+
+#define bail_out(reason) do { \
+ /* \
+ * For backwards compatibility with TAP13 Harnesses, \
+ * Producers _should_ emit a "Bail out!" line at the root \
+ * indentation level whenever a Subtest bails out [1]. \
+ */ \
+ printf("Bail out! %s\n", reason); \
+ exit(TEST_EXIT_FAILURE); \
+} while (0)
+
+/* `fmt` should always be a format string here. */
+#define test_comment(fmt, ...) test_message("# " fmt, __VA_ARGS__)
+
+/*
+ * This is a set of useful assert macros like the standard C
+ * libary's assert(3) macro.
+ *
+ * On an assertion failure an assert macro will save the
+ * diagnostic to the special buffer, to be reported via YAML
+ * Diagnostic block and finish a test function with
+ * `return TEST_EXIT_FAILURE`.
+ *
+ * Due to limitations of the C language `assert_true()` and
+ * `assert_false()` macros can only display the expression that
+ * caused the assertion failure. Type specific assert macros,
+ * `assert_{type}_equal()` and `assert_{type}_not_equal()`, save
+ * the data that caused the assertion failure which increases data
+ * visibility aiding debugging of failing test cases.
+ */
+
+#define LOCATION_FMT "location:\t%s:%d\n"
+#define ASSERT_NAME_FMT(name) "failed_assertion:\t" #name "\n"
+#define ASSERT_EQUAL_FMT(name_type, type_fmt) \
+ LOCATION_FMT \
+ ASSERT_NAME_FMT(assert_ ## name_type ## _equal) \
+ "got: " type_fmt "\n" \
+ "expected: " type_fmt "\n"
+
+#define ASSERT_NOT_EQUAL_FMT(type_fmt) \
+ LOCATION_FMT \
+ ASSERT_NAME_FMT(assert_ ## name_type ## _not_equal) \
+ "got: " type_fmt "\n" \
+ "unexpected: " type_fmt "\n"
+
+#define assert_true(cond) do { \
+ if (!(cond)) { \
+ test_save_diag_data(LOCATION_FMT \
+ "condition_failed:\t'" #cond "'\n", \
+ __FILE__, __LINE__); \
+ _test_exit(TEST_LJMP_EXIT_FAILURE); \
+ } \
+} while (0)
+
+#define assert_false(cond) assert_true(!(cond))
+
+#define assert_general(cond, fmt, ...) do { \
+ if (!(cond)) { \
+ test_save_diag_data(fmt, __VA_ARGS__); \
+ _test_exit(TEST_LJMP_EXIT_FAILURE); \
+ } \
+} while (0)
+
+#define assert_ptr_equal(got, expected) do { \
+ assert_general((got) == (expected), \
+ ASSERT_EQUAL_FMT(ptr, "%p"), \
+ __FILE__, __LINE__, (got), (expected) \
+ ); \
+} while (0)
+
+#define assert_ptr_not_equal(got, unexpected) do { \
+ assert_general((got) != (unexpected), \
+ ASSERT_NOT_EQUAL_FMT(ptr, "%p"), \
+ __FILE__, __LINE__, (got), (unexpected) \
+ ); \
+} while (0)
+
+
+#define assert_int_equal(got, expected) do { \
+ assert_general((got) == (expected), \
+ ASSERT_EQUAL_FMT(int, "%d"), \
+ __FILE__, __LINE__, (got), (expected) \
+ ); \
+} while (0)
+
+#define assert_int_not_equal(got, unexpected) do { \
+ assert_general((got) != (unexpected), \
+ ASSERT_NOT_EQUAL_FMT(int, "%d"), \
+ __FILE__, __LINE__, (got), (unexpected) \
+ ); \
+} while (0)
+
+#define assert_sizet_equal(got, expected) do { \
+ assert_general((got) == (expected), \
+ ASSERT_EQUAL_FMT(sizet, "%lu"), \
+ __FILE__, __LINE__, (got), (expected) \
+ ); \
+} while (0)
+
+#define assert_sizet_not_equal(got, unexpected) do { \
+ assert_general((got) != (unexpected), \
+ ASSERT_NOT_EQUAL_FMT(sizet, "%lu"), \
+ __FILE__, __LINE__, (got), (unexpected) \
+ ); \
+} while (0)
+
+/* Check that doubles are __exactly__ the same. */
+#define assert_double_equal(got, expected) do { \
+ assert_general((got) == (expected), \
+ ASSERT_EQUAL_FMT(double, "%lf"), \
+ __FILE__, __LINE__, (got), (expected) \
+ ); \
+} while (0)
+
+/* Check that doubles are not __exactly__ the same. */
+#define assert_double_not_equal(got, unexpected) do { \
+ assert_general((got) != (unexpected), \
+ ASSERT_NOT_EQUAL_FMT(double, "%lf"), \
+ __FILE__, __LINE__, (got), (unexpected) \
+ ); \
+} while (0)
+
+#endif /* TEST_H */
--
2.34.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests Sergey Kaplun via Tarantool-patches
@ 2023-05-19 11:46 ` Maxim Kokryashkin via Tarantool-patches
2023-05-22 12:33 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 0 replies; 29+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-19 11:46 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 2993 bytes --]
Hi, Sergey!
Thanks for the patch!
LGTM.
>
>diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
>new file mode 100644
>index 00000000..28df9daf
>--- /dev/null
>+++ b/test/tarantool-c-tests/test.h
>@@ -0,0 +1,217 @@
Looks so much better now!
>+#define LOCATION_FMT "location:\t%s:%d\n"
>+#define ASSERT_NAME_FMT(name) "failed_assertion:\t" #name "\n"
>+#define ASSERT_EQUAL_FMT(name_type, type_fmt) \
>+ LOCATION_FMT \
>+ ASSERT_NAME_FMT(assert_ ## name_type ## _equal) \
>+ "got: " type_fmt "\n" \
>+ "expected: " type_fmt "\n"
>+
>+#define ASSERT_NOT_EQUAL_FMT(type_fmt) \
>+ LOCATION_FMT \
>+ ASSERT_NAME_FMT(assert_ ## name_type ## _not_equal) \
>+ "got: " type_fmt "\n" \
>+ "unexpected: " type_fmt "\n"
>+
>+#define assert_true(cond) do { \
>+ if (!(cond)) { \
>+ test_save_diag_data(LOCATION_FMT \
>+ "condition_failed:\t'" #cond "'\n", \
>+ __FILE__, __LINE__); \
>+ _test_exit(TEST_LJMP_EXIT_FAILURE); \
>+ } \
>+} while (0)
>+
>+#define assert_false(cond) assert_true(!(cond))
>+
>+#define assert_general(cond, fmt, ...) do { \
>+ if (!(cond)) { \
>+ test_save_diag_data(fmt, __VA_ARGS__); \
>+ _test_exit(TEST_LJMP_EXIT_FAILURE); \
>+ } \
>+} while (0)
>+
>+#define assert_ptr_equal(got, expected) do { \
>+ assert_general((got) == (expected), \
>+ ASSERT_EQUAL_FMT(ptr, "%p"), \
>+ __FILE__, __LINE__, (got), (expected) \
>+ ); \
>+} while (0)
>+
>+#define assert_ptr_not_equal(got, unexpected) do { \
>+ assert_general((got) != (unexpected), \
>+ ASSERT_NOT_EQUAL_FMT(ptr, "%p"), \
>+ __FILE__, __LINE__, (got), (unexpected) \
>+ ); \
>+} while (0)
>+
>+
>+#define assert_int_equal(got, expected) do { \
>+ assert_general((got) == (expected), \
>+ ASSERT_EQUAL_FMT(int, "%d"), \
>+ __FILE__, __LINE__, (got), (expected) \
>+ ); \
>+} while (0)
>+
>+#define assert_int_not_equal(got, unexpected) do { \
>+ assert_general((got) != (unexpected), \
>+ ASSERT_NOT_EQUAL_FMT(int, "%d"), \
>+ __FILE__, __LINE__, (got), (unexpected) \
>+ ); \
>+} while (0)
>+
>+#define assert_sizet_equal(got, expected) do { \
>+ assert_general((got) == (expected), \
>+ ASSERT_EQUAL_FMT(sizet, "%lu"), \
>+ __FILE__, __LINE__, (got), (expected) \
>+ ); \
>+} while (0)
>+
>+#define assert_sizet_not_equal(got, unexpected) do { \
>+ assert_general((got) != (unexpected), \
>+ ASSERT_NOT_EQUAL_FMT(sizet, "%lu"), \
>+ __FILE__, __LINE__, (got), (unexpected) \
>+ ); \
>+} while (0)
>+
>+/* Check that doubles are __exactly__ the same. */
>+#define assert_double_equal(got, expected) do { \
>+ assert_general((got) == (expected), \
>+ ASSERT_EQUAL_FMT(double, "%lf"), \
>+ __FILE__, __LINE__, (got), (expected) \
>+ ); \
>+} while (0)
>+
>+/* Check that doubles are not __exactly__ the same. */
>+#define assert_double_not_equal(got, unexpected) do { \
>+ assert_general((got) != (unexpected), \
>+ ASSERT_NOT_EQUAL_FMT(double, "%lf"), \
>+ __FILE__, __LINE__, (got), (unexpected) \
>+ ); \
>+} while (0)
>+
>+#endif /* TEST_H */
>--
>2.34.1
--
Best regards,
Maxim Kokryashkin
[-- Attachment #2: Type: text/html, Size: 3521 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests Sergey Kaplun via Tarantool-patches
2023-05-19 11:46 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-22 12:33 ` Sergey Bronnikov via Tarantool-patches
2023-05-24 6:41 ` Sergey Kaplun via Tarantool-patches
1 sibling, 1 reply; 29+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-05-22 12:33 UTC (permalink / raw)
To: Sergey Kaplun, Igor Munkin, Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the patch! Please see my comments inline.
On 5/18/23 23:44, Sergey Kaplun wrote:
> We need an instrument to write tests in plain C for LuaJIT, to be able:
> * easily test LuaC API
> * test patches without usage of plain Lua
> * write unit tests
> * startup LuaJIT with custom memory allocator, to test some GC issues
> * maybe, in future, use custom hashing function to test a behavior
> of LuaJIT tables
> and so on.
Unexpected newline, put this on a previous line.
>
> The <test.c> module serves to achieve these goals without too fancy
> features.
>
> It's functionality inspired by cmoka API [1], but only TAP14 [2]
typo: cmocka
> protocol is supported (Version of TAP set to 13 to be compatible with
> old TAP13 harnesses).
Please add tests for TAP13/TAP14 conformance testing.
It would be unpleasant if proposed library will produce TAP-incompatible
output and it will break parsing in 'prove'.
At least single test for passed testcase "ok", single testcase for
failed "not ok" testcase, one testcase for every directive.
> The group of unit tests is declared like the following:
>
> | void *t_state = NULL;
> | const struct test_unit tgroup[] = {
> | test_unit_new(test_base),
> | test_unit_new(test_subtest),
> | };
> | return test_run_group(tgroup, t_state);
>
> `test_run_group()` runs the whole group of tests, returns
> `TEST_EXIT_SUCCESS` or `TEST_EXIT_FAILURE`.
>
> If a similar group is declared inside unit test, this group will be
> considered as a subtest.
>
> This library provides an API similar to glibc (3) `assert()` to use
> inside unit tests. `assert_[true,false]()` are useful for condition
> checks and `assert_{type}_[not_,]_equal()` are useful for value
> comparisons. If some assertion fails diagnostic is set, all test
> considered as failing and finished via `longjmp()`, so these assertions
> can be used inside custom subroutines.
>
> Also, this module provides ability to skip one test or all tests, mark
> test as todo, bail out all tests. Just use `return skip()`, `skip_all()`
> or `todo()` for early return. They should be used only in the test body
> to make skipping clear. `skip_all()` may be used both for the parent
> test and for a subtest. `bail_out()` prints an error message and exits
> the process.
>
> As a part of this commit, tarantool-c-tests directory is created with
> the corresponding CMakeLists.txt file to build this test library.
> Tests to be rewritten in C with this library in the next commit and
> placed as unit tests are:
> * misclib-getmetrics-capi.test.lua
> * misclib-sysprof-capi.test.lua
>
> For now the tarantool-c-tests target just build the test library without
> new tests to run.
>
> [1]: https://github.com/clibs/cmocka
> [2]: https://testanything.org/tap-version-14-specification.html
>
> Part of tarantool/tarantool#7900
> ---
> test/CMakeLists.txt | 2 +
> test/tarantool-c-tests/CMakeLists.txt | 41 +++++
> test/tarantool-c-tests/test.c | 251 ++++++++++++++++++++++++++
> test/tarantool-c-tests/test.h | 217 ++++++++++++++++++++++
> 4 files changed, 511 insertions(+)
> create mode 100644 test/tarantool-c-tests/CMakeLists.txt
> create mode 100644 test/tarantool-c-tests/test.c
> create mode 100644 test/tarantool-c-tests/test.h
>
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index a8262b12..47296a22 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -48,12 +48,14 @@ separate_arguments(LUAJIT_TEST_COMMAND)
> add_subdirectory(LuaJIT-tests)
> add_subdirectory(PUC-Rio-Lua-5.1-tests)
> add_subdirectory(lua-Harness-tests)
> +add_subdirectory(tarantool-c-tests)
> add_subdirectory(tarantool-tests)
>
> add_custom_target(${PROJECT_NAME}-test DEPENDS
> LuaJIT-tests
> PUC-Rio-Lua-5.1-tests
> lua-Harness-tests
> + tarantool-c-tests
> tarantool-tests
Should we rename tarantool-tests to tarantool-lua-tests in a separate
commit?
> )
>
> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> new file mode 100644
> index 00000000..c6b7cd30
> --- /dev/null
> +++ b/test/tarantool-c-tests/CMakeLists.txt
> @@ -0,0 +1,41 @@
> +find_program(PROVE prove)
> +if(NOT PROVE)
> + message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
> + return()
> +endif()
copy-pasted from test/tarantool-tests/CMakeLists.txt, I would place it
to test/CMakeLists.txt.
At least searching prove (find_program(PROVE prove)).
> +
> +set(C_TEST_SUFFIX .c_test)
Lua tests in test/tarantool-tests have prefix ".test.lua".
Should we use here ".test.c" for consistency?
> +set(C_TEST_FLAGS --failures --shuffle)
> +
> +if(CMAKE_VERBOSE_MAKEFILE)
> + list(APPEND C_TEST_FLAGS --verbose)
> +endif()
> +
> +# Build libtest.
> +
> +set(TEST_LIB_NAME "test")
> +add_library(libtest STATIC EXCLUDE_FROM_ALL ${CMAKE_CURRENT_SOURCE_DIR}/test.c)
> +target_include_directories(libtest PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})
> +set_target_properties(libtest PROPERTIES
> + COMPILE_FLAGS "-Wall -Wextra"
> + OUTPUT_NAME "${TEST_LIB_NAME}"
> + LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
> +)
> +
> +# XXX: For now, just build libtest. The tests to be depended on
> +# will be added in the next commit.
> +add_custom_target(tarantool-c-tests
> + DEPENDS libluajit libtest
> +)
> +
> +# XXX: For now, run 0 tests. Just verify that libtest was built.
> +add_custom_command(TARGET tarantool-c-tests
> + COMMENT "Running Tarantool C tests"
> + COMMAND
> + ${PROVE}
> + ${CMAKE_CURRENT_BINARY_DIR}
> + --ext ${C_TEST_SUFFIX}
> + --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> + ${C_TEST_FLAGS}
> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> +)
> diff --git a/test/tarantool-c-tests/test.c b/test/tarantool-c-tests/test.c
> new file mode 100644
> index 00000000..74cba3a3
> --- /dev/null
> +++ b/test/tarantool-c-tests/test.c
> @@ -0,0 +1,251 @@
> +#include "test.h"
> +
> +/*
> + * Test module, based on TAP 14 specification [1].
> + * [1]: https://testanything.org/tap-version-14-specification.html
> + */
> +
> +/* Need for `PATH_MAX` in diagnostic definition. */
> +#include <limits.h>
> +#include <setjmp.h>
> +#include <stdarg.h>
> +/* Need for `strchr()` in diagnostic parsing. */
> +#include <string.h>
> +
> +/*
> + * Test level: 0 for the parent test, >0 for any subtests.
> + */
> +static int level = -1;
> +
> +/*
> + * The last diagnostic data to be used in the YAML Diagnostic
> + * block.
> + *
> + * Contains filename, line number and failed expression or assert
> + * name and "got" and "expected" fields. All entries are separated
> + * by \n.
> + * The longest field is filename here, so PATH_MAX * 3 as
> + * the diagnostic string length should be enough.
> + *
> + * The first \0 means the end of diagnostic data.
> + *
> + * As far as `strchr()` searches until \0, all previous entries
> + * are suppressed by the last one. If the first byte is \0 --
> + * diagnostic is empty.
> + */
> +#define TEST_DIAG_DATA_MAX (PATH_MAX * 3)
> +char test_diag_buf[TEST_DIAG_DATA_MAX] = {0};
> +
> +const char *skip_reason = NULL;
> +const char *todo_reason = NULL;
> +
> +/* Indent for the TAP. 4 spaces is default for subtest. */
> +static void indent(void)
> +{
> + int i;
> + for (i = 0; i < level; i++)
> + printf(" ");
> +}
> +
> +void test_message(const char *fmt, ...)
> +{
> + va_list ap;
> + indent();
> + va_start(ap, fmt);
> + vprintf(fmt, ap);
> + printf("\n");
> + va_end(ap);
> +}
> +
> +static void test_print_tap_version(void)
> +{
> + /*
> + * Since several TAP13 parsers in popular usage treat
> + * a repeated Version declaration as an error, even if the
> + * Version is indented, Subtests _should not_ include a
> + * Version, if TAP13 Harness compatibility is
> + * desirable [1].
> + */
> + if (level == 0)
> + test_message("TAP version %d", TAP_VERSION);
> +}
> +
> +static void test_start_comment(const char *t_name)
> +{
> + if (level > -1)
> + /*
> + * Inform about starting subtest, easier for
> + * humans to read.
> + * Subtest with a name must be terminated by a
> + * Test Point with a matching Description [1].
> + */
> + test_comment("Subtest: %s", t_name);
> +}
> +
> +void _test_print_skip_all(const char *group_name, const char *reason)
> +{
> + test_start_comment(group_name);
> + /*
> + * XXX: This test isn't started yet, so set indent level
> + * manually.
> + */
> + level++;
> + test_print_tap_version();
> + /*
> + * XXX: `SKIP_DIRECTIVE` is not necessary here according
> + * to the TAP14 specification [1], but some harnesses may
> + * fail to parse the output without it.
> + */
> + test_message("1..0" SKIP_DIRECTIVE "%s", reason);
> + level--;
> +}
> +
> +/* Just inform TAP parser how many tests we want to run. */
> +static void test_plan(size_t planned)
> +{
> + test_message("1..%lu", planned);
> +}
> +
> +/* Human-readable output how many tests/subtests are failed. */
> +static void test_finish(size_t planned, size_t failed)
> +{
> + const char *t_type = level == 0 ? "tests" : "subtests";
> + if (failed > 0)
> + test_comment("Failed %lu %s out of %lu",
> + failed, t_type, planned);
> + fflush(stdout);
> +}
> +
> +void test_set_skip_reason(const char *reason)
> +{
> + skip_reason = reason;
> +}
> +
> +void test_set_todo_reason(const char *reason)
> +{
> + todo_reason = reason;
> +}
> +
> +void test_save_diag_data(const char *fmt, ...)
> +{
> + va_list ap;
> + va_start(ap, fmt);
> + vsnprintf(test_diag_buf, TEST_DIAG_DATA_MAX, fmt, ap);
> + va_end(ap);
> +}
> +
> +static void test_clear_diag_data(void)
> +{
> + /*
> + * Limit buffer with zero byte to show that there is no
> + * any entry.
> + */
> + test_diag_buf[0] = '\0';
> +}
> +
> +static int test_diagnostic_is_set(void)
> +{
> + return test_diag_buf[0] != '\0';
> +}
> +
> +/*
> + * Parse the last diagnostic data entry and print it in YAML
> + * format with the corresponding additional half-indent in TAP
> + * (2 spaces).
> + * Clear diagnostic message to be sure that it's printed once.
> + * XXX: \n separators are changed to \0 during parsing and
> + * printing output for convenience in usage.
> + */
> +static void test_diagnostic(void)
> +{
> + test_message(" ---");
> + char *ent = test_diag_buf;
> + char *ent_end = NULL;
> + while ((ent_end = strchr(ent, '\n')) != NULL) {
> + char *next_ent = ent_end + 1;
> + /*
> + * Limit string with with the zero byte for
> + * formatted output. Anyway, don't need this \n
> + * anymore.
> + */
> + *ent_end = '\0';
> + test_message(" %s", ent);
> + ent = next_ent;
> + }
> + test_message(" ...");
> + test_clear_diag_data();
> +}
> +
> +static jmp_buf test_run_env;
> +
> +TEST_NORET void _test_exit(int status)
> +{
> + longjmp(test_run_env, status);
> +}
> +
> +static int test_run(const struct test_unit *test, size_t test_number,
> + void *test_state)
> +{
> + int status = TEST_EXIT_SUCCESS;
> + /*
> + * Run unit test. Diagnostic in case of failure setup by
> + * helpers assert macros defined in the header.
> + */
> + int jmp_status;
> + if ((jmp_status = setjmp(test_run_env)) == 0) {
> + if (test->f(test_state) != TEST_EXIT_SUCCESS)
> + status = TEST_EXIT_FAILURE;
> + } else {
> + status = jmp_status - TEST_JMP_STATUS_SHIFT;
> + }
> + const char *result = status == TEST_EXIT_SUCCESS ? "ok" : "not ok";
> +
> + /*
> + * Format suffix of the test message for SKIP or TODO
> + * directives.
> + */
> +#define SUFFIX_SZ 1024
> + char suffix[SUFFIX_SZ] = {0};
> + if (skip_reason) {
> + snprintf(suffix, SUFFIX_SZ, SKIP_DIRECTIVE "%s", skip_reason);
> + skip_reason = NULL;
> + } else if (todo_reason) {
> + /* Prevent count this test as failed. */
> + status = TEST_EXIT_SUCCESS;
> + snprintf(suffix, SUFFIX_SZ, TODO_DIRECTIVE "%s", todo_reason);
> + todo_reason = NULL;
> + }
> +#undef SUFFIX_SZ
> +
> + test_message("%s %lu - %s%s", result, test_number, test->name,
> + suffix);
> +
> + if (status && test_diagnostic_is_set())
> + test_diagnostic();
> + return status;
> +}
> +
> +int _test_run_group(const char *group_name, const struct test_unit tests[],
> + size_t n_tests, void *test_state)
> +{
> + test_start_comment(group_name);
> +
> + level++;
> + test_print_tap_version();
> +
> + test_plan(n_tests);
> +
> + size_t n_failed = 0;
> +
> + size_t i;
> + for (i = 0; i < n_tests; i++) {
> + size_t test_number = i + 1;
> + /* Return 1 on failure, 0 on success. */
> + n_failed += test_run(&tests[i], test_number, test_state);
> + }
> +
> + test_finish(n_tests, n_failed);
> +
> + level--;
> + return n_failed > 0 ? TEST_EXIT_FAILURE : TEST_EXIT_SUCCESS;
> +}
> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
> new file mode 100644
> index 00000000..28df9daf
> --- /dev/null
> +++ b/test/tarantool-c-tests/test.h
> @@ -0,0 +1,217 @@
> +#ifndef TEST_H
> +#define TEST_H
I would use prefix to avoid intersection with another headers. For
example "TARANTOOL_TEST_H" or something else.
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +/*
> + * Test module, based on TAP 14 specification [1].
> + * [1]: https://testanything.org/tap-version-14-specification.html
> + * Version 13 is set for better compatibility on old machines.
> + *
> + * TODO:
> + * * Helpers assert macros:
> + * - assert_uint_equal if needed
> + * - assert_uint_not_equal if needed
> + * - assert_str_equal if needed
> + * - assert_str_not_equal if needed
> + * - assert_memory_equal if needed
> + * - assert_memory_not_equal if needed
> + * * Pragmas.
> + */
> +
> +#define TAP_VERSION 13
> +
> +#define TEST_EXIT_SUCCESS 0
> +#define TEST_EXIT_FAILURE 1
> +
> +#define TEST_JMP_STATUS_SHIFT 2
> +#define TEST_LJMP_EXIT_SUCCESS (TEST_EXIT_SUCCESS + TEST_JMP_STATUS_SHIFT)
> +#define TEST_LJMP_EXIT_FAILURE (TEST_EXIT_FAILURE + TEST_JMP_STATUS_SHIFT)
> +
> +#define TEST_NORET __attribute__((noreturn))
> +
> +typedef int (*test_func)(void *test_state);
> +struct test_unit {
> + const char *name;
> + test_func f;
> +};
> +
> +/* API declaration. */
> +
> +/*
> + * Print formatted message with the corresponding indent.
> + * If you want to leave a comment, use `test_comment()` instead.
> + */
> +void test_message(const char *fmt, ...);
> +
> +/* Need for `skip_all()`, please, don't use it. */
> +void _test_print_skip_all(const char *group_name, const char *reason);
> +/* End test via `longjmp()`, please, don't use it. */
> +TEST_NORET void _test_exit(int status);
> +
> +void test_set_skip_reason(const char *reason);
> +void test_set_todo_reason(const char *reason);
> +/*
> + * Save formatted diagnostic data. Each entry separated with \n.
> + */
> +void test_save_diag_data(const char *fmt, ...);
> +
> +/* Internal, it is better to use `test_run_group()` instead. */
> +int _test_run_group(const char *group_name, const struct test_unit tests[],
> + size_t n_tests, void *test_state);
> +
> +/* Initialize `test_unit` structure. */
> +#define test_unit_new(f) {#f, f}
> +
> +#define lengthof(arr) (sizeof(arr) / sizeof((arr)[0]))
> +
> +/*
> + * __func__ is the name for a test group, "main" for the parent
> + * test.
> + */
> +#define test_run_group(t_arr, t_state) \
> + _test_run_group(__func__, t_arr, lengthof(t_arr), t_state)
> +
> +#define SKIP_DIRECTIVE " # SKIP "
> +#define TODO_DIRECTIVE " # TODO "
> +
> +#define skip_all(reason) ({ \
> + _test_print_skip_all(__func__, reason); \
> + TEST_EXIT_SUCCESS; \
> +})
> +
> +static inline int skip(const char *reason)
> +{
> + test_set_skip_reason(reason);
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static inline int todo(const char *reason)
> +{
> + test_set_todo_reason(reason);
> + return TEST_EXIT_FAILURE;
> +}
> +
> +#define bail_out(reason) do { \
> + /* \
> + * For backwards compatibility with TAP13 Harnesses, \
> + * Producers _should_ emit a "Bail out!" line at the root \
> + * indentation level whenever a Subtest bails out [1]. \
> + */ \
> + printf("Bail out! %s\n", reason); \
> + exit(TEST_EXIT_FAILURE); \
> +} while (0)
> +
> +/* `fmt` should always be a format string here. */
> +#define test_comment(fmt, ...) test_message("# " fmt, __VA_ARGS__)
> +
> +/*
> + * This is a set of useful assert macros like the standard C
> + * libary's assert(3) macro.
> + *
> + * On an assertion failure an assert macro will save the
> + * diagnostic to the special buffer, to be reported via YAML
> + * Diagnostic block and finish a test function with
> + * `return TEST_EXIT_FAILURE`.
> + *
> + * Due to limitations of the C language `assert_true()` and
> + * `assert_false()` macros can only display the expression that
> + * caused the assertion failure. Type specific assert macros,
> + * `assert_{type}_equal()` and `assert_{type}_not_equal()`, save
> + * the data that caused the assertion failure which increases data
> + * visibility aiding debugging of failing test cases.
> + */
> +
> +#define LOCATION_FMT "location:\t%s:%d\n"
> +#define ASSERT_NAME_FMT(name) "failed_assertion:\t" #name "\n"
> +#define ASSERT_EQUAL_FMT(name_type, type_fmt) \
> + LOCATION_FMT \
> + ASSERT_NAME_FMT(assert_ ## name_type ## _equal) \
> + "got: " type_fmt "\n" \
> + "expected: " type_fmt "\n"
> +
> +#define ASSERT_NOT_EQUAL_FMT(type_fmt) \
> + LOCATION_FMT \
> + ASSERT_NAME_FMT(assert_ ## name_type ## _not_equal) \
> + "got: " type_fmt "\n" \
> + "unexpected: " type_fmt "\n"
> +
> +#define assert_true(cond) do { \
> + if (!(cond)) { \
> + test_save_diag_data(LOCATION_FMT \
> + "condition_failed:\t'" #cond "'\n", \
> + __FILE__, __LINE__); \
> + _test_exit(TEST_LJMP_EXIT_FAILURE); \
> + } \
> +} while (0)
> +
> +#define assert_false(cond) assert_true(!(cond))
> +
> +#define assert_general(cond, fmt, ...) do { \
> + if (!(cond)) { \
> + test_save_diag_data(fmt, __VA_ARGS__); \
> + _test_exit(TEST_LJMP_EXIT_FAILURE); \
> + } \
> +} while (0)
> +
> +#define assert_ptr_equal(got, expected) do { \
> + assert_general((got) == (expected), \
> + ASSERT_EQUAL_FMT(ptr, "%p"), \
> + __FILE__, __LINE__, (got), (expected) \
> + ); \
> +} while (0)
> +
> +#define assert_ptr_not_equal(got, unexpected) do { \
> + assert_general((got) != (unexpected), \
> + ASSERT_NOT_EQUAL_FMT(ptr, "%p"), \
> + __FILE__, __LINE__, (got), (unexpected) \
> + ); \
> +} while (0)
> +
> +
> +#define assert_int_equal(got, expected) do { \
> + assert_general((got) == (expected), \
> + ASSERT_EQUAL_FMT(int, "%d"), \
> + __FILE__, __LINE__, (got), (expected) \
> + ); \
> +} while (0)
> +
> +#define assert_int_not_equal(got, unexpected) do { \
> + assert_general((got) != (unexpected), \
> + ASSERT_NOT_EQUAL_FMT(int, "%d"), \
> + __FILE__, __LINE__, (got), (unexpected) \
> + ); \
> +} while (0)
> +
> +#define assert_sizet_equal(got, expected) do { \
> + assert_general((got) == (expected), \
> + ASSERT_EQUAL_FMT(sizet, "%lu"), \
> + __FILE__, __LINE__, (got), (expected) \
> + ); \
> +} while (0)
> +
> +#define assert_sizet_not_equal(got, unexpected) do { \
> + assert_general((got) != (unexpected), \
> + ASSERT_NOT_EQUAL_FMT(sizet, "%lu"), \
> + __FILE__, __LINE__, (got), (unexpected) \
> + ); \
> +} while (0)
> +
> +/* Check that doubles are __exactly__ the same. */
> +#define assert_double_equal(got, expected) do { \
> + assert_general((got) == (expected), \
> + ASSERT_EQUAL_FMT(double, "%lf"), \
> + __FILE__, __LINE__, (got), (expected) \
> + ); \
> +} while (0)
> +
> +/* Check that doubles are not __exactly__ the same. */
> +#define assert_double_not_equal(got, unexpected) do { \
> + assert_general((got) != (unexpected), \
> + ASSERT_NOT_EQUAL_FMT(double, "%lf"), \
> + __FILE__, __LINE__, (got), (unexpected) \
> + ); \
> +} while (0)
> +
> +#endif /* TEST_H */
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-22 12:33 ` Sergey Bronnikov via Tarantool-patches
@ 2023-05-24 6:41 ` Sergey Kaplun via Tarantool-patches
2023-05-25 17:33 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-24 6:41 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the review!
Fixed some of your comments, branch is force-pushed.
On 22.05.23, Sergey Bronnikov wrote:
> Hi, Sergey!
>
>
> Thanks for the patch! Please see my comments inline.
>
>
> On 5/18/23 23:44, Sergey Kaplun wrote:
> > We need an instrument to write tests in plain C for LuaJIT, to be able:
> > * easily test LuaC API
> > * test patches without usage of plain Lua
> > * write unit tests
> > * startup LuaJIT with custom memory allocator, to test some GC issues
> > * maybe, in future, use custom hashing function to test a behavior
> > of LuaJIT tables
> > and so on.
>
> Unexpected newline, put this on a previous line.
Fixed.
>
> >
> > The <test.c> module serves to achieve these goals without too fancy
> > features.
> >
> > It's functionality inspired by cmoka API [1], but only TAP14 [2]
> typo: cmocka
Fixed, thanks!
> > protocol is supported (Version of TAP set to 13 to be compatible with
> > old TAP13 harnesses).
>
> Please add tests for TAP13/TAP14 conformance testing.
>
> It would be unpleasant if proposed library will produce TAP-incompatible
> output and it will break parsing in 'prove'.
>
> At least single test for passed testcase "ok", single testcase for
> failed "not ok" testcase, one testcase for every directive.
I've added the tests for ok|skip|todo. "not ok" is skipped, because it
rather tricky to test it behaviour via prove without test failure.
The content of <test/tarantool-c-tests/unit-tap.test.c> is the
following:
===================================================================
#include "test.h"
#define UNUSED(x) ((void)(x))
static int test_ok(void *test_state)
{
UNUSED(test_state);
return TEST_EXIT_SUCCESS;
}
static int test_skip(void *test_state)
{
UNUSED(test_state);
return skip("test skip");
}
static int test_todo(void *test_state)
{
UNUSED(test_state);
return todo("test todo");
}
int main(void)
{
const struct test_unit tgroup[] = {
test_unit_new(test_ok),
test_unit_new(test_skip),
test_unit_new(test_todo)
};
return test_run_group(tgroup, NULL);
}
===================================================================
>
> > The group of unit tests is declared like the following:
<snipped>
> >
> > [1]: https://github.com/clibs/cmocka
> > [2]: https://testanything.org/tap-version-14-specification.html
> >
> > Part of tarantool/tarantool#7900
> > ---
> > test/CMakeLists.txt | 2 +
> > test/tarantool-c-tests/CMakeLists.txt | 41 +++++
> > test/tarantool-c-tests/test.c | 251 ++++++++++++++++++++++++++
> > test/tarantool-c-tests/test.h | 217 ++++++++++++++++++++++
> > 4 files changed, 511 insertions(+)
> > create mode 100644 test/tarantool-c-tests/CMakeLists.txt
> > create mode 100644 test/tarantool-c-tests/test.c
> > create mode 100644 test/tarantool-c-tests/test.h
> >
> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index a8262b12..47296a22 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -48,12 +48,14 @@ separate_arguments(LUAJIT_TEST_COMMAND)
> > add_subdirectory(LuaJIT-tests)
> > add_subdirectory(PUC-Rio-Lua-5.1-tests)
> > add_subdirectory(lua-Harness-tests)
> > +add_subdirectory(tarantool-c-tests)
> > add_subdirectory(tarantool-tests)
> >
> > add_custom_target(${PROJECT_NAME}-test DEPENDS
> > LuaJIT-tests
> > PUC-Rio-Lua-5.1-tests
> > lua-Harness-tests
> > + tarantool-c-tests
> > tarantool-tests
> Should we rename tarantool-tests to tarantool-lua-tests in a separate
> commit?
Maybe, but I don't really want to do it since inconsistencies with
Tarantool's integration suite, so I suppose we can leave it as is, can't
we?
> > )
> >
> > diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> > new file mode 100644
> > index 00000000..c6b7cd30
> > --- /dev/null
> > +++ b/test/tarantool-c-tests/CMakeLists.txt
> > @@ -0,0 +1,41 @@
> > +find_program(PROVE prove)
> > +if(NOT PROVE)
> > + message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
> > + return()
> > +endif()
>
> copy-pasted from test/tarantool-tests/CMakeLists.txt, I would place it
> to test/CMakeLists.txt.
>
> At least searching prove (find_program(PROVE prove)).
As, discussed here [1], this refactoring should be the part of the separate
pachset.
>
> > +
> > +set(C_TEST_SUFFIX .c_test)
>
> Lua tests in test/tarantool-tests have prefix ".test.lua".
>
> Should we use here ".test.c" for consistency?
"c_test" is the suffix for executable files.
".test.c" is used for test files themselves.
>
>
> > +set(C_TEST_FLAGS --failures --shuffle)
> > +
<snipped>
> > diff --git a/test/tarantool-c-tests/test.c b/test/tarantool-c-tests/test.c
> > new file mode 100644
> > index 00000000..74cba3a3
> > --- /dev/null
> > +++ b/test/tarantool-c-tests/test.c
<snipped>
> > diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
> > new file mode 100644
> > index 00000000..28df9daf
> > --- /dev/null
> > +++ b/test/tarantool-c-tests/test.h
> > @@ -0,0 +1,217 @@
> > +#ifndef TEST_H
> > +#define TEST_H
> I would use prefix to avoid intersection with another headers. For
> example "TARANTOOL_TEST_H" or something else.
Replaced with TARANTOOL_LUAJIT_TEST_H. See the iterative patch below:
===================================================================
diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
index 28df9daf..047f01a5 100644
--- a/test/tarantool-c-tests/test.h
+++ b/test/tarantool-c-tests/test.h
@@ -1,5 +1,5 @@
-#ifndef TEST_H
-#define TEST_H
+#ifndef TARANTOOL_LUAJIT_TEST_H
+#define TARANTOOL_LUAJIT_TEST_H
#include <stdio.h>
#include <stdlib.h>
@@ -214,4 +214,4 @@ static inline int todo(const char *reason)
); \
} while (0)
-#endif /* TEST_H */
+#endif /* TARANTOOL_LUAJIT_TEST_H */
===================================================================
Same for utils header:
===================================================================
diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
index cf668006..f651f7f6 100644
--- a/test/tarantool-c-tests/utils.h
+++ b/test/tarantool-c-tests/utils.h
@@ -1,3 +1,6 @@
+#ifndef TARANTOOL_LUAJIT_TEST_UTILS_H
+#define TARANTOOL_LUAJIT_TEST_UTILS_H
+
#include <limits.h>
#include <string.h>
@@ -61,3 +64,5 @@ static void utils_lua_close(lua_State *L)
if (!lua_isfunction((L), -1)) \
bail_out("Can't get auxiliary test function"); \
} while (0)
+
+#endif /* TARANTOOL_LUAJIT_TEST_UTILS_H */
===================================================================
> > +
<snipped>
> > +#endif /* TEST_H */
[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2023-May/027577.html
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-24 6:41 ` Sergey Kaplun via Tarantool-patches
@ 2023-05-25 17:33 ` Sergey Bronnikov via Tarantool-patches
2023-05-29 10:03 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 29+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-05-25 17:33 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi, Sergey! Thanks for fixes, see comments below.
On 5/24/23 09:41, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
>
> Fixed some of your comments, branch is force-pushed.
>
<snipped>
> The <test.c> module serves to achieve these goals without too fancy
> features.
>
> It's functionality inspired by cmoka API [1], but only TAP14 [2]
>> typo: cmocka
> Fixed, thanks!
Nit: probably it is better "CMocka", because it is a proper name ("имя
собственное") and should be with capital letters.
Feel free to ignore.
>
>>> protocol is supported (Version of TAP set to 13 to be compatible with
>>> old TAP13 harnesses).
>> Please add tests for TAP13/TAP14 conformance testing.
>>
>> It would be unpleasant if proposed library will produce TAP-incompatible
>> output and it will break parsing in 'prove'.
>>
>> At least single test for passed testcase "ok", single testcase for
>> failed "not ok" testcase, one testcase for every directive.
> I've added the tests for ok|skip|todo. "not ok" is skipped, because it
> rather tricky to test it behaviour via prove without test failure.
>
> The content of <test/tarantool-c-tests/unit-tap.test.c> is the
> following:
> ===================================================================
> #include "test.h"
>
> #define UNUSED(x) ((void)(x))
>
> static int test_ok(void *test_state)
> {
> UNUSED(test_state);
> return TEST_EXIT_SUCCESS;
> }
>
> static int test_skip(void *test_state)
> {
> UNUSED(test_state);
> return skip("test skip");
> }
>
> static int test_todo(void *test_state)
> {
> UNUSED(test_state);
> return todo("test todo");
> }
>
> int main(void)
> {
> const struct test_unit tgroup[] = {
> test_unit_new(test_ok),
> test_unit_new(test_skip),
> test_unit_new(test_todo)
> };
> return test_run_group(tgroup, NULL);
> }
> ===================================================================
This test checks that macroses/functions in test.h could be called and
nothing more.
We need checking TAP output that binary will emit for different test
statuses, plan, testcase counters ("1..10").
>
>>> The group of unit tests is declared like the following:
> <snipped>
>
>>> [1]: https://github.com/clibs/cmocka
>>> [2]: https://testanything.org/tap-version-14-specification.html
>>>
>>> Part of tarantool/tarantool#7900
>>> ---
>>> test/CMakeLists.txt | 2 +
>>> test/tarantool-c-tests/CMakeLists.txt | 41 +++++
>>> test/tarantool-c-tests/test.c | 251 ++++++++++++++++++++++++++
>>> test/tarantool-c-tests/test.h | 217 ++++++++++++++++++++++
>>> 4 files changed, 511 insertions(+)
>>> create mode 100644 test/tarantool-c-tests/CMakeLists.txt
>>> create mode 100644 test/tarantool-c-tests/test.c
>>> create mode 100644 test/tarantool-c-tests/test.h
>>>
>>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>>> index a8262b12..47296a22 100644
>>> --- a/test/CMakeLists.txt
>>> +++ b/test/CMakeLists.txt
>>> @@ -48,12 +48,14 @@ separate_arguments(LUAJIT_TEST_COMMAND)
>>> add_subdirectory(LuaJIT-tests)
>>> add_subdirectory(PUC-Rio-Lua-5.1-tests)
>>> add_subdirectory(lua-Harness-tests)
>>> +add_subdirectory(tarantool-c-tests)
>>> add_subdirectory(tarantool-tests)
>>>
>>> add_custom_target(${PROJECT_NAME}-test DEPENDS
>>> LuaJIT-tests
>>> PUC-Rio-Lua-5.1-tests
>>> lua-Harness-tests
>>> + tarantool-c-tests
>>> tarantool-tests
>> Should we rename tarantool-tests to tarantool-lua-tests in a separate
>> commit?
> Maybe, but I don't really want to do it since inconsistencies with
> Tarantool's integration suite, so I suppose we can leave it as is, can't
> we?
Sure, let's skip.
>>> )
>>>
>>> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
>>> new file mode 100644
>>> index 00000000..c6b7cd30
>>> --- /dev/null
>>> +++ b/test/tarantool-c-tests/CMakeLists.txt
>>> @@ -0,0 +1,41 @@
>>> +find_program(PROVE prove)
>>> +if(NOT PROVE)
>>> + message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
>>> + return()
>>> +endif()
>> copy-pasted from test/tarantool-tests/CMakeLists.txt, I would place it
>> to test/CMakeLists.txt.
>>
>> At least searching prove (find_program(PROVE prove)).
> As, discussed here [1], this refactoring should be the part of the separate
> pachset.
OK
>
>>> +
>>> +set(C_TEST_SUFFIX .c_test)
>> Lua tests in test/tarantool-tests have prefix ".test.lua".
>>
>> Should we use here ".test.c" for consistency?
> "c_test" is the suffix for executable files.
> ".test.c" is used for test files themselves.
OK
>
>>
>>> +set(C_TEST_FLAGS --failures --shuffle)
>>> +
> <snipped>
>
>>> diff --git a/test/tarantool-c-tests/test.c b/test/tarantool-c-tests/test.c
>>> new file mode 100644
>>> index 00000000..74cba3a3
>>> --- /dev/null
>>> +++ b/test/tarantool-c-tests/test.c
> <snipped>
>
>>> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
>>> new file mode 100644
>>> index 00000000..28df9daf
>>> --- /dev/null
>>> +++ b/test/tarantool-c-tests/test.h
>>> @@ -0,0 +1,217 @@
>>> +#ifndef TEST_H
>>> +#define TEST_H
>> I would use prefix to avoid intersection with another headers. For
>> example "TARANTOOL_TEST_H" or something else.
> Replaced with TARANTOOL_LUAJIT_TEST_H. See the iterative patch below:
Good, thanks!
>
> ===================================================================
> diff --git a/test/tarantool-c-tests/test.h b/test/tarantool-c-tests/test.h
> index 28df9daf..047f01a5 100644
> --- a/test/tarantool-c-tests/test.h
> +++ b/test/tarantool-c-tests/test.h
> @@ -1,5 +1,5 @@
> -#ifndef TEST_H
> -#define TEST_H
> +#ifndef TARANTOOL_LUAJIT_TEST_H
> +#define TARANTOOL_LUAJIT_TEST_H
>
> #include <stdio.h>
> #include <stdlib.h>
> @@ -214,4 +214,4 @@ static inline int todo(const char *reason)
> ); \
> } while (0)
>
> -#endif /* TEST_H */
> +#endif /* TARANTOOL_LUAJIT_TEST_H */
> ===================================================================
>
> Same for utils header:
>
> ===================================================================
> diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
> index cf668006..f651f7f6 100644
> --- a/test/tarantool-c-tests/utils.h
> +++ b/test/tarantool-c-tests/utils.h
> @@ -1,3 +1,6 @@
> +#ifndef TARANTOOL_LUAJIT_TEST_UTILS_H
> +#define TARANTOOL_LUAJIT_TEST_UTILS_H
> +
> #include <limits.h>
> #include <string.h>
>
> @@ -61,3 +64,5 @@ static void utils_lua_close(lua_State *L)
> if (!lua_isfunction((L), -1)) \
> bail_out("Can't get auxiliary test function"); \
> } while (0)
> +
> +#endif /* TARANTOOL_LUAJIT_TEST_UTILS_H */
> ===================================================================
>
>>> +
> <snipped>
>
>>> +#endif /* TEST_H */
> [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2023-May/027577.html
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-25 17:33 ` Sergey Bronnikov via Tarantool-patches
@ 2023-05-29 10:03 ` Sergey Kaplun via Tarantool-patches
2023-05-29 14:38 ` Sergey Bronnikov via Tarantool-patches
0 siblings, 1 reply; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-29 10:03 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the review!
Fixed your comments and force-pushed the branch.
On 25.05.23, Sergey Bronnikov wrote:
> Hi, Sergey! Thanks for fixes, see comments below.
>
> On 5/24/23 09:41, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the review!
> >
> > Fixed some of your comments, branch is force-pushed.
> >
> <snipped>
> > The <test.c> module serves to achieve these goals without too fancy
> > features.
> >
> > It's functionality inspired by cmoka API [1], but only TAP14 [2]
> >> typo: cmocka
> > Fixed, thanks!
>
>
> Nit: probably it is better "CMocka", because it is a proper name ("имя
> собственное") and should be with capital letters.
>
> Feel free to ignore.
Fixed! Thanks!
>
> >
> >>> protocol is supported (Version of TAP set to 13 to be compatible with
> >>> old TAP13 harnesses).
> >> Please add tests for TAP13/TAP14 conformance testing.
> >>
> >> It would be unpleasant if proposed library will produce TAP-incompatible
> >> output and it will break parsing in 'prove'.
> >>
> >> At least single test for passed testcase "ok", single testcase for
> >> failed "not ok" testcase, one testcase for every directive.
> > I've added the tests for ok|skip|todo. "not ok" is skipped, because it
> > rather tricky to test it behaviour via prove without test failure.
> >
> > The content of <test/tarantool-c-tests/unit-tap.test.c> is the
> > following:
> > ===================================================================
<snipped>
> > ===================================================================
>
> This test checks that macroses/functions in test.h could be called and
> nothing more.
>
> We need checking TAP output that binary will emit for different test
> statuses, plan, testcase counters ("1..10").
As discussed offline: its not the main goal of this suite introducing,
also, TAP13 protocol is tested by `prove` itself, and fully compatible
with TAP14 protocol. Also, added the following check of parsing to be
sure in the emmited format correctness.
===================================================================
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index bf98856f..1aade851 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -36,6 +36,9 @@ add_custom_command(TARGET tarantool-c-tests
${CMAKE_CURRENT_BINARY_DIR}
--ext ${C_TEST_SUFFIX}
--jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
+ # Reportt any TAP parse errors, if any, since test module is
+ # maintained by us.
+ --parse
${C_TEST_FLAGS}
WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
===================================================================
Also, mentioned this in the commit message:
| The library itself is tested via some primitive tests for `ok` case,
| `skip` and `todo` directives. The TAP13 format is tested via prove, that
| we are using for running our tests. TAP14 format is compatible with
| TAP13, so there are no other tests required.
<snipped>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-29 10:03 ` Sergey Kaplun via Tarantool-patches
@ 2023-05-29 14:38 ` Sergey Bronnikov via Tarantool-patches
2023-05-31 13:32 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 29+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-05-29 14:38 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
This patch is LGTM after fixing typo below.
On 5/29/23 13:03, Sergey Kaplun wrote:
>
>>> ===================================================================
>> This test checks that macroses/functions in test.h could be called and
>> nothing more.
>>
>> We need checking TAP output that binary will emit for different test
>> statuses, plan, testcase counters ("1..10").
> As discussed offline: its not the main goal of this suite introducing,
> also, TAP13 protocol is tested by `prove` itself, and fully compatible
> with TAP14 protocol. Also, added the following check of parsing to be
> sure in the emmited format correctness.
>
> ===================================================================
> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> index bf98856f..1aade851 100644
> --- a/test/tarantool-c-tests/CMakeLists.txt
> +++ b/test/tarantool-c-tests/CMakeLists.txt
> @@ -36,6 +36,9 @@ add_custom_command(TARGET tarantool-c-tests
> ${CMAKE_CURRENT_BINARY_DIR}
> --ext ${C_TEST_SUFFIX}
> --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> + # Reportt any TAP parse errors, if any, since test module is
Typo: Reportt -> Report
> + # maintained by us.
> + --parse
> ${C_TEST_FLAGS}
> WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> ===================================================================
>
> Also, mentioned this in the commit message:
>
> | The library itself is tested via some primitive tests for `ok` case,
> | `skip` and `todo` directives. The TAP13 format is tested via prove, that
> | we are using for running our tests. TAP14 format is compatible with
> | TAP13, so there are no other tests required.
>
>
> <snipped>
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
2023-05-29 14:38 ` Sergey Bronnikov via Tarantool-patches
@ 2023-05-31 13:32 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-31 13:32 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the review!
On 29.05.23, Sergey Bronnikov wrote:
> This patch is LGTM after fixing typo below.
>
> On 5/29/23 13:03, Sergey Kaplun wrote:
> >
> >>> ===================================================================
> >> This test checks that macroses/functions in test.h could be called and
> >> nothing more.
> >>
> >> We need checking TAP output that binary will emit for different test
> >> statuses, plan, testcase counters ("1..10").
> > As discussed offline: its not the main goal of this suite introducing,
> > also, TAP13 protocol is tested by `prove` itself, and fully compatible
> > with TAP14 protocol. Also, added the following check of parsing to be
> > sure in the emmited format correctness.
> >
> > ===================================================================
> > diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> > index bf98856f..1aade851 100644
> > --- a/test/tarantool-c-tests/CMakeLists.txt
> > +++ b/test/tarantool-c-tests/CMakeLists.txt
> > @@ -36,6 +36,9 @@ add_custom_command(TARGET tarantool-c-tests
> > ${CMAKE_CURRENT_BINARY_DIR}
> > --ext ${C_TEST_SUFFIX}
> > --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
> > + # Reportt any TAP parse errors, if any, since test module is
>
> Typo: Reportt -> Report
Fixed, thanks!
>
<snipped>
> >
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper for C tests
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 1/6] test: fix setting of {DY}LD_LIBRARY_PATH variables Sergey Kaplun via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests Sergey Kaplun via Tarantool-patches
@ 2023-05-18 20:44 ` Sergey Kaplun via Tarantool-patches
2023-05-19 11:58 ` Maxim Kokryashkin via Tarantool-patches
2023-05-29 15:26 ` Sergey Bronnikov via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C Sergey Kaplun via Tarantool-patches
` (3 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-18 20:44 UTC (permalink / raw)
To: Igor Munkin, Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
This header contains generic init and close for tests and helpers for
loading auxiliary Lua script with functions to run inside a C test.
It will be properly used in the next commit.
Part of tarantool/tarantool#7900
---
test/tarantool-c-tests/utils.h | 63 ++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
create mode 100644 test/tarantool-c-tests/utils.h
diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
new file mode 100644
index 00000000..cf668006
--- /dev/null
+++ b/test/tarantool-c-tests/utils.h
@@ -0,0 +1,63 @@
+#include <limits.h>
+#include <string.h>
+
+#include "lauxlib.h"
+#include "lua.h"
+#include "luajit.h"
+#include "lualib.h"
+
+#include "test.h"
+
+/* Generic init for our tests. */
+static lua_State *utils_lua_init(void)
+{
+ lua_State *L = luaL_newstate();
+ if (!L)
+ bail_out("Can't init Lua state");
+ /* Stop collector during library initialization. */
+ lua_gc(L, LUA_GCSTOP, 0);
+ luaL_openlibs(L);
+ lua_gc(L, LUA_GCRESTART, -1);
+ return L;
+}
+
+/* Generic close for our tests. */
+static void utils_lua_close(lua_State *L)
+{
+ lua_close(L);
+}
+
+/*
+ * Load the pair to the test file <filename-script.lua>.
+ * Each file should return the table with functions (named the
+ * same as the corresponding unit tests) to call in unit tests.
+ * Push the table with those functions on the Lua stack.
+ */
+#define utils_load_aux_script(L) do { \
+ /* \
+ * Format script name. \
+ * `__ABS_FILENAME__` is set via CMake. \
+ */ \
+ char script[PATH_MAX] = __ABS_FILENAME__; \
+ char *file_suffix = strstr(script, ".test.c"); \
+ strcpy(file_suffix, "-script.lua"); \
+ \
+ if (luaL_dofile((L), script) != LUA_OK) { \
+ test_comment("Can't load %s: '%s'", script, \
+ lua_tostring((L), -1)); \
+ bail_out("Can't load auxiliary script"); \
+ } \
+ \
+ if (!lua_istable((L), -1)) \
+ bail_out("Returned value from script is not a table"); \
+} while (0)
+
+/*
+ * Accept a table on top of the Lua stack which containing the
+ * function named as the unit test we currently executing.
+ */
+#define utils_get_aux_lfunc(L) do { \
+ lua_getfield((L), -1, __func__); \
+ if (!lua_isfunction((L), -1)) \
+ bail_out("Can't get auxiliary test function"); \
+} while (0)
--
2.34.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper for C tests
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper " Sergey Kaplun via Tarantool-patches
@ 2023-05-19 11:58 ` Maxim Kokryashkin via Tarantool-patches
2023-05-20 7:52 ` Sergey Kaplun via Tarantool-patches
2023-05-29 15:26 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 1 reply; 29+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-19 11:58 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]
Hi, Sergey!
Thanks for the patch!
Please consider a few comments below.
>
>>This header contains generic init and close for tests and helpers for
>>loading auxiliary Lua script with functions to run inside a C test.
>>
>>It will be properly used in the next commit.
>>
>>Part of tarantool/tarantool#7900
>>---
>> test/tarantool-c-tests/utils.h | 63 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 63 insertions(+)
>> create mode 100644 test/tarantool-c-tests/utils.h
>>
>>diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
>>new file mode 100644
>>index 00000000..cf668006
>>--- /dev/null
>>+++ b/test/tarantool-c-tests/utils.h
>>@@ -0,0 +1,63 @@
>>+#include <limits.h>
>>+#include <string.h>
>>+
>>+#include "lauxlib.h"
>>+#include "lua.h"
>>+#include "luajit.h"
>>+#include "lualib.h"
>>+
>>+#include "test.h"
>>+
>>+/* Generic init for our tests. */
>>+static lua_State *utils_lua_init(void)
>>+{
>>+ lua_State *L = luaL_newstate();
>>+ if (!L)
>>+ bail_out("Can't init Lua state");
>>+ /* Stop collector during library initialization. */
>>+ lua_gc(L, LUA_GCSTOP, 0);
>>+ luaL_openlibs(L);
>>+ lua_gc(L, LUA_GCRESTART, -1);
>>+ return L;
>>+}
>>+
>>+/* Generic close for our tests. */
>>+static void utils_lua_close(lua_State *L)
>>+{
>>+ lua_close(L);
>>+}
>Do we __really__ need this function?)
>>+
>>+/*
>>+ * Load the pair to the test file <filename-script.lua>.
>>+ * Each file should return the table with functions (named the
>>+ * same as the corresponding unit tests) to call in unit tests.
>>+ * Push the table with those functions on the Lua stack.
>>+ */
>>+#define utils_load_aux_script(L) do { \
>>+ /* \
>>+ * Format script name. \
>>+ * `__ABS_FILENAME__` is set via CMake. \
>>+ */ \
>>+ char script[PATH_MAX] = __ABS_FILENAME__; \
>>+ char *file_suffix = strstr(script, ".test.c"); \
>The `strstr` function is not safe, use `strnstr` instead.
>Also, maybe it is better to use the `strrstr` variation,
>because it searches from the end. It eleminates issues
>in case of a file with .test.c somewhere else in the name.
>>+ strcpy(file_suffix, "-script.lua"); \
>Same applies here.
>>+ \
>>+ if (luaL_dofile((L), script) != LUA_OK) { \
>>+ test_comment("Can't load %s: '%s'", script, \
>>+ lua_tostring((L), -1)); \
>>+ bail_out("Can't load auxiliary script"); \
>>+ } \
>>+ \
>>+ if (!lua_istable((L), -1)) \
>>+ bail_out("Returned value from script is not a table"); \
>>+} while (0)
>>+
>>+/*
>>+ * Accept a table on top of the Lua stack which containing the
>>+ * function named as the unit test we currently executing.
>>+ */
>>+#define utils_get_aux_lfunc(L) do { \
>>+ lua_getfield((L), -1, __func__); \
>>+ if (!lua_isfunction((L), -1)) \
>>+ bail_out("Can't get auxiliary test function"); \
>>+} while (0)
>>--
>>2.34.1
>
>--
>Best regards,
>Maxim Kokryashkin
>
[-- Attachment #2: Type: text/html, Size: 3945 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper for C tests
2023-05-19 11:58 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-20 7:52 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-20 7:52 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Maxim!
Thanks for the review!
Please, see my comments below.
On 19.05.23, Maxim Kokryashkin wrote:
>
> Hi, Sergey!
> Thanks for the patch!
> Please consider a few comments below.
> >
> >>This header contains generic init and close for tests and helpers for
> >>loading auxiliary Lua script with functions to run inside a C test.
> >>
> >>It will be properly used in the next commit.
> >>
> >>Part of tarantool/tarantool#7900
> >>---
> >> test/tarantool-c-tests/utils.h | 63 ++++++++++++++++++++++++++++++++++
> >> 1 file changed, 63 insertions(+)
> >> create mode 100644 test/tarantool-c-tests/utils.h
> >>
> >>diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
> >>new file mode 100644
> >>index 00000000..cf668006
> >>--- /dev/null
> >>+++ b/test/tarantool-c-tests/utils.h
> >>@@ -0,0 +1,63 @@
> >>+#include <limits.h>
> >>+#include <string.h>
> >>+
> >>+#include "lauxlib.h"
> >>+#include "lua.h"
> >>+#include "luajit.h"
> >>+#include "lualib.h"
> >>+
> >>+#include "test.h"
> >>+
> >>+/* Generic init for our tests. */
> >>+static lua_State *utils_lua_init(void)
> >>+{
> >>+ lua_State *L = luaL_newstate();
> >>+ if (!L)
> >>+ bail_out("Can't init Lua state");
> >>+ /* Stop collector during library initialization. */
> >>+ lua_gc(L, LUA_GCSTOP, 0);
> >>+ luaL_openlibs(L);
> >>+ lua_gc(L, LUA_GCRESTART, -1);
> >>+ return L;
> >>+}
> >>+
> >>+/* Generic close for our tests. */
> >>+static void utils_lua_close(lua_State *L)
> >>+{
> >>+ lua_close(L);
> >>+}
> >Do we __really__ need this function?)
We haven't any other work to do __for now__, but with time maybe other
checks will come here.
So, leave it as is for consistensy, if you don't mind.
> >>+
> >>+/*
> >>+ * Load the pair to the test file <filename-script.lua>.
> >>+ * Each file should return the table with functions (named the
> >>+ * same as the corresponding unit tests) to call in unit tests.
> >>+ * Push the table with those functions on the Lua stack.
> >>+ */
> >>+#define utils_load_aux_script(L) do { \
> >>+ /* \
> >>+ * Format script name. \
> >>+ * `__ABS_FILENAME__` is set via CMake. \
> >>+ */ \
> >>+ char script[PATH_MAX] = __ABS_FILENAME__; \
> >>+ char *file_suffix = strstr(script, ".test.c"); \
> >The `strstr` function is not safe, use `strnstr` instead.
> >Also, maybe it is better to use the `strrstr` variation,
> >because it searches from the end. It eleminates issues
> >in case of a file with .test.c somewhere else in the name.
There's no function named `strnstr()`, in standard C or POSIX. It's not
in glibc/Linux either, IINM. I want to avoid additional linking
dependencies (as for -lsstrings2). `strstr()` is unsafe when we have
non-null terminated strings. It's not our case.
> >>+ strcpy(file_suffix, "-script.lua"); \
> >Same applies here.
Don't see the reason to write `"script.lua", sizeof("script.lua")`
| The strncpy() function is similar, except that at most n bytes of
| src are copied. Warning: If there is no null byte among the
| first n bytes of src, the string placed in dest will not be
| null-terminated.
So, I'll just use `strcpy()`.
> >>+ \
> >>+ if (luaL_dofile((L), script) != LUA_OK) { \
> >>+ test_comment("Can't load %s: '%s'", script, \
> >>+ lua_tostring((L), -1)); \
> >>+ bail_out("Can't load auxiliary script"); \
> >>+ } \
> >>+ \
> >>+ if (!lua_istable((L), -1)) \
> >>+ bail_out("Returned value from script is not a table"); \
> >>+} while (0)
> >>+
> >>+/*
> >>+ * Accept a table on top of the Lua stack which containing the
> >>+ * function named as the unit test we currently executing.
> >>+ */
> >>+#define utils_get_aux_lfunc(L) do { \
> >>+ lua_getfield((L), -1, __func__); \
> >>+ if (!lua_isfunction((L), -1)) \
> >>+ bail_out("Can't get auxiliary test function"); \
> >>+} while (0)
> >>--
> >>2.34.1
> >
> >--
> >Best regards,
> >Maxim Kokryashkin
> >
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper for C tests
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper " Sergey Kaplun via Tarantool-patches
2023-05-19 11:58 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-29 15:26 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 0 replies; 29+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-05-29 15:26 UTC (permalink / raw)
To: Sergey Kaplun, Igor Munkin, Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the patch! See my comments below.
On 5/18/23 23:44, Sergey Kaplun wrote:
> This header contains generic init and close for tests and helpers for
probably "generic init and close *functions*"?
> loading auxiliary Lua script with functions to run inside a C test.
>
> It will be properly used in the next commit.
>
> Part of tarantool/tarantool#7900
> ---
> test/tarantool-c-tests/utils.h | 63 ++++++++++++++++++++++++++++++++++
> 1 file changed, 63 insertions(+)
> create mode 100644 test/tarantool-c-tests/utils.h
>
> diff --git a/test/tarantool-c-tests/utils.h b/test/tarantool-c-tests/utils.h
> new file mode 100644
> index 00000000..cf668006
> --- /dev/null
> +++ b/test/tarantool-c-tests/utils.h
> @@ -0,0 +1,63 @@
> +#include <limits.h>
> +#include <string.h>
> +
> +#include "lauxlib.h"
> +#include "lua.h"
> +#include "luajit.h"
> +#include "lualib.h"
> +
> +#include "test.h"
> +
> +/* Generic init for our tests. */
> +static lua_State *utils_lua_init(void)
> +{
> + lua_State *L = luaL_newstate();
> + if (!L)
> + bail_out("Can't init Lua state");
> + /* Stop collector during library initialization. */
> + lua_gc(L, LUA_GCSTOP, 0);
> + luaL_openlibs(L);
> + lua_gc(L, LUA_GCRESTART, -1);
> + return L;
> +}
> +
> +/* Generic close for our tests. */
> +static void utils_lua_close(lua_State *L)
> +{
> + lua_close(L);
> +}
> +
> +/*
> + * Load the pair to the test file <filename-script.lua>.
> + * Each file should return the table with functions (named the
> + * same as the corresponding unit tests) to call in unit tests.
> + * Push the table with those functions on the Lua stack.
> + */
> +#define utils_load_aux_script(L) do { \
> + /* \
> + * Format script name. \
> + * `__ABS_FILENAME__` is set via CMake. \
> + */ \
> + char script[PATH_MAX] = __ABS_FILENAME__; \
Why do you need to pass user script to a test library from a build system?
Can we pass it in a user function?
my_test.c:
char script_path[PATH_MAX] = __TEST_LUA_SCRIPT_ABS_PATH__;
utils_load_aux_script(L, script_path)
BTW filename cannot be absolute, path could be.
> + char *file_suffix = strstr(script, ".test.c"); \
> + strcpy(file_suffix, "-script.lua"); \
> + \
> + if (luaL_dofile((L), script) != LUA_OK) { \
> + test_comment("Can't load %s: '%s'", script, \
> + lua_tostring((L), -1)); \
> + bail_out("Can't load auxiliary script"); \
> + } \
> + \
> + if (!lua_istable((L), -1)) \
> + bail_out("Returned value from script is not a table"); \
> +} while (0)
> +
> +/*
> + * Accept a table on top of the Lua stack which containing the
> + * function named as the unit test we currently executing.
> + */
> +#define utils_get_aux_lfunc(L) do { \
> + lua_getfield((L), -1, __func__); \
> + if (!lua_isfunction((L), -1)) \
> + bail_out("Can't get auxiliary test function"); \
> +} while (0)
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
` (2 preceding siblings ...)
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 3/6] test: introduce utils.h helper " Sergey Kaplun via Tarantool-patches
@ 2023-05-18 20:44 ` Sergey Kaplun via Tarantool-patches
2023-05-19 12:17 ` Maxim Kokryashkin via Tarantool-patches
2023-05-29 16:15 ` Sergey Bronnikov via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 5/6] test: rewrite misclib-sysprof-capi " Sergey Kaplun via Tarantool-patches
` (2 subsequent siblings)
6 siblings, 2 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-18 20:44 UTC (permalink / raw)
To: Igor Munkin, Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
This patch rewrites the aforementioned test with the usage libtest,
recently introduced. Since we still stand in need of a Lua helper script
for generation of traces, the original test file is reworked as a
standalone script, which returns the table with helper functions. Each
helper function is named the same as the test where it will be used. Now
single quotes are used according to our Lua code style.
In C part all asserts from glibc are replaced with the corresponding
assert_{cond} from the libtest. Now tests return `TEST_EXIT_SUCCESS` at
the finish. Also, the stack check for the amount of return results from
the helper function is slightly changed, since there is one more stack
slot in use (table with these functions). `snap_restores()` C function
duplicates 4 times for each subtest. Common helper
`check_snap_restores()` is used for each of them. Each error throwing is
replaced with `bail_out()` call.
NB: `lua_pop()` to clear the Lua stack after a call should be done before
any possible assertion, which would exit from the test leaving the stack
uncleaned.
All skipconds use macros defined in <lj_arch.h>, so it is included in
the test. As far as this test initializes the LuaJIT VM manually, there
is no need to check `jit.status()` result; checking `LJ_HASJIT` is
enough. Now, only JIT-related tests are skipped, when compiled without
JIT. Nevertheless, all tests are skipped for *BSD arches.
Also, this patch sets the new CMake variable named `LUAJIT_LIBRARY`
equal to `LUAJIT_LIB` in `PARENT_SCOPE` to be used in tarantool-c-tests
linking.
Also, .c_test suffix is added to the <.gitignore>.
Part of tarantool/tarantool#7900
---
.gitignore | 1 +
src/CMakeLists.txt | 2 +
test/tarantool-c-tests/CMakeLists.txt | 32 +-
.../misclib-getmetrics-capi-script.lua} | 83 ++---
.../misclib-getmetrics-capi.test.c | 343 ++++++++++++++++++
test/tarantool-tests/CMakeLists.txt | 1 -
.../misclib-getmetrics-capi/CMakeLists.txt | 1 -
.../misclib-getmetrics-capi/testgetmetrics.c | 270 --------------
8 files changed, 412 insertions(+), 321 deletions(-)
rename test/{tarantool-tests/misclib-getmetrics-capi.test.lua => tarantool-c-tests/misclib-getmetrics-capi-script.lua} (68%)
create mode 100644 test/tarantool-c-tests/misclib-getmetrics-capi.test.c
delete mode 100644 test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
delete mode 100644 test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
diff --git a/.gitignore b/.gitignore
index b7908aee..dc5ea5fc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -24,3 +24,4 @@ install_manifest.txt
luajit-parse-memprof
luajit-parse-sysprof
luajit.pc
+*.c_test
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index dffc0a4d..fed4c38d 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -325,6 +325,8 @@ if(NOT BUILDMODE STREQUAL "dynamic")
set(LUAJIT_BIN luajit_static)
set(LUAJIT_LIB libluajit_static)
endif()
+# Need for the test linking, so the PARENT_SCOPE option is used.
+set(LUAJIT_LIBRARY ${LUAJIT_LIB} PARENT_SCOPE)
set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})
add_executable(${LUAJIT_BIN} EXCLUDE_FROM_ALL ${CLI_SOURCES})
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index c6b7cd30..c9d75d6c 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -22,13 +22,37 @@ set_target_properties(libtest PROPERTIES
LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
)
-# XXX: For now, just build libtest. The tests to be depended on
-# will be added in the next commit.
+# TARGET_C_FLAGS is required here to be sure that headers like
+# lj_arch.h in compiled test are consistent with the LuaJIT library
+# to link.
+AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
+
+set(CTEST_SRC_SUFFIX ".test.c")
+file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
+foreach(test_source ${tests})
+ string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe ${test_source})
+ add_executable(${exe} EXCLUDE_FROM_ALL ${test_source})
+ target_include_directories(${exe} PRIVATE
+ ${CMAKE_CURRENT_SOURCE_DIR}
+ ${LUAJIT_SOURCE_DIR}
+ )
+ set_target_properties(${exe} PROPERTIES
+ # `__FILE__` macro may not represnt absolute path to the
+ # source file, depening on cmake version or
+ # -fmacro-prefix-map flag value.
+ # Use custom macro.
+ COMPILE_FLAGS "${TESTS_C_FLAGS} -D__ABS_FILENAME__='\"${test_source}\"'"
+ OUTPUT_NAME "${exe}${C_TEST_SUFFIX}"
+ RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+ )
+ target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
+ LIST(APPEND TESTS_COMPILED ${exe})
+endforeach()
+
add_custom_target(tarantool-c-tests
- DEPENDS libluajit libtest
+ DEPENDS libluajit libtest ${TESTS_COMPILED}
)
-# XXX: For now, run 0 tests. Just verify that libtest was built.
add_custom_command(TARGET tarantool-c-tests
COMMENT "Running Tarantool C tests"
COMMAND
diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
similarity index 68%
rename from test/tarantool-tests/misclib-getmetrics-capi.test.lua
rename to test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
index 654e5545..2f0ee5cf 100644
--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
+++ b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
@@ -1,32 +1,26 @@
-local tap = require('tap')
-local test = tap.test("clib-misc-getmetrics"):skipcond({
- ['Test requires JIT enabled'] = not jit.status(),
- ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
-})
+local ffi = require('ffi')
-test:plan(11)
+-- Auxiliary script to provide Lua functions to be used in tests
+-- for `getmetrics()` C API inside the test
+-- <misclib-getmetrics-capi.test.c>.
+local M = {}
-local path = arg[0]:gsub('%.test%.lua', '')
-local suffix = package.cpath:match('?.(%a+);')
-package.cpath = ('%s/?.%s;'):format(path, suffix)..package.cpath
+-- XXX: Max nins is limited by max IRRef, that equals to
+-- REF_DROP - REF_BIAS. Unfortunately, these constants are not
+-- provided to Lua space, so we ought to make some math:
+-- * REF_DROP = 0xffff
+-- * REF_BIAS = 0x8000
+-- See <src/lj_ir.h> for details.
+local MAXNINS = 0xffff - 0x8000
-local MAXNINS = require('utils').const.maxnins
local jit_opt_default = {
3, -- level
- "hotloop=56",
- "hotexit=10",
- "minstitch=0",
+ 'hotloop=56',
+ 'hotexit=10',
+ 'minstitch=0',
}
-local testgetmetrics = require("testgetmetrics")
-
-test:ok(testgetmetrics.base())
-test:ok(testgetmetrics.gc_allocated_freed())
-test:ok(testgetmetrics.gc_steps())
-
-test:ok(testgetmetrics.objcount(function(iterations)
- local ffi = require("ffi")
-
+M.objcount = function(iterations)
jit.opt.start(0)
local placeholder = {
@@ -51,35 +45,34 @@ test:ok(testgetmetrics.objcount(function(iterations)
for _ = 1, iterations do
-- Check counting of VLA/VLS/aligned cdata.
- table.insert(placeholder.cdata, ffi.new("char[?]", 4))
+ table.insert(placeholder.cdata, ffi.new('char[?]', 4))
end
for _ = 1, iterations do
-- Check counting of non-VLA/VLS/aligned cdata.
- table.insert(placeholder.cdata, ffi.new("uint64_t", _))
+ table.insert(placeholder.cdata, ffi.new('uint64_t', _))
end
placeholder = nil -- luacheck: no unused
-- Restore default jit settings.
jit.opt.start(unpack(jit_opt_default))
-end))
+end
-test:ok(testgetmetrics.objcount_cdata_decrement(function()
+M.objcount_cdata_decrement = function()
-- gc_cdatanum decrement test.
-- See https://github.com/tarantool/tarantool/issues/5820.
- local ffi = require("ffi")
local function nop() end
- ffi.gc(ffi.cast("void *", 0), nop)
+ ffi.gc(ffi.cast('void *', 0), nop)
-- Does not collect the cdata, but resurrects the object and
-- removes LJ_GC_CDATA_FIN flag.
collectgarbage()
-- Collects the cdata.
collectgarbage()
-end))
+end
-- Compiled loop with a direct exit to the interpreter.
-test:ok(testgetmetrics.snap_restores(function()
- jit.opt.start(0, "hotloop=1")
+M.snap_restores_direct_exit = function()
+ jit.opt.start(0, 'hotloop=1')
local sum = 0
for i = 1, 20 do
@@ -91,11 +84,11 @@ test:ok(testgetmetrics.snap_restores(function()
-- A single snapshot restoration happened on loop finish.
return 1
-end))
+end
-- Compiled loop with a side exit which does not get compiled.
-test:ok(testgetmetrics.snap_restores(function()
- jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(MAXNINS))
+M.snap_restores_side_exit_not_compiled = function()
+ jit.opt.start(0, 'hotloop=1', 'hotexit=2', ('minstitch=%d'):format(MAXNINS))
local function foo(i)
-- math.fmod is not yet compiled!
@@ -112,13 +105,13 @@ test:ok(testgetmetrics.snap_restores(function()
-- Side exits from the root trace could not get compiled.
return 5
-end))
+end
-- Compiled loop with a side exit which gets compiled.
-test:ok(testgetmetrics.snap_restores(function()
+M.snap_restores_side_exit_compiled = function()
-- Optimization level is important here as `loop` optimization
-- may unroll the loop body and insert +1 side exit.
- jit.opt.start(0, "hotloop=1", "hotexit=5")
+ jit.opt.start(0, 'hotloop=1', 'hotexit=5')
local function foo(i)
return i <= 10 and i or tostring(i)
@@ -136,13 +129,13 @@ test:ok(testgetmetrics.snap_restores(function()
-- and compiled
-- 1 side exit on loop end
return 6
-end))
+end
-- Compiled scalar trace with a direct exit to the interpreter.
-test:ok(testgetmetrics.snap_restores(function()
+M.snap_restores_direct_exit_scalar = function()
-- For calls it will be 2 * hotloop (see lj_dispatch.{c,h}
-- and hotcall@vm_*.dasc).
- jit.opt.start(3, "hotloop=2", "hotexit=3")
+ jit.opt.start(3, 'hotloop=2', 'hotexit=3')
local function foo(i)
return i <= 15 and i or tostring(i)
@@ -167,15 +160,15 @@ test:ok(testgetmetrics.snap_restores(function()
jit.opt.start(unpack(jit_opt_default))
return 2
-end))
+end
-test:ok(testgetmetrics.strhash())
-
-test:ok(testgetmetrics.tracenum_base(function()
+M.tracenum_base = function()
local sum = 0
for i = 1, 200 do
sum = sum + i
end
-- Compiled only 1 loop as new trace.
return 1
-end))
+end
+
+return M
diff --git a/test/tarantool-c-tests/misclib-getmetrics-capi.test.c b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
new file mode 100644
index 00000000..202cd395
--- /dev/null
+++ b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
@@ -0,0 +1,343 @@
+#include "lua.h"
+#include "luajit.h"
+#include "lauxlib.h"
+#include "lmisclib.h"
+
+#include "test.h"
+#include "utils.h"
+
+/* Need for skipcond for BSD and JIT. */
+#include "lj_arch.h"
+
+static int base(void *test_state)
+{
+ lua_State *L = test_state;
+ struct luam_Metrics metrics;
+ luaM_metrics(L, &metrics);
+
+ /*
+ * Just check structure format, not values that fields
+ * contain.
+ */
+ (void)metrics.strhash_hit;
+ (void)metrics.strhash_miss;
+
+ (void)metrics.gc_strnum;
+ (void)metrics.gc_tabnum;
+ (void)metrics.gc_udatanum;
+ (void)metrics.gc_cdatanum;
+
+ (void)metrics.gc_total;
+ (void)metrics.gc_freed;
+ (void)metrics.gc_allocated;
+
+ (void)metrics.gc_steps_pause;
+ (void)metrics.gc_steps_propagate;
+ (void)metrics.gc_steps_atomic;
+ (void)metrics.gc_steps_sweepstring;
+ (void)metrics.gc_steps_sweep;
+ (void)metrics.gc_steps_finalize;
+
+ (void)metrics.jit_snap_restore;
+ (void)metrics.jit_trace_abort;
+ (void)metrics.jit_mcode_size;
+ (void)metrics.jit_trace_num;
+
+ return TEST_EXIT_SUCCESS;
+}
+
+static int gc_allocated_freed(void *test_state)
+{
+ lua_State *L = test_state;
+ struct luam_Metrics oldm, newm;
+ /* Force up garbage collect all dead objects. */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+
+ luaM_metrics(L, &oldm);
+ /* Simple garbage generation. */
+ if (luaL_dostring(L, "local i = 0 for j = 1, 10 do i = i + j end"))
+ bail_out("failed to translate Lua code snippet");
+ lua_gc(L, LUA_GCCOLLECT, 0);
+ luaM_metrics(L, &newm);
+ assert_true(newm.gc_allocated - oldm.gc_allocated > 0);
+ assert_true(newm.gc_freed - oldm.gc_freed > 0);
+
+ return TEST_EXIT_SUCCESS;
+}
+
+static int gc_steps(void *test_state)
+{
+ lua_State *L = test_state;
+ struct luam_Metrics oldm, newm;
+ /*
+ * Some garbage has already happened before the next line,
+ * i.e. during frontend processing Lua test chunk.
+ * Let's put a full garbage collection cycle on top
+ * of that, and confirm that non-null values are reported
+ * (we are not yet interested in actual numbers):
+ */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+
+ luaM_metrics(L, &oldm);
+ assert_true(oldm.gc_steps_pause > 0);
+ assert_true(oldm.gc_steps_propagate > 0);
+ assert_true(oldm.gc_steps_atomic > 0);
+ assert_true(oldm.gc_steps_sweepstring > 0);
+ assert_true(oldm.gc_steps_sweep > 0);
+ /* Nothing to finalize, skipped. */
+ assert_true(oldm.gc_steps_finalize == 0);
+
+ /*
+ * As long as we don't create new Lua objects
+ * consequent call should return the same values:
+ */
+ luaM_metrics(L, &newm);
+ assert_sizet_equal(newm.gc_steps_pause, oldm.gc_steps_pause);
+ assert_sizet_equal(newm.gc_steps_propagate, oldm.gc_steps_propagate);
+ assert_sizet_equal(newm.gc_steps_atomic, oldm.gc_steps_atomic);
+ assert_sizet_equal(newm.gc_steps_sweepstring,
+ oldm.gc_steps_sweepstring);
+ assert_sizet_equal(newm.gc_steps_sweep, oldm.gc_steps_sweep);
+ /* Nothing to finalize, skipped. */
+ assert_true(newm.gc_steps_finalize == 0);
+ oldm = newm;
+
+ /*
+ * Now the last phase: run full GC once and make sure that
+ * everything is being reported as expected:
+ */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+ luaM_metrics(L, &newm);
+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
+ /* Nothing to finalize, skipped. */
+ assert_true(newm.gc_steps_finalize == 0);
+ oldm = newm;
+
+ /*
+ * Now let's run three GC cycles to ensure that
+ * increment was not a lucky coincidence.
+ */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+ lua_gc(L, LUA_GCCOLLECT, 0);
+ lua_gc(L, LUA_GCCOLLECT, 0);
+ luaM_metrics(L, &newm);
+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
+ /* Nothing to finalize, skipped. */
+ assert_true(newm.gc_steps_finalize == 0);
+
+ return TEST_EXIT_SUCCESS;
+}
+
+static int objcount(void *test_state)
+{
+ lua_State *L = test_state;
+ struct luam_Metrics oldm, newm;
+ if (!LJ_HASJIT)
+ return skip("Test requires JIT enabled");
+
+ utils_get_aux_lfunc(L);
+
+ /* Force up garbage collect all dead objects. */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+
+ luaM_metrics(L, &oldm);
+ /* Generate garbage. Argument is iterations amount. */
+ lua_pushnumber(L, 1000);
+ lua_call(L, 1, 0);
+ lua_gc(L, LUA_GCCOLLECT, 0);
+ luaM_metrics(L, &newm);
+ assert_sizet_equal(newm.gc_strnum, oldm.gc_strnum);
+ assert_sizet_equal(newm.gc_tabnum, oldm.gc_tabnum);
+ assert_sizet_equal(newm.gc_udatanum, oldm.gc_udatanum);
+ assert_sizet_equal(newm.gc_cdatanum, oldm.gc_cdatanum);
+
+ return TEST_EXIT_SUCCESS;
+}
+
+static int objcount_cdata_decrement(void *test_state)
+{
+ lua_State *L = test_state;
+ /*
+ * cdata decrement test.
+ * See https://github.com/tarantool/tarantool/issues/5820.
+ */
+ struct luam_Metrics oldm, newm;
+ utils_get_aux_lfunc(L);
+
+ /* Force up garbage collect all dead objects. */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+
+ luaM_metrics(L, &oldm);
+ /*
+ * The function generates and collects cdata with
+ * LJ_GC_CDATA_FIN flag.
+ */
+ lua_call(L, 0, 0);
+ luaM_metrics(L, &newm);
+ assert_sizet_equal(newm.gc_cdatanum, oldm.gc_cdatanum);
+
+ return TEST_EXIT_SUCCESS;
+}
+
+/*
+ * Get function to call to generate the corresponding snapshot
+ * restores on top of the Lua stack. Function returns the amount
+ * of snapshot restorations expected.
+ * Clear stack after call.
+ */
+static void check_snap_restores(lua_State *L)
+{
+ struct luam_Metrics oldm, newm;
+ luaM_metrics(L, &oldm);
+ /* Generate snapshots. */
+ lua_call(L, 0, 1);
+ int n = lua_gettop(L);
+ /*
+ * The first value is the table with functions,
+ * the second is number of snapshot restores.
+ */
+ if (n != 2 || !lua_isnumber(L, -1))
+ bail_out("incorrect return value: 1 number is required");
+ size_t snap_restores = lua_tonumber(L, -1);
+ luaM_metrics(L, &newm);
+ /*
+ * Remove `snap_restores` from stack.
+ * Must be done before potiential assert and exit from
+ * the test.
+ */
+ lua_pop(L, 1);
+ assert_true(newm.jit_snap_restore - oldm.jit_snap_restore
+ == snap_restores);
+}
+
+static int snap_restores_direct_exit(void *test_state)
+{
+ lua_State *L = test_state;
+ utils_get_aux_lfunc(L);
+ check_snap_restores(L);
+ return TEST_EXIT_SUCCESS;
+}
+
+static int snap_restores_direct_exit_scalar(void *test_state)
+{
+ lua_State *L = test_state;
+ utils_get_aux_lfunc(L);
+ check_snap_restores(L);
+ return TEST_EXIT_SUCCESS;
+}
+
+static int snap_restores_side_exit_compiled(void *test_state)
+{
+ lua_State *L = test_state;
+ utils_get_aux_lfunc(L);
+ check_snap_restores(L);
+ return TEST_EXIT_SUCCESS;
+}
+
+static int snap_restores_side_exit_not_compiled(void *test_state)
+{
+ lua_State *L = test_state;
+ utils_get_aux_lfunc(L);
+ check_snap_restores(L);
+ return TEST_EXIT_SUCCESS;
+}
+
+static int snap_restores_group(void *test_state)
+{
+ if (!LJ_HASJIT)
+ return skip("Test requires JIT enabled");
+ const struct test_unit tgroup[] = {
+ test_unit_new(snap_restores_direct_exit),
+ test_unit_new(snap_restores_direct_exit_scalar),
+ test_unit_new(snap_restores_side_exit_compiled),
+ test_unit_new(snap_restores_side_exit_not_compiled)
+ };
+ return test_run_group(tgroup, test_state);
+}
+
+static int strhash(void *test_state)
+{
+ lua_State *L = test_state;
+ struct luam_Metrics oldm, newm;
+ lua_pushstring(L, "strhash_hit");
+ luaM_metrics(L, &oldm);
+ lua_pushstring(L, "strhash_hit");
+ lua_pushstring(L, "new_str");
+ luaM_metrics(L, &newm);
+ /* Remove pushed strings. */
+ lua_pop(L, 3);
+ assert_true(newm.strhash_hit - oldm.strhash_hit == 1);
+ assert_true(newm.strhash_miss - oldm.strhash_miss == 1);
+ return TEST_EXIT_SUCCESS;
+}
+
+static int tracenum_base(void *test_state)
+{
+ lua_State *L = test_state;
+ if (!LJ_HASJIT)
+ return skip("Test requires JIT enabled");
+ struct luam_Metrics metrics;
+ utils_get_aux_lfunc(L);
+
+ luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
+ /* Force up garbage collect all dead objects. */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+
+ luaM_metrics(L, &metrics);
+ assert_true(metrics.jit_trace_num == 0);
+
+ /* Generate traces. */
+ lua_call(L, 0, 1);
+ int n = lua_gettop(L);
+ /*
+ * The first value is the table with functions,
+ * the second is the amount of traces.
+ */
+ if (n != 2 || !lua_isnumber(L, -1))
+ bail_out("incorrect return value: 1 number is required");
+ size_t jit_trace_num = lua_tonumber(L, -1);
+ luaM_metrics(L, &metrics);
+ /* Remove `jit_trace_num` from Lua stack. */
+ lua_pop(L, 1);
+
+ assert_sizet_equal(metrics.jit_trace_num, jit_trace_num);
+
+ luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
+ /* Force up garbage collect all dead objects. */
+ lua_gc(L, LUA_GCCOLLECT, 0);
+ luaM_metrics(L, &metrics);
+ assert_true(metrics.jit_trace_num == 0);
+
+ return TEST_EXIT_SUCCESS;
+}
+
+int main(void)
+{
+ if (LUAJIT_OS == LUAJIT_OS_BSD)
+ return skip_all("Disabled on *BSD due to #4819");
+
+ lua_State *L = utils_lua_init();
+
+ utils_load_aux_script(L);
+ const struct test_unit tgroup[] = {
+ test_unit_new(base),
+ test_unit_new(gc_allocated_freed),
+ test_unit_new(gc_steps),
+ test_unit_new(objcount),
+ test_unit_new(objcount_cdata_decrement),
+ test_unit_new(snap_restores_group),
+ test_unit_new(strhash),
+ test_unit_new(tracenum_base)
+ };
+ const int test_result = test_run_group(tgroup, L);
+ utils_lua_close(L);
+ return test_result;
+}
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 38d6ae49..b4ce39d3 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -66,7 +66,6 @@ add_subdirectory(lj-416-xor-before-jcc)
add_subdirectory(lj-601-fix-gc-finderrfunc)
add_subdirectory(lj-727-lightuserdata-itern)
add_subdirectory(lj-flush-on-trace)
-add_subdirectory(misclib-getmetrics-capi)
add_subdirectory(misclib-sysprof-capi)
# The part of the memory profiler toolchain is located in tools
diff --git a/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt b/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
deleted file mode 100644
index 60eb5bbb..00000000
--- a/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
+++ /dev/null
@@ -1 +0,0 @@
-BuildTestCLib(testgetmetrics testgetmetrics.c)
diff --git a/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c b/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
deleted file mode 100644
index 67776338..00000000
--- a/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
+++ /dev/null
@@ -1,270 +0,0 @@
-#include <lua.h>
-#include <luajit.h>
-#include <lauxlib.h>
-
-#include <lmisclib.h>
-
-#undef NDEBUG
-#include <assert.h>
-
-static int base(lua_State *L)
-{
- struct luam_Metrics metrics;
- luaM_metrics(L, &metrics);
-
- /* Just check structure format, not values that fields contain. */
- (void)metrics.strhash_hit;
- (void)metrics.strhash_miss;
-
- (void)metrics.gc_strnum;
- (void)metrics.gc_tabnum;
- (void)metrics.gc_udatanum;
- (void)metrics.gc_cdatanum;
-
- (void)metrics.gc_total;
- (void)metrics.gc_freed;
- (void)metrics.gc_allocated;
-
- (void)metrics.gc_steps_pause;
- (void)metrics.gc_steps_propagate;
- (void)metrics.gc_steps_atomic;
- (void)metrics.gc_steps_sweepstring;
- (void)metrics.gc_steps_sweep;
- (void)metrics.gc_steps_finalize;
-
- (void)metrics.jit_snap_restore;
- (void)metrics.jit_trace_abort;
- (void)metrics.jit_mcode_size;
- (void)metrics.jit_trace_num;
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int gc_allocated_freed(lua_State *L)
-{
- struct luam_Metrics oldm, newm;
- /* Force up garbage collect all dead objects. */
- lua_gc(L, LUA_GCCOLLECT, 0);
-
- luaM_metrics(L, &oldm);
- /* Simple garbage generation. */
- if (luaL_dostring(L, "local i = 0 for j = 1, 10 do i = i + j end"))
- luaL_error(L, "failed to translate Lua code snippet");
- lua_gc(L, LUA_GCCOLLECT, 0);
- luaM_metrics(L, &newm);
- assert(newm.gc_allocated - oldm.gc_allocated > 0);
- assert(newm.gc_freed - oldm.gc_freed > 0);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int gc_steps(lua_State *L)
-{
- struct luam_Metrics oldm, newm;
- /*
- * Some garbage has already happened before the next line,
- * i.e. during frontend processing Lua test chunk.
- * Let's put a full garbage collection cycle on top
- * of that, and confirm that non-null values are reported
- * (we are not yet interested in actual numbers):
- */
- lua_gc(L, LUA_GCCOLLECT, 0);
-
- luaM_metrics(L, &oldm);
- assert(oldm.gc_steps_pause > 0);
- assert(oldm.gc_steps_propagate > 0);
- assert(oldm.gc_steps_atomic > 0);
- assert(oldm.gc_steps_sweepstring > 0);
- assert(oldm.gc_steps_sweep > 0);
- /* Nothing to finalize, skipped. */
- assert(oldm.gc_steps_finalize == 0);
-
- /*
- * As long as we don't create new Lua objects
- * consequent call should return the same values:
- */
- luaM_metrics(L, &newm);
- assert(newm.gc_steps_pause - oldm.gc_steps_pause == 0);
- assert(newm.gc_steps_propagate - oldm.gc_steps_propagate == 0);
- assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 0);
- assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring == 0);
- assert(newm.gc_steps_sweep - oldm.gc_steps_sweep == 0);
- /* Nothing to finalize, skipped. */
- assert(newm.gc_steps_finalize == 0);
- oldm = newm;
-
- /*
- * Now the last phase: run full GC once and make sure that
- * everything is being reported as expected:
- */
- lua_gc(L, LUA_GCCOLLECT, 0);
- luaM_metrics(L, &newm);
- assert(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
- assert(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
- assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
- assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
- assert(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
- /* Nothing to finalize, skipped. */
- assert(newm.gc_steps_finalize == 0);
- oldm = newm;
-
- /*
- * Now let's run three GC cycles to ensure that
- * increment was not a lucky coincidence.
- */
- lua_gc(L, LUA_GCCOLLECT, 0);
- lua_gc(L, LUA_GCCOLLECT, 0);
- lua_gc(L, LUA_GCCOLLECT, 0);
- luaM_metrics(L, &newm);
- assert(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
- assert(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
- assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
- assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
- assert(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
- /* Nothing to finalize, skipped. */
- assert(newm.gc_steps_finalize == 0);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int objcount(lua_State *L)
-{
- struct luam_Metrics oldm, newm;
- int n = lua_gettop(L);
- if (n != 1 || !lua_isfunction(L, 1))
- luaL_error(L, "incorrect argument: 1 function is required");
-
- /* Force up garbage collect all dead objects. */
- lua_gc(L, LUA_GCCOLLECT, 0);
-
- luaM_metrics(L, &oldm);
- /* Generate garbage. Argument is iterations amount. */
- lua_pushnumber(L, 1000);
- lua_call(L, 1, 0);
- lua_gc(L, LUA_GCCOLLECT, 0);
- luaM_metrics(L, &newm);
- assert(newm.gc_strnum - oldm.gc_strnum == 0);
- assert(newm.gc_tabnum - oldm.gc_tabnum == 0);
- assert(newm.gc_udatanum - oldm.gc_udatanum == 0);
- assert(newm.gc_cdatanum - oldm.gc_cdatanum == 0);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int objcount_cdata_decrement(lua_State *L)
-{
- /*
- * cdata decrement test.
- * See https://github.com/tarantool/tarantool/issues/5820.
- */
- struct luam_Metrics oldm, newm;
- int n = lua_gettop(L);
- if (n != 1 || !lua_isfunction(L, 1))
- luaL_error(L, "incorrect argument: 1 function is required");
-
- /* Force up garbage collect all dead objects. */
- lua_gc(L, LUA_GCCOLLECT, 0);
-
- luaM_metrics(L, &oldm);
- /*
- * The function generates and collects cdata with
- * LJ_GC_CDATA_FIN flag.
- */
- lua_call(L, 0, 0);
- luaM_metrics(L, &newm);
- assert(newm.gc_cdatanum - oldm.gc_cdatanum == 0);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int snap_restores(lua_State *L)
-{
- struct luam_Metrics oldm, newm;
- int n = lua_gettop(L);
- if (n != 1 || !lua_isfunction(L, 1))
- luaL_error(L, "incorrect arguments: 1 function is required");
-
- luaM_metrics(L, &oldm);
- /* Generate snapshots. */
- lua_call(L, 0, 1);
- n = lua_gettop(L);
- if (n != 1 || !lua_isnumber(L, 1))
- luaL_error(L, "incorrect return value: 1 number is required");
- size_t snap_restores = lua_tonumber(L, 1);
- luaM_metrics(L, &newm);
- assert(newm.jit_snap_restore - oldm.jit_snap_restore == snap_restores);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int strhash(lua_State *L)
-{
- struct luam_Metrics oldm, newm;
- lua_pushstring(L, "strhash_hit");
- luaM_metrics(L, &oldm);
- lua_pushstring(L, "strhash_hit");
- lua_pushstring(L, "new_str");
- luaM_metrics(L, &newm);
- assert(newm.strhash_hit - oldm.strhash_hit == 1);
- assert(newm.strhash_miss - oldm.strhash_miss == 1);
- lua_pop(L, 3);
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int tracenum_base(lua_State *L)
-{
- struct luam_Metrics metrics;
- int n = lua_gettop(L);
- if (n != 1 || !lua_isfunction(L, 1))
- luaL_error(L, "incorrect arguments: 1 function is required");
-
- luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
- /* Force up garbage collect all dead objects. */
- lua_gc(L, LUA_GCCOLLECT, 0);
-
- luaM_metrics(L, &metrics);
- assert(metrics.jit_trace_num == 0);
-
- /* Generate traces. */
- lua_call(L, 0, 1);
- n = lua_gettop(L);
- if (n != 1 || !lua_isnumber(L, 1))
- luaL_error(L, "incorrect return value: 1 number is required");
- size_t jit_trace_num = lua_tonumber(L, 1);
- luaM_metrics(L, &metrics);
- assert(metrics.jit_trace_num == jit_trace_num);
-
- luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
- /* Force up garbage collect all dead objects. */
- lua_gc(L, LUA_GCCOLLECT, 0);
- luaM_metrics(L, &metrics);
- assert(metrics.jit_trace_num == 0);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static const struct luaL_Reg testgetmetrics[] = {
- {"base", base},
- {"gc_allocated_freed", gc_allocated_freed},
- {"gc_steps", gc_steps},
- {"objcount", objcount},
- {"objcount_cdata_decrement", objcount_cdata_decrement},
- {"snap_restores", snap_restores},
- {"strhash", strhash},
- {"tracenum_base", tracenum_base},
- {NULL, NULL}
-};
-
-LUA_API int luaopen_testgetmetrics(lua_State *L)
-{
- luaL_register(L, "testgetmetrics", testgetmetrics);
- return 1;
-}
--
2.34.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C Sergey Kaplun via Tarantool-patches
@ 2023-05-19 12:17 ` Maxim Kokryashkin via Tarantool-patches
2023-05-20 8:08 ` Sergey Kaplun via Tarantool-patches
2023-05-29 16:15 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 1 reply; 29+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-19 12:17 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 25179 bytes --]
Hi, Sergey
Thanks for the patch.
LGTM, except for a few comments below.
>
>>This patch rewrites the aforementioned test with the usage libtest,
>>recently introduced. Since we still stand in need of a Lua helper script
>>for generation of traces, the original test file is reworked as a
>>standalone script, which returns the table with helper functions. Each
>>helper function is named the same as the test where it will be used. Now
>>single quotes are used according to our Lua code style.
>>
>>In C part all asserts from glibc are replaced with the corresponding
>>assert_{cond} from the libtest. Now tests return `TEST_EXIT_SUCCESS` at
>>the finish. Also, the stack check for the amount of return results from
>>the helper function is slightly changed, since there is one more stack
>>slot in use (table with these functions). `snap_restores()` C function
>>duplicates 4 times for each subtest. Common helper
>>`check_snap_restores()` is used for each of them. Each error throwing is
>>replaced with `bail_out()` call.
>>
>>NB: `lua_pop()` to clear the Lua stack after a call should be done before
>>any possible assertion, which would exit from the test leaving the stack
>>uncleaned.
>>
>>All skipconds use macros defined in <lj_arch.h>, so it is included in
>>the test. As far as this test initializes the LuaJIT VM manually, there
>>is no need to check `jit.status()` result; checking `LJ_HASJIT` is
>>enough. Now, only JIT-related tests are skipped, when compiled without
>>JIT. Nevertheless, all tests are skipped for *BSD arches.
>>
>>Also, this patch sets the new CMake variable named `LUAJIT_LIBRARY`
>>equal to `LUAJIT_LIB` in `PARENT_SCOPE` to be used in tarantool-c-tests
>>linking.
>>
>>Also, .c_test suffix is added to the <.gitignore>.
>>
>>Part of tarantool/tarantool#7900
>>---
>> .gitignore | 1 +
>> src/CMakeLists.txt | 2 +
>> test/tarantool-c-tests/CMakeLists.txt | 32 +-
>> .../misclib-getmetrics-capi-script.lua} | 83 ++---
>> .../misclib-getmetrics-capi.test.c | 343 ++++++++++++++++++
>> test/tarantool-tests/CMakeLists.txt | 1 -
>> .../misclib-getmetrics-capi/CMakeLists.txt | 1 -
>> .../misclib-getmetrics-capi/testgetmetrics.c | 270 --------------
>> 8 files changed, 412 insertions(+), 321 deletions(-)
>> rename test/{tarantool-tests/misclib-getmetrics-capi.test.lua => tarantool-c-tests/misclib-getmetrics-capi-script.lua} (68%)
>> create mode 100644 test/tarantool-c-tests/misclib-getmetrics-capi.test.c
>> delete mode 100644 test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
>> delete mode 100644 test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
>>
>>diff --git a/.gitignore b/.gitignore
>>index b7908aee..dc5ea5fc 100644
>>--- a/.gitignore
>>+++ b/.gitignore
>>@@ -24,3 +24,4 @@ install_manifest.txt
>> luajit-parse-memprof
>> luajit-parse-sysprof
>> luajit.pc
>>+*.c_test
>>diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
>>index dffc0a4d..fed4c38d 100644
>>--- a/src/CMakeLists.txt
>>+++ b/src/CMakeLists.txt
>>@@ -325,6 +325,8 @@ if(NOT BUILDMODE STREQUAL "dynamic")
>> set(LUAJIT_BIN luajit_static)
>> set(LUAJIT_LIB libluajit_static)
>> endif()
>>+# Need for the test linking, so the PARENT_SCOPE option is used.
>>+set(LUAJIT_LIBRARY ${LUAJIT_LIB} PARENT_SCOPE)
>> set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})
>>
>> add_executable(${LUAJIT_BIN} EXCLUDE_FROM_ALL ${CLI_SOURCES})
>>diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
>>index c6b7cd30..c9d75d6c 100644
>>--- a/test/tarantool-c-tests/CMakeLists.txt
>>+++ b/test/tarantool-c-tests/CMakeLists.txt
>>@@ -22,13 +22,37 @@ set_target_properties(libtest PROPERTIES
>> LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
>> )
>>
>>-# XXX: For now, just build libtest. The tests to be depended on
>>-# will be added in the next commit.
>>+# TARGET_C_FLAGS is required here to be sure that headers like
>>+# lj_arch.h in compiled test are consistent with the LuaJIT library
>>+# to link.
>>+AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
>>+
>>+set(CTEST_SRC_SUFFIX ".test.c")
>>+file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
>>+foreach(test_source ${tests})
>>+ string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe ${test_source})
>Please drop a comment explaining the line above.
>
><snipped>
>>
>>diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
>>similarity index 68%
>>rename from test/tarantool-tests/misclib-getmetrics-capi.test.lua
>>rename to test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
>>index 654e5545..2f0ee5cf 100644
>>--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
>>+++ b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
><snipped>
>>diff --git a/test/tarantool-c-tests/misclib-getmetrics-capi.test.c b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
>>new file mode 100644
>>index 00000000..202cd395
>>--- /dev/null
>>+++ b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
>>@@ -0,0 +1,343 @@
>>+#include "lua.h"
>>+#include "luajit.h"
>>+#include "lauxlib.h"
>>+#include "lmisclib.h"
>>+
>>+#include "test.h"
>>+#include "utils.h"
>>+
>>+/* Need for skipcond for BSD and JIT. */
>>+#include "lj_arch.h"
>>+
>>+static int base(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ struct luam_Metrics metrics;
>>+ luaM_metrics(L, &metrics);
>>+
>>+ /*
>>+ * Just check structure format, not values that fields
>>+ * contain.
>>+ */
>>+ (void)metrics.strhash_hit;
>>+ (void)metrics.strhash_miss;
>>+
>>+ (void)metrics.gc_strnum;
>>+ (void)metrics.gc_tabnum;
>>+ (void)metrics.gc_udatanum;
>>+ (void)metrics.gc_cdatanum;
>>+
>>+ (void)metrics.gc_total;
>>+ (void)metrics.gc_freed;
>>+ (void)metrics.gc_allocated;
>>+
>>+ (void)metrics.gc_steps_pause;
>>+ (void)metrics.gc_steps_propagate;
>>+ (void)metrics.gc_steps_atomic;
>>+ (void)metrics.gc_steps_sweepstring;
>>+ (void)metrics.gc_steps_sweep;
>>+ (void)metrics.gc_steps_finalize;
>>+
>>+ (void)metrics.jit_snap_restore;
>>+ (void)metrics.jit_trace_abort;
>>+ (void)metrics.jit_mcode_size;
>>+ (void)metrics.jit_trace_num;
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int gc_allocated_freed(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ struct luam_Metrics oldm, newm;
>>+ /* Force up garbage collect all dead objects. */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+
>>+ luaM_metrics(L, &oldm);
>>+ /* Simple garbage generation. */
>>+ if (luaL_dostring(L, "local i = 0 for j = 1, 10 do i = i + j end"))
>>+ bail_out("failed to translate Lua code snippet");
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+ luaM_metrics(L, &newm);
>>+ assert_true(newm.gc_allocated - oldm.gc_allocated > 0);
>>+ assert_true(newm.gc_freed - oldm.gc_freed > 0);
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int gc_steps(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ struct luam_Metrics oldm, newm;
>>+ /*
>>+ * Some garbage has already happened before the next line,
>>+ * i.e. during frontend processing Lua test chunk.
>>+ * Let's put a full garbage collection cycle on top
>>+ * of that, and confirm that non-null values are reported
>>+ * (we are not yet interested in actual numbers):
>>+ */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+
>>+ luaM_metrics(L, &oldm);
>>+ assert_true(oldm.gc_steps_pause > 0);
>>+ assert_true(oldm.gc_steps_propagate > 0);
>>+ assert_true(oldm.gc_steps_atomic > 0);
>>+ assert_true(oldm.gc_steps_sweepstring > 0);
>>+ assert_true(oldm.gc_steps_sweep > 0);
>>+ /* Nothing to finalize, skipped. */
>>+ assert_true(oldm.gc_steps_finalize == 0);
>>+
>>+ /*
>>+ * As long as we don't create new Lua objects
>>+ * consequent call should return the same values:
>>+ */
>>+ luaM_metrics(L, &newm);
>>+ assert_sizet_equal(newm.gc_steps_pause, oldm.gc_steps_pause);
>>+ assert_sizet_equal(newm.gc_steps_propagate, oldm.gc_steps_propagate);
>>+ assert_sizet_equal(newm.gc_steps_atomic, oldm.gc_steps_atomic);
>>+ assert_sizet_equal(newm.gc_steps_sweepstring,
>>+ oldm.gc_steps_sweepstring);
>>+ assert_sizet_equal(newm.gc_steps_sweep, oldm.gc_steps_sweep);
>>+ /* Nothing to finalize, skipped. */
>>+ assert_true(newm.gc_steps_finalize == 0);
>>+ oldm = newm;
>>+
>>+ /*
>>+ * Now the last phase: run full GC once and make sure that
>>+ * everything is being reported as expected:
>>+ */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+ luaM_metrics(L, &newm);
>>+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
>>+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
>>+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
>>+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
>>+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
>>+ /* Nothing to finalize, skipped. */
>>+ assert_true(newm.gc_steps_finalize == 0);
>>+ oldm = newm;
>>+
>>+ /*
>>+ * Now let's run three GC cycles to ensure that
>>+ * increment was not a lucky coincidence.
>>+ */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>I think it is better to create a for loop here.
>>+ luaM_metrics(L, &newm);
>>+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
>>+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
>>+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
>>+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
>>+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
>That part is duplicated. It is better to either write a corresponding macro, or
>a function.
>>+ /* Nothing to finalize, skipped. */
>>+ assert_true(newm.gc_steps_finalize == 0);
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int objcount(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ struct luam_Metrics oldm, newm;
>>+ if (!LJ_HASJIT)
>>+ return skip("Test requires JIT enabled");
>>+
>>+ utils_get_aux_lfunc(L);
>>+
>>+ /* Force up garbage collect all dead objects. */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+
>>+ luaM_metrics(L, &oldm);
>>+ /* Generate garbage. Argument is iterations amount. */
>>+ lua_pushnumber(L, 1000);
>>+ lua_call(L, 1, 0);
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+ luaM_metrics(L, &newm);
>>+ assert_sizet_equal(newm.gc_strnum, oldm.gc_strnum);
>>+ assert_sizet_equal(newm.gc_tabnum, oldm.gc_tabnum);
>>+ assert_sizet_equal(newm.gc_udatanum, oldm.gc_udatanum);
>>+ assert_sizet_equal(newm.gc_cdatanum, oldm.gc_cdatanum);
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int objcount_cdata_decrement(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ /*
>>+ * cdata decrement test.
>>+ * See https://github.com/tarantool/tarantool/issues/5820 .
>>+ */
>>+ struct luam_Metrics oldm, newm;
>>+ utils_get_aux_lfunc(L);
>>+
>>+ /* Force up garbage collect all dead objects. */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+
>>+ luaM_metrics(L, &oldm);
>>+ /*
>>+ * The function generates and collects cdata with
>>+ * LJ_GC_CDATA_FIN flag.
>>+ */
>>+ lua_call(L, 0, 0);
>>+ luaM_metrics(L, &newm);
>>+ assert_sizet_equal(newm.gc_cdatanum, oldm.gc_cdatanum);
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+/*
>>+ * Get function to call to generate the corresponding snapshot
>>+ * restores on top of the Lua stack. Function returns the amount
>>+ * of snapshot restorations expected.
>>+ * Clear stack after call.
>>+ */
>>+static void check_snap_restores(lua_State *L)
>>+{
>>+ struct luam_Metrics oldm, newm;
>>+ luaM_metrics(L, &oldm);
>>+ /* Generate snapshots. */
>>+ lua_call(L, 0, 1);
>>+ int n = lua_gettop(L);
>>+ /*
>>+ * The first value is the table with functions,
>>+ * the second is number of snapshot restores.
>>+ */
>>+ if (n != 2 || !lua_isnumber(L, -1))
>>+ bail_out("incorrect return value: 1 number is required");
>>+ size_t snap_restores = lua_tonumber(L, -1);
>>+ luaM_metrics(L, &newm);
>>+ /*
>>+ * Remove `snap_restores` from stack.
>>+ * Must be done before potiential assert and exit from
>>+ * the test.
>>+ */
>>+ lua_pop(L, 1);
>>+ assert_true(newm.jit_snap_restore - oldm.jit_snap_restore
>>+ == snap_restores);
>>+}
>>+
>>+static int snap_restores_direct_exit(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ utils_get_aux_lfunc(L);
>>+ check_snap_restores(L);
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int snap_restores_direct_exit_scalar(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ utils_get_aux_lfunc(L);
>>+ check_snap_restores(L);
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int snap_restores_side_exit_compiled(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ utils_get_aux_lfunc(L);
>>+ check_snap_restores(L);
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int snap_restores_side_exit_not_compiled(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ utils_get_aux_lfunc(L);
>>+ check_snap_restores(L);
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>Same here. Either a dedicated implementation func, or a macro is needed here.
>>+static int snap_restores_group(void *test_state)
>>+{
>>+ if (!LJ_HASJIT)
>>+ return skip("Test requires JIT enabled");
>>+ const struct test_unit tgroup[] = {
>>+ test_unit_new(snap_restores_direct_exit),
>>+ test_unit_new(snap_restores_direct_exit_scalar),
>>+ test_unit_new(snap_restores_side_exit_compiled),
>>+ test_unit_new(snap_restores_side_exit_not_compiled)
>>+ };
>>+ return test_run_group(tgroup, test_state);
>>+}
>>+
>>+static int strhash(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ struct luam_Metrics oldm, newm;
>>+ lua_pushstring(L, "strhash_hit");
>>+ luaM_metrics(L, &oldm);
>>+ lua_pushstring(L, "strhash_hit");
>>+ lua_pushstring(L, "new_str");
>>+ luaM_metrics(L, &newm);
>>+ /* Remove pushed strings. */
>>+ lua_pop(L, 3);
>>+ assert_true(newm.strhash_hit - oldm.strhash_hit == 1);
>>+ assert_true(newm.strhash_miss - oldm.strhash_miss == 1);
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int tracenum_base(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ if (!LJ_HASJIT)
>>+ return skip("Test requires JIT enabled");
>>+ struct luam_Metrics metrics;
>>+ utils_get_aux_lfunc(L);
>>+
>>+ luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
>>+ /* Force up garbage collect all dead objects. */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+
>>+ luaM_metrics(L, &metrics);
>>+ assert_true(metrics.jit_trace_num == 0);
>>+
>>+ /* Generate traces. */
>>+ lua_call(L, 0, 1);
>>+ int n = lua_gettop(L);
>>+ /*
>>+ * The first value is the table with functions,
>>+ * the second is the amount of traces.
>>+ */
>>+ if (n != 2 || !lua_isnumber(L, -1))
>>+ bail_out("incorrect return value: 1 number is required");
>>+ size_t jit_trace_num = lua_tonumber(L, -1);
>>+ luaM_metrics(L, &metrics);
>>+ /* Remove `jit_trace_num` from Lua stack. */
>>+ lua_pop(L, 1);
>>+
>>+ assert_sizet_equal(metrics.jit_trace_num, jit_trace_num);
>>+
>>+ luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
>>+ /* Force up garbage collect all dead objects. */
>>+ lua_gc(L, LUA_GCCOLLECT, 0);
>>+ luaM_metrics(L, &metrics);
>>+ assert_true(metrics.jit_trace_num == 0);
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+int main(void)
>>+{
>>+ if (LUAJIT_OS == LUAJIT_OS_BSD)
>>+ return skip_all("Disabled on *BSD due to #4819");
>>+
>>+ lua_State *L = utils_lua_init();
>>+
>>+ utils_load_aux_script(L);
>>+ const struct test_unit tgroup[] = {
>>+ test_unit_new(base),
>>+ test_unit_new(gc_allocated_freed),
>>+ test_unit_new(gc_steps),
>>+ test_unit_new(objcount),
>>+ test_unit_new(objcount_cdata_decrement),
>>+ test_unit_new(snap_restores_group),
>>+ test_unit_new(strhash),
>>+ test_unit_new(tracenum_base)
>>+ };
>>+ const int test_result = test_run_group(tgroup, L);
>>+ utils_lua_close(L);
>>+ return test_result;
>>+}
>>diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>>index 38d6ae49..b4ce39d3 100644
>>--- a/test/tarantool-tests/CMakeLists.txt
>>+++ b/test/tarantool-tests/CMakeLists.txt
>>@@ -66,7 +66,6 @@ add_subdirectory(lj-416-xor-before-jcc)
>> add_subdirectory(lj-601-fix-gc-finderrfunc)
>> add_subdirectory(lj-727-lightuserdata-itern)
>> add_subdirectory(lj-flush-on-trace)
>>-add_subdirectory(misclib-getmetrics-capi)
>> add_subdirectory(misclib-sysprof-capi)
>>
>> # The part of the memory profiler toolchain is located in tools
>>diff --git a/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt b/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
>>deleted file mode 100644
>>index 60eb5bbb..00000000
>>--- a/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
>>+++ /dev/null
>>@@ -1 +0,0 @@
>>-BuildTestCLib(testgetmetrics testgetmetrics.c)
>>diff --git a/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c b/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
>>deleted file mode 100644
>>index 67776338..00000000
>>--- a/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
>>+++ /dev/null
>>@@ -1,270 +0,0 @@
>>-#include <lua.h>
>>-#include <luajit.h>
>>-#include <lauxlib.h>
>>-
>>-#include <lmisclib.h>
>>-
>>-#undef NDEBUG
>>-#include <assert.h>
>>-
>>-static int base(lua_State *L)
>>-{
>>- struct luam_Metrics metrics;
>>- luaM_metrics(L, &metrics);
>>-
>>- /* Just check structure format, not values that fields contain. */
>>- (void)metrics.strhash_hit;
>>- (void)metrics.strhash_miss;
>>-
>>- (void)metrics.gc_strnum;
>>- (void)metrics.gc_tabnum;
>>- (void)metrics.gc_udatanum;
>>- (void)metrics.gc_cdatanum;
>>-
>>- (void)metrics.gc_total;
>>- (void)metrics.gc_freed;
>>- (void)metrics.gc_allocated;
>>-
>>- (void)metrics.gc_steps_pause;
>>- (void)metrics.gc_steps_propagate;
>>- (void)metrics.gc_steps_atomic;
>>- (void)metrics.gc_steps_sweepstring;
>>- (void)metrics.gc_steps_sweep;
>>- (void)metrics.gc_steps_finalize;
>>-
>>- (void)metrics.jit_snap_restore;
>>- (void)metrics.jit_trace_abort;
>>- (void)metrics.jit_mcode_size;
>>- (void)metrics.jit_trace_num;
>>-
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static int gc_allocated_freed(lua_State *L)
>>-{
>>- struct luam_Metrics oldm, newm;
>>- /* Force up garbage collect all dead objects. */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>-
>>- luaM_metrics(L, &oldm);
>>- /* Simple garbage generation. */
>>- if (luaL_dostring(L, "local i = 0 for j = 1, 10 do i = i + j end"))
>>- luaL_error(L, "failed to translate Lua code snippet");
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>- luaM_metrics(L, &newm);
>>- assert(newm.gc_allocated - oldm.gc_allocated > 0);
>>- assert(newm.gc_freed - oldm.gc_freed > 0);
>>-
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static int gc_steps(lua_State *L)
>>-{
>>- struct luam_Metrics oldm, newm;
>>- /*
>>- * Some garbage has already happened before the next line,
>>- * i.e. during frontend processing Lua test chunk.
>>- * Let's put a full garbage collection cycle on top
>>- * of that, and confirm that non-null values are reported
>>- * (we are not yet interested in actual numbers):
>>- */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>-
>>- luaM_metrics(L, &oldm);
>>- assert(oldm.gc_steps_pause > 0);
>>- assert(oldm.gc_steps_propagate > 0);
>>- assert(oldm.gc_steps_atomic > 0);
>>- assert(oldm.gc_steps_sweepstring > 0);
>>- assert(oldm.gc_steps_sweep > 0);
>>- /* Nothing to finalize, skipped. */
>>- assert(oldm.gc_steps_finalize == 0);
>>-
>>- /*
>>- * As long as we don't create new Lua objects
>>- * consequent call should return the same values:
>>- */
>>- luaM_metrics(L, &newm);
>>- assert(newm.gc_steps_pause - oldm.gc_steps_pause == 0);
>>- assert(newm.gc_steps_propagate - oldm.gc_steps_propagate == 0);
>>- assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 0);
>>- assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring == 0);
>>- assert(newm.gc_steps_sweep - oldm.gc_steps_sweep == 0);
>>- /* Nothing to finalize, skipped. */
>>- assert(newm.gc_steps_finalize == 0);
>>- oldm = newm;
>>-
>>- /*
>>- * Now the last phase: run full GC once and make sure that
>>- * everything is being reported as expected:
>>- */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>- luaM_metrics(L, &newm);
>>- assert(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
>>- assert(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
>>- assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
>>- assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
>>- assert(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
>>- /* Nothing to finalize, skipped. */
>>- assert(newm.gc_steps_finalize == 0);
>>- oldm = newm;
>>-
>>- /*
>>- * Now let's run three GC cycles to ensure that
>>- * increment was not a lucky coincidence.
>>- */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>- luaM_metrics(L, &newm);
>>- assert(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
>>- assert(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
>>- assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
>>- assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
>>- assert(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
>>- /* Nothing to finalize, skipped. */
>>- assert(newm.gc_steps_finalize == 0);
>>-
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static int objcount(lua_State *L)
>>-{
>>- struct luam_Metrics oldm, newm;
>>- int n = lua_gettop(L);
>>- if (n != 1 || !lua_isfunction(L, 1))
>>- luaL_error(L, "incorrect argument: 1 function is required");
>>-
>>- /* Force up garbage collect all dead objects. */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>-
>>- luaM_metrics(L, &oldm);
>>- /* Generate garbage. Argument is iterations amount. */
>>- lua_pushnumber(L, 1000);
>>- lua_call(L, 1, 0);
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>- luaM_metrics(L, &newm);
>>- assert(newm.gc_strnum - oldm.gc_strnum == 0);
>>- assert(newm.gc_tabnum - oldm.gc_tabnum == 0);
>>- assert(newm.gc_udatanum - oldm.gc_udatanum == 0);
>>- assert(newm.gc_cdatanum - oldm.gc_cdatanum == 0);
>>-
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static int objcount_cdata_decrement(lua_State *L)
>>-{
>>- /*
>>- * cdata decrement test.
>>- * See https://github.com/tarantool/tarantool/issues/5820 .
>>- */
>>- struct luam_Metrics oldm, newm;
>>- int n = lua_gettop(L);
>>- if (n != 1 || !lua_isfunction(L, 1))
>>- luaL_error(L, "incorrect argument: 1 function is required");
>>-
>>- /* Force up garbage collect all dead objects. */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>-
>>- luaM_metrics(L, &oldm);
>>- /*
>>- * The function generates and collects cdata with
>>- * LJ_GC_CDATA_FIN flag.
>>- */
>>- lua_call(L, 0, 0);
>>- luaM_metrics(L, &newm);
>>- assert(newm.gc_cdatanum - oldm.gc_cdatanum == 0);
>>-
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static int snap_restores(lua_State *L)
>>-{
>>- struct luam_Metrics oldm, newm;
>>- int n = lua_gettop(L);
>>- if (n != 1 || !lua_isfunction(L, 1))
>>- luaL_error(L, "incorrect arguments: 1 function is required");
>>-
>>- luaM_metrics(L, &oldm);
>>- /* Generate snapshots. */
>>- lua_call(L, 0, 1);
>>- n = lua_gettop(L);
>>- if (n != 1 || !lua_isnumber(L, 1))
>>- luaL_error(L, "incorrect return value: 1 number is required");
>>- size_t snap_restores = lua_tonumber(L, 1);
>>- luaM_metrics(L, &newm);
>>- assert(newm.jit_snap_restore - oldm.jit_snap_restore == snap_restores);
>>-
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static int strhash(lua_State *L)
>>-{
>>- struct luam_Metrics oldm, newm;
>>- lua_pushstring(L, "strhash_hit");
>>- luaM_metrics(L, &oldm);
>>- lua_pushstring(L, "strhash_hit");
>>- lua_pushstring(L, "new_str");
>>- luaM_metrics(L, &newm);
>>- assert(newm.strhash_hit - oldm.strhash_hit == 1);
>>- assert(newm.strhash_miss - oldm.strhash_miss == 1);
>>- lua_pop(L, 3);
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static int tracenum_base(lua_State *L)
>>-{
>>- struct luam_Metrics metrics;
>>- int n = lua_gettop(L);
>>- if (n != 1 || !lua_isfunction(L, 1))
>>- luaL_error(L, "incorrect arguments: 1 function is required");
>>-
>>- luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
>>- /* Force up garbage collect all dead objects. */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>-
>>- luaM_metrics(L, &metrics);
>>- assert(metrics.jit_trace_num == 0);
>>-
>>- /* Generate traces. */
>>- lua_call(L, 0, 1);
>>- n = lua_gettop(L);
>>- if (n != 1 || !lua_isnumber(L, 1))
>>- luaL_error(L, "incorrect return value: 1 number is required");
>>- size_t jit_trace_num = lua_tonumber(L, 1);
>>- luaM_metrics(L, &metrics);
>>- assert(metrics.jit_trace_num == jit_trace_num);
>>-
>>- luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
>>- /* Force up garbage collect all dead objects. */
>>- lua_gc(L, LUA_GCCOLLECT, 0);
>>- luaM_metrics(L, &metrics);
>>- assert(metrics.jit_trace_num == 0);
>>-
>>- lua_pushboolean(L, 1);
>>- return 1;
>>-}
>>-
>>-static const struct luaL_Reg testgetmetrics[] = {
>>- {"base", base},
>>- {"gc_allocated_freed", gc_allocated_freed},
>>- {"gc_steps", gc_steps},
>>- {"objcount", objcount},
>>- {"objcount_cdata_decrement", objcount_cdata_decrement},
>>- {"snap_restores", snap_restores},
>>- {"strhash", strhash},
>>- {"tracenum_base", tracenum_base},
>>- {NULL, NULL}
>>-};
>>-
>>-LUA_API int luaopen_testgetmetrics(lua_State *L)
>>-{
>>- luaL_register(L, "testgetmetrics", testgetmetrics);
>>- return 1;
>>-}
>>--
>>2.34.1
>--
>Best regards,
>Maxim Kokryashkin
>
[-- Attachment #2: Type: text/html, Size: 28042 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C
2023-05-19 12:17 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-20 8:08 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-20 8:08 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Maxim!
Thanks for the review!
See my answers below.
On 19.05.23, Maxim Kokryashkin wrote:
>
> Hi, Sergey
> Thanks for the patch.
> LGTM, except for a few comments below.
>
> >
<snipped>
> >>diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> >>index c6b7cd30..c9d75d6c 100644
> >>--- a/test/tarantool-c-tests/CMakeLists.txt
> >>+++ b/test/tarantool-c-tests/CMakeLists.txt
> >>@@ -22,13 +22,37 @@ set_target_properties(libtest PROPERTIES
> >> LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
> >> )
> >>
> >>-# XXX: For now, just build libtest. The tests to be depended on
> >>-# will be added in the next commit.
> >>+# TARGET_C_FLAGS is required here to be sure that headers like
> >>+# lj_arch.h in compiled test are consistent with the LuaJIT library
> >>+# to link.
> >>+AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
> >>+
> >>+set(CTEST_SRC_SUFFIX ".test.c")
> >>+file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
> >>+foreach(test_source ${tests})
> >>+ string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe ${test_source})
> >Please drop a comment explaining the line above.
Added the following comment.
Branch is force-pushed.
===================================================================
diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index c9d75d6c..20495386 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -30,6 +30,7 @@ AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
set(CTEST_SRC_SUFFIX ".test.c")
file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
foreach(test_source ${tests})
+ # Get test name without suffix. Needed to set OUTPUT_NAME.
string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe ${test_source})
add_executable(${exe} EXCLUDE_FROM_ALL ${test_source})
target_include_directories(${exe} PRIVATE
===================================================================
> >
> ><snipped>
> >>
> >>diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> >>similarity index 68%
> >>rename from test/tarantool-tests/misclib-getmetrics-capi.test.lua
> >>rename to test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> >>index 654e5545..2f0ee5cf 100644
> >>--- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> >>+++ b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> ><snipped>
> >>diff --git a/test/tarantool-c-tests/misclib-getmetrics-capi.test.c b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
> >>new file mode 100644
> >>index 00000000..202cd395
> >>--- /dev/null
> >>+++ b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
<snipped>
> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >>+ luaM_metrics(L, &newm);
> >>+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
> >>+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
> >>+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
> >>+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
> >>+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
> >>+ /* Nothing to finalize, skipped. */
> >>+ assert_true(newm.gc_steps_finalize == 0);
> >>+ oldm = newm;
> >>+
> >>+ /*
> >>+ * Now let's run three GC cycles to ensure that
> >>+ * increment was not a lucky coincidence.
> >>+ */
> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >>+ lua_gc(L, LUA_GCCOLLECT, 0);
> >I think it is better to create a for loop here.
I just reuse code from the previous implementation, there is no goal to
refactor it. Also, use 3 calls in a raw looks OK to me.
> >>+ luaM_metrics(L, &newm);
> >>+ assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
> >>+ assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
> >>+ assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
> >>+ assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
> >>+ assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
> >That part is duplicated. It is better to either write a corresponding macro, or
> >a function.
Ditto.
> >>+ /* Nothing to finalize, skipped. */
<snipped>
> >>+/*
> >>+ * Get function to call to generate the corresponding snapshot
> >>+ * restores on top of the Lua stack. Function returns the amount
> >>+ * of snapshot restorations expected.
> >>+ * Clear stack after call.
> >>+ */
> >>+static void check_snap_restores(lua_State *L)
> >>+{
> >>+ struct luam_Metrics oldm, newm;
> >>+ luaM_metrics(L, &oldm);
> >>+ /* Generate snapshots. */
> >>+ lua_call(L, 0, 1);
> >>+ int n = lua_gettop(L);
> >>+ /*
> >>+ * The first value is the table with functions,
> >>+ * the second is number of snapshot restores.
> >>+ */
> >>+ if (n != 2 || !lua_isnumber(L, -1))
> >>+ bail_out("incorrect return value: 1 number is required");
> >>+ size_t snap_restores = lua_tonumber(L, -1);
> >>+ luaM_metrics(L, &newm);
> >>+ /*
> >>+ * Remove `snap_restores` from stack.
> >>+ * Must be done before potiential assert and exit from
> >>+ * the test.
> >>+ */
> >>+ lua_pop(L, 1);
> >>+ assert_true(newm.jit_snap_restore - oldm.jit_snap_restore
> >>+ == snap_restores);
> >>+}
> >>+
> >>+static int snap_restores_direct_exit(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >>+static int snap_restores_direct_exit_scalar(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >>+static int snap_restores_side_exit_compiled(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >>+static int snap_restores_side_exit_not_compiled(void *test_state)
> >>+{
> >>+ lua_State *L = test_state;
> >>+ utils_get_aux_lfunc(L);
> >>+ check_snap_restores(L);
> >>+ return TEST_EXIT_SUCCESS;
> >>+}
> >>+
> >Same here. Either a dedicated implementation func, or a macro is needed here.
`utils_get_aux_lfunc(L)` helper macro looks to the __func__ value, to
load the corresponding function from Lua script. We may add the macro to
generate functions, but I doubt that it will improve readability of the
code. I may add an additional comment to `check_snap_restores()` about
`utils_get_aux_lfunc(L)`, but existing one looks self-sufficient for me
right now, so please guide me here:).
> >>+static int snap_restores_group(void *test_state)
<snipped>
> >>diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
<snipped>
> >>--
> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin
> >
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C Sergey Kaplun via Tarantool-patches
2023-05-19 12:17 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-29 16:15 ` Sergey Bronnikov via Tarantool-patches
1 sibling, 0 replies; 29+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-05-29 16:15 UTC (permalink / raw)
To: Sergey Kaplun, Igor Munkin, Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Sergey
see my comments below
On 5/18/23 23:44, Sergey Kaplun wrote:
> This patch rewrites the aforementioned test with the usage libtest,
> recently introduced. Since we still stand in need of a Lua helper script
> for generation of traces, the original test file is reworked as a
> standalone script, which returns the table with helper functions. Each
> helper function is named the same as the test where it will be used. Now
> single quotes are used according to our Lua code style.
>
> In C part all asserts from glibc are replaced with the corresponding
> assert_{cond} from the libtest. Now tests return `TEST_EXIT_SUCCESS` at
> the finish. Also, the stack check for the amount of return results from
> the helper function is slightly changed, since there is one more stack
> slot in use (table with these functions). `snap_restores()` C function
> duplicates 4 times for each subtest. Common helper
> `check_snap_restores()` is used for each of them. Each error throwing is
> replaced with `bail_out()` call.
>
> NB: `lua_pop()` to clear the Lua stack after a call should be done before
> any possible assertion, which would exit from the test leaving the stack
> uncleaned.
>
> All skipconds use macros defined in <lj_arch.h>, so it is included in
> the test. As far as this test initializes the LuaJIT VM manually, there
> is no need to check `jit.status()` result; checking `LJ_HASJIT` is
> enough. Now, only JIT-related tests are skipped, when compiled without
> JIT. Nevertheless, all tests are skipped for *BSD arches.
>
> Also, this patch sets the new CMake variable named `LUAJIT_LIBRARY`
> equal to `LUAJIT_LIB` in `PARENT_SCOPE` to be used in tarantool-c-tests
> linking.
>
> Also, .c_test suffix is added to the <.gitignore>.
>
> Part of tarantool/tarantool#7900
> ---
> .gitignore | 1 +
> src/CMakeLists.txt | 2 +
> test/tarantool-c-tests/CMakeLists.txt | 32 +-
> .../misclib-getmetrics-capi-script.lua} | 83 ++---
> .../misclib-getmetrics-capi.test.c | 343 ++++++++++++++++++
> test/tarantool-tests/CMakeLists.txt | 1 -
> .../misclib-getmetrics-capi/CMakeLists.txt | 1 -
> .../misclib-getmetrics-capi/testgetmetrics.c | 270 --------------
> 8 files changed, 412 insertions(+), 321 deletions(-)
> rename test/{tarantool-tests/misclib-getmetrics-capi.test.lua => tarantool-c-tests/misclib-getmetrics-capi-script.lua} (68%)
> create mode 100644 test/tarantool-c-tests/misclib-getmetrics-capi.test.c
> delete mode 100644 test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
> delete mode 100644 test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
>
> diff --git a/.gitignore b/.gitignore
> index b7908aee..dc5ea5fc 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -24,3 +24,4 @@ install_manifest.txt
> luajit-parse-memprof
> luajit-parse-sysprof
> luajit.pc
> +*.c_test
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index dffc0a4d..fed4c38d 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -325,6 +325,8 @@ if(NOT BUILDMODE STREQUAL "dynamic")
> set(LUAJIT_BIN luajit_static)
> set(LUAJIT_LIB libluajit_static)
> endif()
> +# Need for the test linking, so the PARENT_SCOPE option is used.
> +set(LUAJIT_LIBRARY ${LUAJIT_LIB} PARENT_SCOPE)
> set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})
>
> add_executable(${LUAJIT_BIN} EXCLUDE_FROM_ALL ${CLI_SOURCES})
> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
> index c6b7cd30..c9d75d6c 100644
> --- a/test/tarantool-c-tests/CMakeLists.txt
> +++ b/test/tarantool-c-tests/CMakeLists.txt
> @@ -22,13 +22,37 @@ set_target_properties(libtest PROPERTIES
> LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
> )
>
> -# XXX: For now, just build libtest. The tests to be depended on
> -# will be added in the next commit.
> +# TARGET_C_FLAGS is required here to be sure that headers like
> +# lj_arch.h in compiled test are consistent with the LuaJIT library
> +# to link.
> +AppendFlags(TESTS_C_FLAGS ${TARGET_C_FLAGS})
> +
> +set(CTEST_SRC_SUFFIX ".test.c")
> +file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
> +foreach(test_source ${tests})
> + string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe ${test_source})
Please replace with get_filename_component [1]:
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -31,7 +31,7 @@ set(CTEST_SRC_SUFFIX ".test.c")
file(GLOB tests "${CMAKE_CURRENT_SOURCE_DIR}/*${CTEST_SRC_SUFFIX}")
foreach(test_source ${tests})
# Get test name without suffix. Needed to set OUTPUT_NAME.
- string(REGEX REPLACE ".+/([^/]+)${CTEST_SRC_SUFFIX}" "\\1" exe
${test_source})
+ get_filename_component(exe ${test_source} NAME_WE)
add_executable(${exe} EXCLUDE_FROM_ALL ${test_source})
target_include_directories(${exe} PRIVATE
${CMAKE_CURRENT_SOURCE_DIR}
1: https://cmake.org/cmake/help/latest/command/get_filename_component.html
> + add_executable(${exe} EXCLUDE_FROM_ALL ${test_source})
> + target_include_directories(${exe} PRIVATE
> + ${CMAKE_CURRENT_SOURCE_DIR}
> + ${LUAJIT_SOURCE_DIR}
> + )
> + set_target_properties(${exe} PROPERTIES
> + # `__FILE__` macro may not represnt absolute path to the
Typo: represnt
> + # source file, depening on cmake version or
Typo: depening
> + # -fmacro-prefix-map flag value.
> + # Use custom macro.
> + COMPILE_FLAGS "${TESTS_C_FLAGS} -D__ABS_FILENAME__='\"${test_source}\"'"
> + OUTPUT_NAME "${exe}${C_TEST_SUFFIX}"
> + RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
> + )
> + target_link_libraries(${exe} libtest ${LUAJIT_LIBRARY})
> + LIST(APPEND TESTS_COMPILED ${exe})
> +endforeach()
> +
> add_custom_target(tarantool-c-tests
> - DEPENDS libluajit libtest
> + DEPENDS libluajit libtest ${TESTS_COMPILED}
> )
>
> -# XXX: For now, run 0 tests. Just verify that libtest was built.
> add_custom_command(TARGET tarantool-c-tests
> COMMENT "Running Tarantool C tests"
> COMMAND
> diff --git a/test/tarantool-tests/misclib-getmetrics-capi.test.lua b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> similarity index 68%
> rename from test/tarantool-tests/misclib-getmetrics-capi.test.lua
> rename to test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> index 654e5545..2f0ee5cf 100644
> --- a/test/tarantool-tests/misclib-getmetrics-capi.test.lua
> +++ b/test/tarantool-c-tests/misclib-getmetrics-capi-script.lua
> @@ -1,32 +1,26 @@
> -local tap = require('tap')
> -local test = tap.test("clib-misc-getmetrics"):skipcond({
> - ['Test requires JIT enabled'] = not jit.status(),
> - ['Disabled on *BSD due to #4819'] = jit.os == 'BSD',
> -})
> +local ffi = require('ffi')
>
> -test:plan(11)
> +-- Auxiliary script to provide Lua functions to be used in tests
> +-- for `getmetrics()` C API inside the test
> +-- <misclib-getmetrics-capi.test.c>.
> +local M = {}
>
> -local path = arg[0]:gsub('%.test%.lua', '')
> -local suffix = package.cpath:match('?.(%a+);')
> -package.cpath = ('%s/?.%s;'):format(path, suffix)..package.cpath
> +-- XXX: Max nins is limited by max IRRef, that equals to
> +-- REF_DROP - REF_BIAS. Unfortunately, these constants are not
> +-- provided to Lua space, so we ought to make some math:
> +-- * REF_DROP = 0xffff
> +-- * REF_BIAS = 0x8000
> +-- See <src/lj_ir.h> for details.
> +local MAXNINS = 0xffff - 0x8000
>
> -local MAXNINS = require('utils').const.maxnins
> local jit_opt_default = {
> 3, -- level
> - "hotloop=56",
> - "hotexit=10",
> - "minstitch=0",
> + 'hotloop=56',
> + 'hotexit=10',
> + 'minstitch=0',
> }
>
> -local testgetmetrics = require("testgetmetrics")
> -
> -test:ok(testgetmetrics.base())
> -test:ok(testgetmetrics.gc_allocated_freed())
> -test:ok(testgetmetrics.gc_steps())
> -
> -test:ok(testgetmetrics.objcount(function(iterations)
> - local ffi = require("ffi")
> -
> +M.objcount = function(iterations)
> jit.opt.start(0)
>
> local placeholder = {
> @@ -51,35 +45,34 @@ test:ok(testgetmetrics.objcount(function(iterations)
>
> for _ = 1, iterations do
> -- Check counting of VLA/VLS/aligned cdata.
> - table.insert(placeholder.cdata, ffi.new("char[?]", 4))
> + table.insert(placeholder.cdata, ffi.new('char[?]', 4))
> end
>
> for _ = 1, iterations do
> -- Check counting of non-VLA/VLS/aligned cdata.
> - table.insert(placeholder.cdata, ffi.new("uint64_t", _))
> + table.insert(placeholder.cdata, ffi.new('uint64_t', _))
> end
>
> placeholder = nil -- luacheck: no unused
> -- Restore default jit settings.
> jit.opt.start(unpack(jit_opt_default))
> -end))
> +end
>
> -test:ok(testgetmetrics.objcount_cdata_decrement(function()
> +M.objcount_cdata_decrement = function()
> -- gc_cdatanum decrement test.
> -- See https://github.com/tarantool/tarantool/issues/5820.
> - local ffi = require("ffi")
> local function nop() end
> - ffi.gc(ffi.cast("void *", 0), nop)
> + ffi.gc(ffi.cast('void *', 0), nop)
> -- Does not collect the cdata, but resurrects the object and
> -- removes LJ_GC_CDATA_FIN flag.
> collectgarbage()
> -- Collects the cdata.
> collectgarbage()
> -end))
> +end
>
> -- Compiled loop with a direct exit to the interpreter.
> -test:ok(testgetmetrics.snap_restores(function()
> - jit.opt.start(0, "hotloop=1")
> +M.snap_restores_direct_exit = function()
> + jit.opt.start(0, 'hotloop=1')
>
> local sum = 0
> for i = 1, 20 do
> @@ -91,11 +84,11 @@ test:ok(testgetmetrics.snap_restores(function()
>
> -- A single snapshot restoration happened on loop finish.
> return 1
> -end))
> +end
>
> -- Compiled loop with a side exit which does not get compiled.
> -test:ok(testgetmetrics.snap_restores(function()
> - jit.opt.start(0, "hotloop=1", "hotexit=2", ("minstitch=%d"):format(MAXNINS))
> +M.snap_restores_side_exit_not_compiled = function()
> + jit.opt.start(0, 'hotloop=1', 'hotexit=2', ('minstitch=%d'):format(MAXNINS))
>
> local function foo(i)
> -- math.fmod is not yet compiled!
> @@ -112,13 +105,13 @@ test:ok(testgetmetrics.snap_restores(function()
>
> -- Side exits from the root trace could not get compiled.
> return 5
> -end))
> +end
>
> -- Compiled loop with a side exit which gets compiled.
> -test:ok(testgetmetrics.snap_restores(function()
> +M.snap_restores_side_exit_compiled = function()
> -- Optimization level is important here as `loop` optimization
> -- may unroll the loop body and insert +1 side exit.
> - jit.opt.start(0, "hotloop=1", "hotexit=5")
> + jit.opt.start(0, 'hotloop=1', 'hotexit=5')
>
> local function foo(i)
> return i <= 10 and i or tostring(i)
> @@ -136,13 +129,13 @@ test:ok(testgetmetrics.snap_restores(function()
> -- and compiled
> -- 1 side exit on loop end
> return 6
> -end))
> +end
>
> -- Compiled scalar trace with a direct exit to the interpreter.
> -test:ok(testgetmetrics.snap_restores(function()
> +M.snap_restores_direct_exit_scalar = function()
> -- For calls it will be 2 * hotloop (see lj_dispatch.{c,h}
> -- and hotcall@vm_*.dasc).
> - jit.opt.start(3, "hotloop=2", "hotexit=3")
> + jit.opt.start(3, 'hotloop=2', 'hotexit=3')
>
> local function foo(i)
> return i <= 15 and i or tostring(i)
> @@ -167,15 +160,15 @@ test:ok(testgetmetrics.snap_restores(function()
> jit.opt.start(unpack(jit_opt_default))
>
> return 2
> -end))
> +end
>
> -test:ok(testgetmetrics.strhash())
> -
> -test:ok(testgetmetrics.tracenum_base(function()
> +M.tracenum_base = function()
> local sum = 0
> for i = 1, 200 do
> sum = sum + i
> end
> -- Compiled only 1 loop as new trace.
> return 1
> -end))
> +end
> +
> +return M
> diff --git a/test/tarantool-c-tests/misclib-getmetrics-capi.test.c b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
> new file mode 100644
> index 00000000..202cd395
> --- /dev/null
> +++ b/test/tarantool-c-tests/misclib-getmetrics-capi.test.c
> @@ -0,0 +1,343 @@
> +#include "lua.h"
> +#include "luajit.h"
> +#include "lauxlib.h"
> +#include "lmisclib.h"
> +
> +#include "test.h"
> +#include "utils.h"
> +
> +/* Need for skipcond for BSD and JIT. */
> +#include "lj_arch.h"
> +
> +static int base(void *test_state)
> +{
> + lua_State *L = test_state;
> + struct luam_Metrics metrics;
> + luaM_metrics(L, &metrics);
> +
> + /*
> + * Just check structure format, not values that fields
> + * contain.
> + */
> + (void)metrics.strhash_hit;
> + (void)metrics.strhash_miss;
> +
> + (void)metrics.gc_strnum;
> + (void)metrics.gc_tabnum;
> + (void)metrics.gc_udatanum;
> + (void)metrics.gc_cdatanum;
> +
> + (void)metrics.gc_total;
> + (void)metrics.gc_freed;
> + (void)metrics.gc_allocated;
> +
> + (void)metrics.gc_steps_pause;
> + (void)metrics.gc_steps_propagate;
> + (void)metrics.gc_steps_atomic;
> + (void)metrics.gc_steps_sweepstring;
> + (void)metrics.gc_steps_sweep;
> + (void)metrics.gc_steps_finalize;
> +
> + (void)metrics.jit_snap_restore;
> + (void)metrics.jit_trace_abort;
> + (void)metrics.jit_mcode_size;
> + (void)metrics.jit_trace_num;
> +
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int gc_allocated_freed(void *test_state)
> +{
> + lua_State *L = test_state;
> + struct luam_Metrics oldm, newm;
> + /* Force up garbage collect all dead objects. */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> +
> + luaM_metrics(L, &oldm);
> + /* Simple garbage generation. */
> + if (luaL_dostring(L, "local i = 0 for j = 1, 10 do i = i + j end"))
> + bail_out("failed to translate Lua code snippet");
> + lua_gc(L, LUA_GCCOLLECT, 0);
> + luaM_metrics(L, &newm);
> + assert_true(newm.gc_allocated - oldm.gc_allocated > 0);
> + assert_true(newm.gc_freed - oldm.gc_freed > 0);
> +
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int gc_steps(void *test_state)
> +{
> + lua_State *L = test_state;
> + struct luam_Metrics oldm, newm;
> + /*
> + * Some garbage has already happened before the next line,
> + * i.e. during frontend processing Lua test chunk.
> + * Let's put a full garbage collection cycle on top
> + * of that, and confirm that non-null values are reported
> + * (we are not yet interested in actual numbers):
> + */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> +
> + luaM_metrics(L, &oldm);
> + assert_true(oldm.gc_steps_pause > 0);
> + assert_true(oldm.gc_steps_propagate > 0);
> + assert_true(oldm.gc_steps_atomic > 0);
> + assert_true(oldm.gc_steps_sweepstring > 0);
> + assert_true(oldm.gc_steps_sweep > 0);
> + /* Nothing to finalize, skipped. */
> + assert_true(oldm.gc_steps_finalize == 0);
> +
> + /*
> + * As long as we don't create new Lua objects
> + * consequent call should return the same values:
> + */
> + luaM_metrics(L, &newm);
> + assert_sizet_equal(newm.gc_steps_pause, oldm.gc_steps_pause);
> + assert_sizet_equal(newm.gc_steps_propagate, oldm.gc_steps_propagate);
> + assert_sizet_equal(newm.gc_steps_atomic, oldm.gc_steps_atomic);
> + assert_sizet_equal(newm.gc_steps_sweepstring,
> + oldm.gc_steps_sweepstring);
> + assert_sizet_equal(newm.gc_steps_sweep, oldm.gc_steps_sweep);
> + /* Nothing to finalize, skipped. */
> + assert_true(newm.gc_steps_finalize == 0);
> + oldm = newm;
> +
> + /*
> + * Now the last phase: run full GC once and make sure that
> + * everything is being reported as expected:
> + */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> + luaM_metrics(L, &newm);
> + assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
> + assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
> + assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
> + assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
> + assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
> + /* Nothing to finalize, skipped. */
> + assert_true(newm.gc_steps_finalize == 0);
> + oldm = newm;
> +
> + /*
> + * Now let's run three GC cycles to ensure that
> + * increment was not a lucky coincidence.
> + */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> + lua_gc(L, LUA_GCCOLLECT, 0);
> + lua_gc(L, LUA_GCCOLLECT, 0);
> + luaM_metrics(L, &newm);
> + assert_true(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
> + assert_true(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
> + assert_true(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
> + assert_true(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
> + assert_true(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
> + /* Nothing to finalize, skipped. */
> + assert_true(newm.gc_steps_finalize == 0);
> +
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int objcount(void *test_state)
> +{
> + lua_State *L = test_state;
> + struct luam_Metrics oldm, newm;
> + if (!LJ_HASJIT)
> + return skip("Test requires JIT enabled");
> +
> + utils_get_aux_lfunc(L);
> +
> + /* Force up garbage collect all dead objects. */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> +
> + luaM_metrics(L, &oldm);
> + /* Generate garbage. Argument is iterations amount. */
> + lua_pushnumber(L, 1000);
> + lua_call(L, 1, 0);
> + lua_gc(L, LUA_GCCOLLECT, 0);
> + luaM_metrics(L, &newm);
> + assert_sizet_equal(newm.gc_strnum, oldm.gc_strnum);
> + assert_sizet_equal(newm.gc_tabnum, oldm.gc_tabnum);
> + assert_sizet_equal(newm.gc_udatanum, oldm.gc_udatanum);
> + assert_sizet_equal(newm.gc_cdatanum, oldm.gc_cdatanum);
> +
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int objcount_cdata_decrement(void *test_state)
> +{
> + lua_State *L = test_state;
> + /*
> + * cdata decrement test.
> + * See https://github.com/tarantool/tarantool/issues/5820.
> + */
> + struct luam_Metrics oldm, newm;
> + utils_get_aux_lfunc(L);
> +
> + /* Force up garbage collect all dead objects. */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> +
> + luaM_metrics(L, &oldm);
> + /*
> + * The function generates and collects cdata with
> + * LJ_GC_CDATA_FIN flag.
> + */
> + lua_call(L, 0, 0);
> + luaM_metrics(L, &newm);
> + assert_sizet_equal(newm.gc_cdatanum, oldm.gc_cdatanum);
> +
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +/*
> + * Get function to call to generate the corresponding snapshot
> + * restores on top of the Lua stack. Function returns the amount
> + * of snapshot restorations expected.
> + * Clear stack after call.
> + */
> +static void check_snap_restores(lua_State *L)
> +{
> + struct luam_Metrics oldm, newm;
> + luaM_metrics(L, &oldm);
> + /* Generate snapshots. */
> + lua_call(L, 0, 1);
> + int n = lua_gettop(L);
> + /*
> + * The first value is the table with functions,
> + * the second is number of snapshot restores.
> + */
> + if (n != 2 || !lua_isnumber(L, -1))
> + bail_out("incorrect return value: 1 number is required");
> + size_t snap_restores = lua_tonumber(L, -1);
> + luaM_metrics(L, &newm);
> + /*
> + * Remove `snap_restores` from stack.
> + * Must be done before potiential assert and exit from
> + * the test.
> + */
> + lua_pop(L, 1);
> + assert_true(newm.jit_snap_restore - oldm.jit_snap_restore
> + == snap_restores);
> +}
> +
> +static int snap_restores_direct_exit(void *test_state)
> +{
> + lua_State *L = test_state;
> + utils_get_aux_lfunc(L);
> + check_snap_restores(L);
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int snap_restores_direct_exit_scalar(void *test_state)
> +{
> + lua_State *L = test_state;
> + utils_get_aux_lfunc(L);
> + check_snap_restores(L);
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int snap_restores_side_exit_compiled(void *test_state)
> +{
> + lua_State *L = test_state;
> + utils_get_aux_lfunc(L);
> + check_snap_restores(L);
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int snap_restores_side_exit_not_compiled(void *test_state)
> +{
> + lua_State *L = test_state;
> + utils_get_aux_lfunc(L);
> + check_snap_restores(L);
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int snap_restores_group(void *test_state)
> +{
> + if (!LJ_HASJIT)
> + return skip("Test requires JIT enabled");
> + const struct test_unit tgroup[] = {
> + test_unit_new(snap_restores_direct_exit),
> + test_unit_new(snap_restores_direct_exit_scalar),
> + test_unit_new(snap_restores_side_exit_compiled),
> + test_unit_new(snap_restores_side_exit_not_compiled)
> + };
> + return test_run_group(tgroup, test_state);
> +}
> +
> +static int strhash(void *test_state)
> +{
> + lua_State *L = test_state;
> + struct luam_Metrics oldm, newm;
> + lua_pushstring(L, "strhash_hit");
> + luaM_metrics(L, &oldm);
> + lua_pushstring(L, "strhash_hit");
> + lua_pushstring(L, "new_str");
> + luaM_metrics(L, &newm);
> + /* Remove pushed strings. */
> + lua_pop(L, 3);
> + assert_true(newm.strhash_hit - oldm.strhash_hit == 1);
> + assert_true(newm.strhash_miss - oldm.strhash_miss == 1);
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +static int tracenum_base(void *test_state)
> +{
> + lua_State *L = test_state;
> + if (!LJ_HASJIT)
> + return skip("Test requires JIT enabled");
> + struct luam_Metrics metrics;
> + utils_get_aux_lfunc(L);
> +
> + luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
> + /* Force up garbage collect all dead objects. */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> +
> + luaM_metrics(L, &metrics);
> + assert_true(metrics.jit_trace_num == 0);
> +
> + /* Generate traces. */
> + lua_call(L, 0, 1);
> + int n = lua_gettop(L);
> + /*
> + * The first value is the table with functions,
> + * the second is the amount of traces.
> + */
> + if (n != 2 || !lua_isnumber(L, -1))
> + bail_out("incorrect return value: 1 number is required");
> + size_t jit_trace_num = lua_tonumber(L, -1);
> + luaM_metrics(L, &metrics);
> + /* Remove `jit_trace_num` from Lua stack. */
> + lua_pop(L, 1);
> +
> + assert_sizet_equal(metrics.jit_trace_num, jit_trace_num);
> +
> + luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
> + /* Force up garbage collect all dead objects. */
> + lua_gc(L, LUA_GCCOLLECT, 0);
> + luaM_metrics(L, &metrics);
> + assert_true(metrics.jit_trace_num == 0);
> +
> + return TEST_EXIT_SUCCESS;
> +}
> +
> +int main(void)
> +{
> + if (LUAJIT_OS == LUAJIT_OS_BSD)
> + return skip_all("Disabled on *BSD due to #4819");
> +
> + lua_State *L = utils_lua_init();
> +
> + utils_load_aux_script(L);
> + const struct test_unit tgroup[] = {
> + test_unit_new(base),
> + test_unit_new(gc_allocated_freed),
> + test_unit_new(gc_steps),
> + test_unit_new(objcount),
> + test_unit_new(objcount_cdata_decrement),
> + test_unit_new(snap_restores_group),
> + test_unit_new(strhash),
> + test_unit_new(tracenum_base)
> + };
> + const int test_result = test_run_group(tgroup, L);
> + utils_lua_close(L);
> + return test_result;
> +}
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 38d6ae49..b4ce39d3 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -66,7 +66,6 @@ add_subdirectory(lj-416-xor-before-jcc)
> add_subdirectory(lj-601-fix-gc-finderrfunc)
> add_subdirectory(lj-727-lightuserdata-itern)
> add_subdirectory(lj-flush-on-trace)
> -add_subdirectory(misclib-getmetrics-capi)
> add_subdirectory(misclib-sysprof-capi)
>
> # The part of the memory profiler toolchain is located in tools
> diff --git a/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt b/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
> deleted file mode 100644
> index 60eb5bbb..00000000
> --- a/test/tarantool-tests/misclib-getmetrics-capi/CMakeLists.txt
> +++ /dev/null
> @@ -1 +0,0 @@
> -BuildTestCLib(testgetmetrics testgetmetrics.c)
> diff --git a/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c b/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
> deleted file mode 100644
> index 67776338..00000000
> --- a/test/tarantool-tests/misclib-getmetrics-capi/testgetmetrics.c
> +++ /dev/null
> @@ -1,270 +0,0 @@
> -#include <lua.h>
> -#include <luajit.h>
> -#include <lauxlib.h>
> -
> -#include <lmisclib.h>
> -
> -#undef NDEBUG
> -#include <assert.h>
> -
> -static int base(lua_State *L)
> -{
> - struct luam_Metrics metrics;
> - luaM_metrics(L, &metrics);
> -
> - /* Just check structure format, not values that fields contain. */
> - (void)metrics.strhash_hit;
> - (void)metrics.strhash_miss;
> -
> - (void)metrics.gc_strnum;
> - (void)metrics.gc_tabnum;
> - (void)metrics.gc_udatanum;
> - (void)metrics.gc_cdatanum;
> -
> - (void)metrics.gc_total;
> - (void)metrics.gc_freed;
> - (void)metrics.gc_allocated;
> -
> - (void)metrics.gc_steps_pause;
> - (void)metrics.gc_steps_propagate;
> - (void)metrics.gc_steps_atomic;
> - (void)metrics.gc_steps_sweepstring;
> - (void)metrics.gc_steps_sweep;
> - (void)metrics.gc_steps_finalize;
> -
> - (void)metrics.jit_snap_restore;
> - (void)metrics.jit_trace_abort;
> - (void)metrics.jit_mcode_size;
> - (void)metrics.jit_trace_num;
> -
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static int gc_allocated_freed(lua_State *L)
> -{
> - struct luam_Metrics oldm, newm;
> - /* Force up garbage collect all dead objects. */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> -
> - luaM_metrics(L, &oldm);
> - /* Simple garbage generation. */
> - if (luaL_dostring(L, "local i = 0 for j = 1, 10 do i = i + j end"))
> - luaL_error(L, "failed to translate Lua code snippet");
> - lua_gc(L, LUA_GCCOLLECT, 0);
> - luaM_metrics(L, &newm);
> - assert(newm.gc_allocated - oldm.gc_allocated > 0);
> - assert(newm.gc_freed - oldm.gc_freed > 0);
> -
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static int gc_steps(lua_State *L)
> -{
> - struct luam_Metrics oldm, newm;
> - /*
> - * Some garbage has already happened before the next line,
> - * i.e. during frontend processing Lua test chunk.
> - * Let's put a full garbage collection cycle on top
> - * of that, and confirm that non-null values are reported
> - * (we are not yet interested in actual numbers):
> - */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> -
> - luaM_metrics(L, &oldm);
> - assert(oldm.gc_steps_pause > 0);
> - assert(oldm.gc_steps_propagate > 0);
> - assert(oldm.gc_steps_atomic > 0);
> - assert(oldm.gc_steps_sweepstring > 0);
> - assert(oldm.gc_steps_sweep > 0);
> - /* Nothing to finalize, skipped. */
> - assert(oldm.gc_steps_finalize == 0);
> -
> - /*
> - * As long as we don't create new Lua objects
> - * consequent call should return the same values:
> - */
> - luaM_metrics(L, &newm);
> - assert(newm.gc_steps_pause - oldm.gc_steps_pause == 0);
> - assert(newm.gc_steps_propagate - oldm.gc_steps_propagate == 0);
> - assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 0);
> - assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring == 0);
> - assert(newm.gc_steps_sweep - oldm.gc_steps_sweep == 0);
> - /* Nothing to finalize, skipped. */
> - assert(newm.gc_steps_finalize == 0);
> - oldm = newm;
> -
> - /*
> - * Now the last phase: run full GC once and make sure that
> - * everything is being reported as expected:
> - */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> - luaM_metrics(L, &newm);
> - assert(newm.gc_steps_pause - oldm.gc_steps_pause == 1);
> - assert(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 1);
> - assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 1);
> - assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 1);
> - assert(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 1);
> - /* Nothing to finalize, skipped. */
> - assert(newm.gc_steps_finalize == 0);
> - oldm = newm;
> -
> - /*
> - * Now let's run three GC cycles to ensure that
> - * increment was not a lucky coincidence.
> - */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> - lua_gc(L, LUA_GCCOLLECT, 0);
> - lua_gc(L, LUA_GCCOLLECT, 0);
> - luaM_metrics(L, &newm);
> - assert(newm.gc_steps_pause - oldm.gc_steps_pause == 3);
> - assert(newm.gc_steps_propagate - oldm.gc_steps_propagate >= 3);
> - assert(newm.gc_steps_atomic - oldm.gc_steps_atomic == 3);
> - assert(newm.gc_steps_sweepstring - oldm.gc_steps_sweepstring >= 3);
> - assert(newm.gc_steps_sweep - oldm.gc_steps_sweep >= 3);
> - /* Nothing to finalize, skipped. */
> - assert(newm.gc_steps_finalize == 0);
> -
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static int objcount(lua_State *L)
> -{
> - struct luam_Metrics oldm, newm;
> - int n = lua_gettop(L);
> - if (n != 1 || !lua_isfunction(L, 1))
> - luaL_error(L, "incorrect argument: 1 function is required");
> -
> - /* Force up garbage collect all dead objects. */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> -
> - luaM_metrics(L, &oldm);
> - /* Generate garbage. Argument is iterations amount. */
> - lua_pushnumber(L, 1000);
> - lua_call(L, 1, 0);
> - lua_gc(L, LUA_GCCOLLECT, 0);
> - luaM_metrics(L, &newm);
> - assert(newm.gc_strnum - oldm.gc_strnum == 0);
> - assert(newm.gc_tabnum - oldm.gc_tabnum == 0);
> - assert(newm.gc_udatanum - oldm.gc_udatanum == 0);
> - assert(newm.gc_cdatanum - oldm.gc_cdatanum == 0);
> -
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static int objcount_cdata_decrement(lua_State *L)
> -{
> - /*
> - * cdata decrement test.
> - * See https://github.com/tarantool/tarantool/issues/5820.
> - */
> - struct luam_Metrics oldm, newm;
> - int n = lua_gettop(L);
> - if (n != 1 || !lua_isfunction(L, 1))
> - luaL_error(L, "incorrect argument: 1 function is required");
> -
> - /* Force up garbage collect all dead objects. */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> -
> - luaM_metrics(L, &oldm);
> - /*
> - * The function generates and collects cdata with
> - * LJ_GC_CDATA_FIN flag.
> - */
> - lua_call(L, 0, 0);
> - luaM_metrics(L, &newm);
> - assert(newm.gc_cdatanum - oldm.gc_cdatanum == 0);
> -
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static int snap_restores(lua_State *L)
> -{
> - struct luam_Metrics oldm, newm;
> - int n = lua_gettop(L);
> - if (n != 1 || !lua_isfunction(L, 1))
> - luaL_error(L, "incorrect arguments: 1 function is required");
> -
> - luaM_metrics(L, &oldm);
> - /* Generate snapshots. */
> - lua_call(L, 0, 1);
> - n = lua_gettop(L);
> - if (n != 1 || !lua_isnumber(L, 1))
> - luaL_error(L, "incorrect return value: 1 number is required");
> - size_t snap_restores = lua_tonumber(L, 1);
> - luaM_metrics(L, &newm);
> - assert(newm.jit_snap_restore - oldm.jit_snap_restore == snap_restores);
> -
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static int strhash(lua_State *L)
> -{
> - struct luam_Metrics oldm, newm;
> - lua_pushstring(L, "strhash_hit");
> - luaM_metrics(L, &oldm);
> - lua_pushstring(L, "strhash_hit");
> - lua_pushstring(L, "new_str");
> - luaM_metrics(L, &newm);
> - assert(newm.strhash_hit - oldm.strhash_hit == 1);
> - assert(newm.strhash_miss - oldm.strhash_miss == 1);
> - lua_pop(L, 3);
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static int tracenum_base(lua_State *L)
> -{
> - struct luam_Metrics metrics;
> - int n = lua_gettop(L);
> - if (n != 1 || !lua_isfunction(L, 1))
> - luaL_error(L, "incorrect arguments: 1 function is required");
> -
> - luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
> - /* Force up garbage collect all dead objects. */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> -
> - luaM_metrics(L, &metrics);
> - assert(metrics.jit_trace_num == 0);
> -
> - /* Generate traces. */
> - lua_call(L, 0, 1);
> - n = lua_gettop(L);
> - if (n != 1 || !lua_isnumber(L, 1))
> - luaL_error(L, "incorrect return value: 1 number is required");
> - size_t jit_trace_num = lua_tonumber(L, 1);
> - luaM_metrics(L, &metrics);
> - assert(metrics.jit_trace_num == jit_trace_num);
> -
> - luaJIT_setmode(L, 0, LUAJIT_MODE_FLUSH);
> - /* Force up garbage collect all dead objects. */
> - lua_gc(L, LUA_GCCOLLECT, 0);
> - luaM_metrics(L, &metrics);
> - assert(metrics.jit_trace_num == 0);
> -
> - lua_pushboolean(L, 1);
> - return 1;
> -}
> -
> -static const struct luaL_Reg testgetmetrics[] = {
> - {"base", base},
> - {"gc_allocated_freed", gc_allocated_freed},
> - {"gc_steps", gc_steps},
> - {"objcount", objcount},
> - {"objcount_cdata_decrement", objcount_cdata_decrement},
> - {"snap_restores", snap_restores},
> - {"strhash", strhash},
> - {"tracenum_base", tracenum_base},
> - {NULL, NULL}
> -};
> -
> -LUA_API int luaopen_testgetmetrics(lua_State *L)
> -{
> - luaL_register(L, "testgetmetrics", testgetmetrics);
> - return 1;
> -}
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH v2 luajit 5/6] test: rewrite misclib-sysprof-capi test in C
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
` (3 preceding siblings ...)
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 4/6] test: rewrite misclib-getmetrics-capi test in C Sergey Kaplun via Tarantool-patches
@ 2023-05-18 20:44 ` Sergey Kaplun via Tarantool-patches
2023-05-19 13:00 ` Maxim Kokryashkin via Tarantool-patches
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 6/6] test: rewrite lj-49-bad-lightuserdata " Sergey Kaplun via Tarantool-patches
2023-05-19 14:29 ` [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Maxim Kokryashkin via Tarantool-patches
6 siblings, 1 reply; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-18 20:44 UTC (permalink / raw)
To: Igor Munkin, Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
This patch rewrites the aforementioned test with the usage libtest
recently introduced. The approach is similar to the previous patch.
`c_payload()` function to profile is now set up globally on script
loading. The payload for enabled and disabled JIT is the same. As far as
for sysprof testing is necessary to replace backtrace with libunwind
first, the payload tests are marked with `todo()`.
Nevertheless, glibc `assert()` still necessary to check the correctness
of the profile writer, so it is still used there.
Relates to tarantool/tarantool#781
Part of tarantool/tarantool#7900
---
.../misclib-sysprof-capi-script.lua | 35 ++
.../misclib-sysprof-capi.test.c | 325 ++++++++++++++++++
test/tarantool-tests/CMakeLists.txt | 1 -
.../misclib-sysprof-capi.test.lua | 54 ---
.../misclib-sysprof-capi/CMakeLists.txt | 1 -
.../misclib-sysprof-capi/testsysprof.c | 260 --------------
6 files changed, 360 insertions(+), 316 deletions(-)
create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi-script.lua
create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi.test.c
delete mode 100644 test/tarantool-tests/misclib-sysprof-capi.test.lua
delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt
delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
diff --git a/test/tarantool-c-tests/misclib-sysprof-capi-script.lua b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua
new file mode 100644
index 00000000..dd8387db
--- /dev/null
+++ b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua
@@ -0,0 +1,35 @@
+local M = {}
+
+-- luacheck: no global
+assert(c_payload, 'c_payload global function should be set via script loader')
+
+local function lua_payload(n)
+ if n <= 1 then
+ return n
+ end
+ return lua_payload(n - 1) + lua_payload(n - 2)
+end
+
+local function payload()
+ local n_iterations = 500000
+
+ local co = coroutine.create(function()
+ for i = 1, n_iterations do
+ if i % 2 == 0 then
+ c_payload(10)
+ else
+ lua_payload(10)
+ end
+ coroutine.yield()
+ end
+ end)
+
+ for _ = 1, n_iterations do
+ coroutine.resume(co)
+ end
+end
+
+M.profile_func_jiton = payload
+M.profile_func_jitoff = payload
+
+return M
diff --git a/test/tarantool-c-tests/misclib-sysprof-capi.test.c b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
new file mode 100644
index 00000000..f75f82cd
--- /dev/null
+++ b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
@@ -0,0 +1,325 @@
+#include "lauxlib.h"
+#include "lmisclib.h"
+#include "lua.h"
+#include "luajit.h"
+
+#include "test.h"
+#include "utils.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+/* XXX: Still need normal assert inside writer functions. */
+#undef NDEBUG
+#include <assert.h>
+
+/* Need for skipcond for OS and ARCH. */
+#include "lj_arch.h"
+
+#define UNUSED(x) ((void)(x))
+
+/* --- utils -------------------------------------------------- */
+
+#define SYSPROF_INTERVAL_DEFAULT 100
+
+/*
+ * Yep, 8Mb. Tuned in order not to bother the platform with too
+ * often flushes.
+ */
+#define STREAM_BUFFER_SIZE (8 * 1024 * 1024)
+
+
+/* --- C Payload ---------------------------------------------- */
+
+static double fib(double n)
+{
+ if (n <= 1)
+ return n;
+ return fib(n - 1) + fib(n - 2);
+}
+
+static int c_payload(lua_State *L)
+{
+ fib(luaL_checknumber(L, 1));
+ lua_pushboolean(L, 1);
+ return 1;
+}
+/* --- sysprof C API tests ------------------------------------ */
+
+static int base(void *test_state)
+{
+ UNUSED(test_state);
+ struct luam_Sysprof_Options opt = {};
+ struct luam_Sysprof_Counters cnt = {};
+
+ (void)opt.interval;
+ (void)opt.mode;
+ (void)opt.ctx;
+ (void)opt.buf;
+ (void)opt.len;
+
+ luaM_sysprof_report(&cnt);
+
+ (void)cnt.samples;
+ (void)cnt.vmst_interp;
+ (void)cnt.vmst_lfunc;
+ (void)cnt.vmst_ffunc;
+ (void)cnt.vmst_cfunc;
+ (void)cnt.vmst_gc;
+ (void)cnt.vmst_exit;
+ (void)cnt.vmst_record;
+ (void)cnt.vmst_opt;
+ (void)cnt.vmst_asm;
+ (void)cnt.vmst_trace;
+
+ return TEST_EXIT_SUCCESS;
+}
+
+static int validation(void *test_state)
+{
+ lua_State *L = test_state;
+ struct luam_Sysprof_Options opt = {};
+ int status = PROFILE_SUCCESS;
+
+ /* Unknown mode. */
+ opt.mode = 0x40;
+ status = luaM_sysprof_start(L, &opt);
+ assert_true(status == PROFILE_ERRUSE);
+
+ /* Buffer not configured. */
+ opt.mode = LUAM_SYSPROF_CALLGRAPH;
+ opt.buf = NULL;
+ status = luaM_sysprof_start(L, &opt);
+ assert_true(status == PROFILE_ERRUSE);
+
+ /* Bad interval. */
+ opt.mode = LUAM_SYSPROF_DEFAULT;
+ opt.interval = 0;
+ status = luaM_sysprof_start(L, &opt);
+ assert_true(status == PROFILE_ERRUSE);
+
+ /* Check if profiling started. */
+ opt.mode = LUAM_SYSPROF_DEFAULT;
+ opt.interval = SYSPROF_INTERVAL_DEFAULT;
+ status = luaM_sysprof_start(L, &opt);
+ assert_true(status == PROFILE_SUCCESS);
+
+ /* Already running. */
+ status = luaM_sysprof_start(L, &opt);
+ assert_true(status == PROFILE_ERRRUN);
+
+ /* Profiler stopping. */
+ status = luaM_sysprof_stop(L);
+ assert_true(status == PROFILE_SUCCESS);
+
+ /* Stopping profiler which is not running. */
+ status = luaM_sysprof_stop(L);
+ assert_true(status == PROFILE_ERRRUN);
+
+ return TEST_EXIT_SUCCESS;
+}
+
+/*
+ * FIXME: The following two tests are disabled because sometimes
+ * `backtrace` dynamically loads a platform-specific unwinder,
+ * which is not signal-safe.
+ */
+
+#if 0
+/*
+ * Structure given as ctx to sysprof writer and on_stop callback.
+ */
+struct sysprof_ctx {
+ /* Output file descriptor for data. */
+ int fd;
+ /* Buffer for data. */
+ uint8_t buf[STREAM_BUFFER_SIZE];
+};
+
+/*
+ * Default buffer writer function.
+ * Just call fwrite to the corresponding FILE.
+ */
+static size_t buffer_writer_default(const void **buf_addr, size_t len,
+ void *opt)
+{
+ struct sysprof_ctx *ctx = opt;
+ int fd = ctx->fd;
+ const void * const buf_start = *buf_addr;
+ const void *data = *buf_addr;
+ size_t write_total = 0;
+
+ assert(len <= STREAM_BUFFER_SIZE);
+
+ for (;;) {
+ const size_t written = write(fd, data, len - write_total);
+
+ if (written == 0) {
+ /* Re-tries write in case of EINTR. */
+ if (errno != EINTR) {
+ /*
+ * Will be freed as whole chunk
+ * later.
+ */
+ *buf_addr = NULL;
+ return write_total;
+ }
+ errno = 0;
+ continue;
+ }
+
+ write_total += written;
+ assert(write_total <= len);
+
+ if (write_total == len)
+ break;
+
+ data = (uint8_t *)data + (ptrdiff_t)written;
+ }
+
+ *buf_addr = buf_start;
+ return write_total;
+}
+
+/*
+ * Default on stop callback. Just close the corresponding stream.
+ */
+static int on_stop_cb_default(void *opt, uint8_t *buf)
+{
+ UNUSED(buf);
+ struct sysprof_ctx *ctx = opt;
+ int fd = ctx->fd;
+ free(ctx);
+ return close(fd);
+}
+
+static int stream_init(struct luam_Sysprof_Options *opt)
+{
+ struct sysprof_ctx *ctx = calloc(1, sizeof(struct sysprof_ctx));
+ if (NULL == ctx)
+ return PROFILE_ERRIO;
+
+ ctx->fd = open("/dev/null", O_WRONLY | O_CREAT, 0644);
+ if (-1 == ctx->fd) {
+ free(ctx);
+ return PROFILE_ERRIO;
+ }
+
+ opt->ctx = ctx;
+ opt->buf = ctx->buf;
+ opt->len = STREAM_BUFFER_SIZE;
+
+ return PROFILE_SUCCESS;
+}
+
+/* Get function to profile on top, call it. */
+static int check_profile_func(lua_State *L)
+{
+ struct luam_Sysprof_Options opt = {};
+ struct luam_Sysprof_Counters cnt = {};
+ int status = PROFILE_ERRUSE;
+ /*
+ * Since all the other modes functionality is the
+ * subset of CALLGRAPH mode, run this mode to test
+ * the profiler's behavior.
+ */
+ opt.mode = LUAM_SYSPROF_CALLGRAPH;
+ opt.interval = SYSPROF_INTERVAL_DEFAULT;
+ stream_init(&opt);
+
+ /*
+ * XXX: Payload function on top will not be removed if any
+ * of those assertions fail. So, the next call to the
+ * `utils_get_aux_lfunc()` will fail. It's OK, since
+ * we are already in trouble, just keep it in mind.
+ */
+ assert_true(luaM_sysprof_set_writer(buffer_writer_default)
+ == PROFILE_SUCCESS);
+ assert_true(luaM_sysprof_set_on_stop(on_stop_cb_default)
+ == PROFILE_SUCCESS);
+ assert_true(luaM_sysprof_set_backtracer(NULL) == PROFILE_SUCCESS);
+
+ status = luaM_sysprof_start(L, &opt);
+ assert_true(status == PROFILE_SUCCESS);
+
+ /* Run payload. */
+ if (lua_pcall(L, 0, 0, 0) != LUA_OK) {
+ test_comment("error running payload: %s", lua_tostring(L, -1));
+ bail_out("error running sysprof test payload");
+ }
+
+ status = luaM_sysprof_stop(L);
+ assert_true(status == PROFILE_SUCCESS);
+
+ status = luaM_sysprof_report(&cnt);
+ assert_true(status == PROFILE_SUCCESS);
+
+ assert_true(cnt.samples > 1);
+ assert_true(cnt.samples == cnt.vmst_asm +
+ cnt.vmst_cfunc +
+ cnt.vmst_exit +
+ cnt.vmst_ffunc +
+ cnt.vmst_gc +
+ cnt.vmst_interp +
+ cnt.vmst_lfunc +
+ cnt.vmst_opt +
+ cnt.vmst_record +
+ cnt.vmst_trace);
+
+ return TEST_EXIT_SUCCESS;
+}
+#endif
+
+static int profile_func_jitoff(void *test_state)
+{
+ UNUSED(test_state);
+ return todo("Need to replace backtrace with libunwind first");
+#if 0
+ lua_State *L = test_state;
+ utils_get_aux_lfunc(L);
+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_OFF);
+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_FLUSH);
+ check_profile_func(L);
+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_ON);
+ return TEST_EXIT_SUCCESS;
+#endif
+}
+
+static int profile_func_jiton(void *test_state)
+{
+ UNUSED(test_state);
+ return todo("Need to replace backtrace with libunwind first");
+#if 0
+ lua_State *L = test_state;
+ utils_get_aux_lfunc(L);
+ check_profile_func(L);
+ return TEST_EXIT_SUCCESS;
+#endif
+}
+
+int main(void)
+{
+ if (LUAJIT_OS != LUAJIT_OS_LINUX)
+ return skip_all("Sysprof is implemented for Linux only");
+ if (LUAJIT_TARGET != LUAJIT_ARCH_X86
+ && LUAJIT_TARGET != LUAJIT_ARCH_X64)
+ return skip_all("Sysprof is implemented for x86_64 only");
+
+ lua_State *L = utils_lua_init();
+
+ lua_pushcfunction(L, c_payload);
+ lua_setfield(L, LUA_GLOBALSINDEX, "c_payload");
+ utils_load_aux_script(L);
+
+ const struct test_unit tgroup[] = {
+ test_unit_new(base),
+ test_unit_new(validation),
+ test_unit_new(profile_func_jitoff),
+ test_unit_new(profile_func_jiton)
+ };
+ const int test_result = test_run_group(tgroup, L);
+ utils_lua_close(L);
+ return test_result;
+}
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index b4ce39d3..b1c7207f 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -66,7 +66,6 @@ add_subdirectory(lj-416-xor-before-jcc)
add_subdirectory(lj-601-fix-gc-finderrfunc)
add_subdirectory(lj-727-lightuserdata-itern)
add_subdirectory(lj-flush-on-trace)
-add_subdirectory(misclib-sysprof-capi)
# The part of the memory profiler toolchain is located in tools
# directory, jit, profiler, and bytecode toolchains are located
diff --git a/test/tarantool-tests/misclib-sysprof-capi.test.lua b/test/tarantool-tests/misclib-sysprof-capi.test.lua
deleted file mode 100644
index 4395bce3..00000000
--- a/test/tarantool-tests/misclib-sysprof-capi.test.lua
+++ /dev/null
@@ -1,54 +0,0 @@
-local tap = require("tap")
-local test = tap.test("clib-misc-sysprof"):skipcond({
- ["Sysprof is implemented for x86_64 only"] = jit.arch ~= "x86" and
- jit.arch ~= "x64",
- ["Sysprof is implemented for Linux only"] = jit.os ~= "Linux",
-})
-
-test:plan(2)
-
-local testsysprof = require("testsysprof")
-
-jit.off()
-
-test:ok(testsysprof.base())
-test:ok(testsysprof.validation())
-
--- FIXME: The following two tests are disabled because sometimes
--- `backtrace` dynamically loads a platform-specific unwinder, which is
--- not signal-safe.
---[[
-local function lua_payload(n)
- if n <= 1 then
- return n
- end
- return lua_payload(n - 1) + lua_payload(n - 2)
-end
-
-local function payload()
- local n_iterations = 500000
-
- local co = coroutine.create(function ()
- for i = 1, n_iterations do
- if i % 2 == 0 then
- testsysprof.c_payload(10)
- else
- lua_payload(10)
- end
- coroutine.yield()
- end
- end)
-
- for _ = 1, n_iterations do
- coroutine.resume(co)
- end
-end
-
-test:ok(testsysprof.profile_func(payload))
-
-jit.on()
-jit.flush()
-
-test:ok(testsysprof.profile_func(payload))
---]]
-os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt b/test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt
deleted file mode 100644
index d9fb1a1a..00000000
--- a/test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt
+++ /dev/null
@@ -1 +0,0 @@
-BuildTestCLib(testsysprof testsysprof.c)
diff --git a/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c b/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
deleted file mode 100644
index d7a3e355..00000000
--- a/test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
+++ /dev/null
@@ -1,260 +0,0 @@
-#include <lua.h>
-#include <luajit.h>
-#include <lauxlib.h>
-
-#include <lmisclib.h>
-
-#include <fcntl.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <errno.h>
-
-#undef NDEBUG
-#include <assert.h>
-
-#define UNUSED(x) ((void)(x))
-
-/* --- utils -------------------------------------------------------------- */
-
-#define SYSPROF_INTERVAL_DEFAULT 100
-
-/*
- ** Yep, 8Mb. Tuned in order not to bother the platform with too often flushes.
- */
-#define STREAM_BUFFER_SIZE (8 * 1024 * 1024)
-
-/* Structure given as ctx to sysprof writer and on_stop callback. */
-struct sysprof_ctx {
- /* Output file descriptor for data. */
- int fd;
- /* Buffer for data. */
- uint8_t buf[STREAM_BUFFER_SIZE];
-};
-
-/*
- ** Default buffer writer function.
- ** Just call fwrite to the corresponding FILE.
- */
-static size_t buffer_writer_default(const void **buf_addr, size_t len,
- void *opt)
-{
- struct sysprof_ctx *ctx = opt;
- int fd = ctx->fd;
- const void * const buf_start = *buf_addr;
- const void *data = *buf_addr;
- size_t write_total = 0;
-
- assert(len <= STREAM_BUFFER_SIZE);
-
- for (;;) {
- const size_t written = write(fd, data, len - write_total);
-
- if (written == 0) {
- /* Re-tries write in case of EINTR. */
- if (errno != EINTR) {
- /* Will be freed as whole chunk later. */
- *buf_addr = NULL;
- return write_total;
- }
- errno = 0;
- continue;
- }
-
- write_total += written;
- assert(write_total <= len);
-
- if (write_total == len)
- break;
-
- data = (uint8_t *)data + (ptrdiff_t)written;
- }
-
- *buf_addr = buf_start;
- return write_total;
-}
-
-/* Default on stop callback. Just close the corresponding stream. */
-static int on_stop_cb_default(void *opt, uint8_t *buf)
-{
- UNUSED(buf);
- struct sysprof_ctx *ctx = opt;
- int fd = ctx->fd;
- free(ctx);
- return close(fd);
-}
-
-static int stream_init(struct luam_Sysprof_Options *opt)
-{
- struct sysprof_ctx *ctx = calloc(1, sizeof(struct sysprof_ctx));
- if (NULL == ctx)
- return PROFILE_ERRIO;
-
- ctx->fd = open("/dev/null", O_WRONLY | O_CREAT, 0644);
- if (-1 == ctx->fd) {
- free(ctx);
- return PROFILE_ERRIO;
- }
-
- opt->ctx = ctx;
- opt->buf = ctx->buf;
- opt->len = STREAM_BUFFER_SIZE;
-
- return PROFILE_SUCCESS;
-}
-
-/* --- Payload ------------------------------------------------------------ */
-
-static double fib(double n)
-{
- if (n <= 1)
- return n;
- return fib(n - 1) + fib(n - 2);
-}
-
-static int c_payload(lua_State *L)
-{
- fib(luaL_checknumber(L, 1));
- lua_pushboolean(L, 1);
- return 1;
-}
-
-/* --- sysprof C API tests ------------------------------------------------ */
-
-static int base(lua_State *L)
-{
- struct luam_Sysprof_Options opt = {};
- struct luam_Sysprof_Counters cnt = {};
-
- (void)opt.interval;
- (void)opt.mode;
- (void)opt.ctx;
- (void)opt.buf;
- (void)opt.len;
-
- luaM_sysprof_report(&cnt);
-
- (void)cnt.samples;
- (void)cnt.vmst_interp;
- (void)cnt.vmst_lfunc;
- (void)cnt.vmst_ffunc;
- (void)cnt.vmst_cfunc;
- (void)cnt.vmst_gc;
- (void)cnt.vmst_exit;
- (void)cnt.vmst_record;
- (void)cnt.vmst_opt;
- (void)cnt.vmst_asm;
- (void)cnt.vmst_trace;
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int validation(lua_State *L)
-{
- struct luam_Sysprof_Options opt = {};
- int status = PROFILE_SUCCESS;
-
- /* Unknown mode. */
- opt.mode = 0x40;
- status = luaM_sysprof_start(L, &opt);
- assert(PROFILE_ERRUSE == status);
-
- /* Buffer not configured. */
- opt.mode = LUAM_SYSPROF_CALLGRAPH;
- opt.buf = NULL;
- status = luaM_sysprof_start(L, &opt);
- assert(PROFILE_ERRUSE == status);
-
- /* Bad interval. */
- opt.mode = LUAM_SYSPROF_DEFAULT;
- opt.interval = 0;
- status = luaM_sysprof_start(L, &opt);
- assert(PROFILE_ERRUSE == status);
-
- /* Check if profiling started. */
- opt.mode = LUAM_SYSPROF_DEFAULT;
- opt.interval = SYSPROF_INTERVAL_DEFAULT;
- status = luaM_sysprof_start(L, &opt);
- assert(PROFILE_SUCCESS == status);
-
- /* Already running. */
- status = luaM_sysprof_start(L, &opt);
- assert(PROFILE_ERRRUN == status);
-
- /* Profiler stopping. */
- status = luaM_sysprof_stop(L);
- assert(PROFILE_SUCCESS == status);
-
- /* Stopping profiler which is not running. */
- status = luaM_sysprof_stop(L);
- assert(PROFILE_ERRRUN == status);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static int profile_func(lua_State *L)
-{
- struct luam_Sysprof_Options opt = {};
- struct luam_Sysprof_Counters cnt = {};
- int status = PROFILE_ERRUSE;
-
- int n = lua_gettop(L);
- if (n != 1 || !lua_isfunction(L, 1))
- luaL_error(L, "incorrect argument: 1 function is required");
-
- /*
- ** Since all the other modes functionality is the
- ** subset of CALLGRAPH mode, run this mode to test
- ** the profiler's behavior.
- */
- opt.mode = LUAM_SYSPROF_CALLGRAPH;
- opt.interval = SYSPROF_INTERVAL_DEFAULT;
- stream_init(&opt);
-
- assert(luaM_sysprof_set_writer(buffer_writer_default) == PROFILE_SUCCESS);
- assert(luaM_sysprof_set_on_stop(on_stop_cb_default) == PROFILE_SUCCESS);
- assert(luaM_sysprof_set_backtracer(NULL) == PROFILE_SUCCESS);
-
- status = luaM_sysprof_start(L, &opt);
- assert(PROFILE_SUCCESS == status);
-
- /* Run payload. */
- if (lua_pcall(L, 0, 0, 0) != LUA_OK)
- luaL_error(L, "error running payload: %s", lua_tostring(L, -1));
-
- status = luaM_sysprof_stop(L);
- assert(PROFILE_SUCCESS == status);
-
- status = luaM_sysprof_report(&cnt);
- assert(PROFILE_SUCCESS == status);
-
- assert(cnt.samples > 1);
- assert(cnt.samples == cnt.vmst_asm +
- cnt.vmst_cfunc +
- cnt.vmst_exit +
- cnt.vmst_ffunc +
- cnt.vmst_gc +
- cnt.vmst_interp +
- cnt.vmst_lfunc +
- cnt.vmst_opt +
- cnt.vmst_record +
- cnt.vmst_trace);
-
- lua_pushboolean(L, 1);
- return 1;
-}
-
-static const struct luaL_Reg testsysprof[] = {
- {"base", base},
- {"c_payload", c_payload},
- {"profile_func", profile_func},
- {"validation", validation},
- {NULL, NULL}
-};
-
-LUA_API int luaopen_testsysprof(lua_State *L)
-{
- luaL_register(L, "testsysprof", testsysprof);
- return 1;
-}
--
2.34.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 5/6] test: rewrite misclib-sysprof-capi test in C
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 5/6] test: rewrite misclib-sysprof-capi " Sergey Kaplun via Tarantool-patches
@ 2023-05-19 13:00 ` Maxim Kokryashkin via Tarantool-patches
2023-05-20 7:28 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 29+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-19 13:00 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 10498 bytes --]
Hi, Sergey!
Thanks for the patch!
LGTM, except for two questions below.
>
>>This patch rewrites the aforementioned test with the usage libtest
>>recently introduced. The approach is similar to the previous patch.
>>`c_payload()` function to profile is now set up globally on script
>>loading. The payload for enabled and disabled JIT is the same. As far as
>>for sysprof testing is necessary to replace backtrace with libunwind
>>first, the payload tests are marked with `todo()`.
>>
>>Nevertheless, glibc `assert()` still necessary to check the correctness
>>of the profile writer, so it is still used there.
>>
>>Relates to tarantool/tarantool#781
>>Part of tarantool/tarantool#7900
>>---
>> .../misclib-sysprof-capi-script.lua | 35 ++
>> .../misclib-sysprof-capi.test.c | 325 ++++++++++++++++++
>> test/tarantool-tests/CMakeLists.txt | 1 -
>> .../misclib-sysprof-capi.test.lua | 54 ---
>> .../misclib-sysprof-capi/CMakeLists.txt | 1 -
>> .../misclib-sysprof-capi/testsysprof.c | 260 --------------
>> 6 files changed, 360 insertions(+), 316 deletions(-)
>> create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi-script.lua
>> create mode 100644 test/tarantool-c-tests/misclib-sysprof-capi.test.c
>> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi.test.lua
>> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/CMakeLists.txt
>> delete mode 100644 test/tarantool-tests/misclib-sysprof-capi/testsysprof.c
>>
>>diff --git a/test/tarantool-c-tests/misclib-sysprof-capi-script.lua b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua
>>new file mode 100644
>>index 00000000..dd8387db
>>--- /dev/null
>>+++ b/test/tarantool-c-tests/misclib-sysprof-capi-script.lua
>><snipped>
>>
>>diff --git a/test/tarantool-c-tests/misclib-sysprof-capi.test.c b/test/tarantool-c-tests/misclib-sysprof-capi.test.cnew file mode 100644
>>index 00000000..f75f82cd
>>--- /dev/null
>>+++ b/test/tarantool-c-tests/misclib-sysprof-capi.test.c
>>@@ -0,0 +1,325 @@
>>+#include "lauxlib.h"
>>+#include "lmisclib.h"
>>+#include "lua.h"
>>+#include "luajit.h"
>>+
>>+#include "test.h"
>>+#include "utils.h"
>>+
>>+#include <errno.h>
>>+#include <fcntl.h>
>>+#include <stdlib.h>
>>+#include <unistd.h>
>>+
>>+/* XXX: Still need normal assert inside writer functions. */
>>+#undef NDEBUG
>>+#include <assert.h>
>>+
>>+/* Need for skipcond for OS and ARCH. */
>>+#include "lj_arch.h"
>>+
>>+#define UNUSED(x) ((void)(x))
>>+
>>+/* --- utils -------------------------------------------------- */
>>+
>>+#define SYSPROF_INTERVAL_DEFAULT 100
>>+
>>+/*
>>+ * Yep, 8Mb. Tuned in order not to bother the platform with too
>>+ * often flushes.
>>+ */
>>+#define STREAM_BUFFER_SIZE (8 * 1024 * 1024)
>>+
>>+
>>+/* --- C Payload ---------------------------------------------- */
>>+
>>+static double fib(double n)
>>+{
>>+ if (n <= 1)
>>+ return n;
>>+ return fib(n - 1) + fib(n - 2);
>>+}
>>+
>>+static int c_payload(lua_State *L)
>>+{
>>+ fib(luaL_checknumber(L, 1));
>>+ lua_pushboolean(L, 1);
>>+ return 1;
>>+}
>>+/* --- sysprof C API tests ------------------------------------ */
>>+
>>+static int base(void *test_state)
>>+{
>>+ UNUSED(test_state);
>>+ struct luam_Sysprof_Options opt = {};
>>+ struct luam_Sysprof_Counters cnt = {};
>>+
>>+ (void)opt.interval;
>>+ (void)opt.mode;
>>+ (void)opt.ctx;
>>+ (void)opt.buf;
>>+ (void)opt.len;
>>+
>>+ luaM_sysprof_report(&cnt);
>>+
>>+ (void)cnt.samples;
>>+ (void)cnt.vmst_interp;
>>+ (void)cnt.vmst_lfunc;
>>+ (void)cnt.vmst_ffunc;
>>+ (void)cnt.vmst_cfunc;
>>+ (void)cnt.vmst_gc;
>>+ (void)cnt.vmst_exit;
>>+ (void)cnt.vmst_record;
>>+ (void)cnt.vmst_opt;
>>+ (void)cnt.vmst_asm;
>>+ (void)cnt.vmst_trace;
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+static int validation(void *test_state)
>>+{
>>+ lua_State *L = test_state;
>>+ struct luam_Sysprof_Options opt = {};
>>+ int status = PROFILE_SUCCESS;
>>+
>>+ /* Unknown mode. */
>>+ opt.mode = 0x40;
>>+ status = luaM_sysprof_start(L, &opt);
>>+ assert_true(status == PROFILE_ERRUSE);
>>+
>>+ /* Buffer not configured. */
>>+ opt.mode = LUAM_SYSPROF_CALLGRAPH;
>>+ opt.buf = NULL;
>>+ status = luaM_sysprof_start(L, &opt);
>>+ assert_true(status == PROFILE_ERRUSE);
>>+
>>+ /* Bad interval. */
>>+ opt.mode = LUAM_SYSPROF_DEFAULT;
>>+ opt.interval = 0;
>>+ status = luaM_sysprof_start(L, &opt);
>>+ assert_true(status == PROFILE_ERRUSE);
>>+
>>+ /* Check if profiling started. */
>>+ opt.mode = LUAM_SYSPROF_DEFAULT;
>>+ opt.interval = SYSPROF_INTERVAL_DEFAULT;
>>+ status = luaM_sysprof_start(L, &opt);
>>+ assert_true(status == PROFILE_SUCCESS);
>>+
>>+ /* Already running. */
>>+ status = luaM_sysprof_start(L, &opt);
>>+ assert_true(status == PROFILE_ERRRUN);
>>+
>>+ /* Profiler stopping. */
>>+ status = luaM_sysprof_stop(L);
>>+ assert_true(status == PROFILE_SUCCESS);
>>+
>>+ /* Stopping profiler which is not running. */
>>+ status = luaM_sysprof_stop(L);
>>+ assert_true(status == PROFILE_ERRRUN);
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+
>>+/*
>>+ * FIXME: The following two tests are disabled because sometimes
>>+ * `backtrace` dynamically loads a platform-specific unwinder,
>>+ * which is not signal-safe.
>>+ */
>>+
>>+#if 0
>>+/*
>>+ * Structure given as ctx to sysprof writer and on_stop callback.
>>+ */
>>+struct sysprof_ctx {
>>+ /* Output file descriptor for data. */
>>+ int fd;
>>+ /* Buffer for data. */
>>+ uint8_t buf[STREAM_BUFFER_SIZE];
>>+};
>>+
>>+/*
>>+ * Default buffer writer function.
>>+ * Just call fwrite to the corresponding FILE.
>>+ */
>>+static size_t buffer_writer_default(const void **buf_addr, size_t len,
>>+ void *opt)
>>+{
>>+ struct sysprof_ctx *ctx = opt;
>>+ int fd = ctx->fd;
>>+ const void * const buf_start = *buf_addr;
>>+ const void *data = *buf_addr;
>>+ size_t write_total = 0;
>>+
>>+ assert(len <= STREAM_BUFFER_SIZE);
>>+
>>+ for (;;) {
>>+ const size_t written = write(fd, data, len - write_total);
>>+
>>+ if (written == 0) {
>>+ /* Re-tries write in case of EINTR. */
>>+ if (errno != EINTR) {
>>+ /*
>>+ * Will be freed as whole chunk
>>+ * later.
>>+ */
>>+ *buf_addr = NULL;
>>+ return write_total;
>>+ }
>>+ errno = 0;
>>+ continue;
>>+ }
>>+
>>+ write_total += written;
>>+ assert(write_total <= len);
>>+
>>+ if (write_total == len)
>>+ break;
>>+
>>+ data = (uint8_t *)data + (ptrdiff_t)written;
>>+ }
>>+
>>+ *buf_addr = buf_start;
>>+ return write_total;
>>+}
>>+
>>+/*
>>+ * Default on stop callback. Just close the corresponding stream.
>>+ */
>>+static int on_stop_cb_default(void *opt, uint8_t *buf)
>>+{
>>+ UNUSED(buf);
>>+ struct sysprof_ctx *ctx = opt;
>>+ int fd = ctx->fd;
>>+ free(ctx);
>>+ return close(fd);
>>+}
>>+
>>+static int stream_init(struct luam_Sysprof_Options *opt)
>>+{
>>+ struct sysprof_ctx *ctx = calloc(1, sizeof(struct sysprof_ctx));
>>+ if (NULL == ctx)
>>+ return PROFILE_ERRIO;
>>+
>>+ ctx->fd = open("/dev/null", O_WRONLY | O_CREAT, 0644);
>>+ if (-1 == ctx->fd) {
>>+ free(ctx);
>>+ return PROFILE_ERRIO;
>>+ }
>>+
>>+ opt->ctx = ctx;
>>+ opt->buf = ctx->buf;
>>+ opt->len = STREAM_BUFFER_SIZE;
>>+
>>+ return PROFILE_SUCCESS;
>>+}
>>+
>>+/* Get function to profile on top, call it. */
>>+static int check_profile_func(lua_State *L)
>>+{
>>+ struct luam_Sysprof_Options opt = {};
>>+ struct luam_Sysprof_Counters cnt = {};
>>+ int status = PROFILE_ERRUSE;
>>+ /*
>>+ * Since all the other modes functionality is the
>>+ * subset of CALLGRAPH mode, run this mode to test
>>+ * the profiler's behavior.
>>+ */
>>+ opt.mode = LUAM_SYSPROF_CALLGRAPH;
>>+ opt.interval = SYSPROF_INTERVAL_DEFAULT;
>>+ stream_init(&opt);
>>+
>>+ /*
>>+ * XXX: Payload function on top will not be removed if any
>>+ * of those assertions fail. So, the next call to the
>>+ * `utils_get_aux_lfunc()` will fail. It's OK, since
>>+ * we are already in trouble, just keep it in mind.
>>+ */
>>+ assert_true(luaM_sysprof_set_writer(buffer_writer_default)
>>+ == PROFILE_SUCCESS);
>>+ assert_true(luaM_sysprof_set_on_stop(on_stop_cb_default)
>>+ == PROFILE_SUCCESS);
>>+ assert_true(luaM_sysprof_set_backtracer(NULL) == PROFILE_SUCCESS);
>>+
>>+ status = luaM_sysprof_start(L, &opt);
>>+ assert_true(status == PROFILE_SUCCESS);
>>+
>>+ /* Run payload. */
>>+ if (lua_pcall(L, 0, 0, 0) != LUA_OK) {
>>+ test_comment("error running payload: %s", lua_tostring(L, -1));
>>+ bail_out("error running sysprof test payload");
>>+ }
>>+
>>+ status = luaM_sysprof_stop(L);
>>+ assert_true(status == PROFILE_SUCCESS);
>>+
>>+ status = luaM_sysprof_report(&cnt);
>>+ assert_true(status == PROFILE_SUCCESS);
>>+
>>+ assert_true(cnt.samples > 1);
>>+ assert_true(cnt.samples == cnt.vmst_asm +
>>+ cnt.vmst_cfunc +
>>+ cnt.vmst_exit +
>>+ cnt.vmst_ffunc +
>>+ cnt.vmst_gc +
>>+ cnt.vmst_interp +
>>+ cnt.vmst_lfunc +
>>+ cnt.vmst_opt +
>>+ cnt.vmst_record +
>>+ cnt.vmst_trace);
>>+
>>+ return TEST_EXIT_SUCCESS;
>>+}
>>+#endif
>>+
>>+static int profile_func_jitoff(void *test_state)
>>+{
>>+ UNUSED(test_state);
>>+ return todo("Need to replace backtrace with libunwind first");
>>+#if 0
>>+ lua_State *L = test_state;
>>+ utils_get_aux_lfunc(L);
>>+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_OFF);
>>+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_FLUSH);
>>+ check_profile_func(L);
>>+ (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_ON);
>>+ return TEST_EXIT_SUCCESS;
>>+#endif
>>+}
>>+
>>+static int profile_func_jiton(void *test_state)
>>+{
>>+ UNUSED(test_state);
>>+ return todo("Need to replace backtrace with libunwind first");
>>+#if 0
>>+ lua_State *L = test_state;
>>+ utils_get_aux_lfunc(L);
>>+ check_profile_func(L);
>>+ return TEST_EXIT_SUCCESS;
>>+#endif
>>+}
>>+
>>+int main(void)
>>+{
>>+ if (LUAJIT_OS != LUAJIT_OS_LINUX)
>>+ return skip_all("Sysprof is implemented for Linux only");
>>+ if (LUAJIT_TARGET != LUAJIT_ARCH_X86
>>+ && LUAJIT_TARGET != LUAJIT_ARCH_X64)
>>+ return skip_all("Sysprof is implemented for x86_64 only");
>>+
>>+ lua_State *L = utils_lua_init();
>>+
>>+ lua_pushcfunction(L, c_payload);
>>+ lua_setfield(L, LUA_GLOBALSINDEX, "c_payload");
>Is there a cleaner approach?
>>+ utils_load_aux_script(L);
>>+
>>+ const struct test_unit tgroup[] = {
>>+ test_unit_new(base),
>>+ test_unit_new(validation),
>>+ test_unit_new(profile_func_jitoff),
>>+ test_unit_new(profile_func_jiton)
>Maybe we shouldn’t put these tests into group as long as they are disabled?
>>+ };
>>+ const int test_result = test_run_group(tgroup, L);
>>+ utils_lua_close(L);
>>+ return test_result;
>>+}
>><snipped>
>--
>Best regards,
>Maxim Kokryashkin
>
[-- Attachment #2: Type: text/html, Size: 12107 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 5/6] test: rewrite misclib-sysprof-capi test in C
2023-05-19 13:00 ` Maxim Kokryashkin via Tarantool-patches
@ 2023-05-20 7:28 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-20 7:28 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Maxim!
Thanks for the review!
On 19.05.23, Maxim Kokryashkin wrote:
>
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for two questions below.
>
> >
<snipped>
> >>+
> >>+ lua_pushcfunction(L, c_payload);
> >>+ lua_setfield(L, LUA_GLOBALSINDEX, "c_payload");
> >Is there a cleaner approach?
I suppose no. I think that set the global value is the simpliest way for
this test.
> >>+ utils_load_aux_script(L);
> >>+
> >>+ const struct test_unit tgroup[] = {
> >>+ test_unit_new(base),
> >>+ test_unit_new(validation),
> >>+ test_unit_new(profile_func_jitoff),
> >>+ test_unit_new(profile_func_jiton)
> >Maybe we shouldn’t put these tests into group as long as they are disabled?
We still may skip them via TODO directive, so let them as is in the
output.
I'll wait for the 2nd review opinion and either use `#if 0` for comment
or double return.
> >>+ };
> >>+ const int test_result = test_run_group(tgroup, L);
> >>+ utils_lua_close(L);
> >>+ return test_result;
> >>+}
> >><snipped>
> >--
> >Best regards,
> >Maxim Kokryashkin
> >
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Tarantool-patches] [PATCH v2 luajit 6/6] test: rewrite lj-49-bad-lightuserdata test in C
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
` (4 preceding siblings ...)
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 5/6] test: rewrite misclib-sysprof-capi " Sergey Kaplun via Tarantool-patches
@ 2023-05-18 20:44 ` Sergey Kaplun via Tarantool-patches
2023-05-19 12:40 ` Maxim Kokryashkin via Tarantool-patches
2023-05-19 14:29 ` [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Maxim Kokryashkin via Tarantool-patches
6 siblings, 1 reply; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-18 20:44 UTC (permalink / raw)
To: Igor Munkin, Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
This patch rewrites the aforementioned test with the usage libtest
recently introduced. The approach is similar to the previous patch.
Nevertheless, glibc `assert()` is used to check the correctness
of the `mmap()` usage.
Part of tarantool/tarantool#7900
---
.../lj-49-bad-lightuserdata.test.c} | 47 ++++++++++---------
test/tarantool-tests/CMakeLists.txt | 1 -
.../lj-49-bad-lightuserdata.test.lua | 11 -----
.../lj-49-bad-lightuserdata/CMakeLists.txt | 1 -
4 files changed, 25 insertions(+), 35 deletions(-)
rename test/{tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c => tarantool-c-tests/lj-49-bad-lightuserdata.test.c} (55%)
delete mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
delete mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c b/test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
similarity index 55%
rename from test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
rename to test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
index 1b909fc6..a9cc4763 100644
--- a/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
+++ b/test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
@@ -1,16 +1,21 @@
-#include <lua.h>
-#include <lauxlib.h>
+#include "lua.h"
+#include "lauxlib.h"
#include <sys/mman.h>
#include <unistd.h>
-#undef NDEBUG
-#include <assert.h>
+#include "test.h"
+#include "utils.h"
#define START ((void *)-1)
-static int crafted_ptr(lua_State *L)
+/* XXX: Still need normal assert to validate mmap correctness. */
+#undef NDEBUG
+#include <assert.h>
+
+static int crafted_ptr(void *test_state)
{
+ lua_State *L = test_state;
/*
* We know that for arm64 at least 48 bits are available.
* So emulate manually push of lightuseradata within
@@ -18,15 +23,15 @@ static int crafted_ptr(lua_State *L)
*/
void *longptr = (void *)(1llu << 48);
lua_pushlightuserdata(L, longptr);
- assert(longptr == lua_topointer(L, -1));
+ assert_ptr_equal(longptr, lua_topointer(L, -1));
/* Clear our stack. */
lua_pop(L, 0);
- lua_pushboolean(L, 1);
- return 1;
+ return TEST_EXIT_SUCCESS;
}
-static int mmaped_ptr(lua_State *L)
+static int mmaped_ptr(void *test_state)
{
+ lua_State *L = test_state;
/*
* If start mapping address is not NULL, then the kernel
* takes it as a hint about where to place the mapping, so
@@ -38,24 +43,22 @@ static int mmaped_ptr(lua_State *L)
-1, 0);
if (mmaped != MAP_FAILED) {
lua_pushlightuserdata(L, mmaped);
- assert(mmaped == lua_topointer(L, -1));
+ assert_ptr_equal(mmaped, lua_topointer(L, -1));
assert(munmap(mmaped, pagesize) == 0);
}
/* Clear our stack. */
lua_pop(L, 0);
- lua_pushboolean(L, 1);
- return 1;
+ return TEST_EXIT_SUCCESS;
}
-static const struct luaL_Reg testlightuserdata[] = {
- {"crafted_ptr", crafted_ptr},
- {"mmaped_ptr", mmaped_ptr},
- {NULL, NULL}
-};
-
-LUA_API int luaopen_testlightuserdata(lua_State *L)
+int main(void)
{
- luaL_register(L, "testlightuserdata", testlightuserdata);
- return 1;
+ lua_State *L = utils_lua_init();
+ const struct test_unit tgroup[] = {
+ test_unit_new(crafted_ptr),
+ test_unit_new(mmaped_ptr)
+ };
+ const int test_result = test_run_group(tgroup, L);
+ utils_lua_close(L);
+ return test_result;
}
-
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index b1c7207f..527905b6 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -61,7 +61,6 @@ add_subdirectory(gh-5813-resolving-of-c-symbols/gnuhash)
add_subdirectory(gh-5813-resolving-of-c-symbols/stripped)
add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
add_subdirectory(gh-6189-cur_L)
-add_subdirectory(lj-49-bad-lightuserdata)
add_subdirectory(lj-416-xor-before-jcc)
add_subdirectory(lj-601-fix-gc-finderrfunc)
add_subdirectory(lj-727-lightuserdata-itern)
diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua b/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
deleted file mode 100644
index 94a743c7..00000000
--- a/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
+++ /dev/null
@@ -1,11 +0,0 @@
-local tap = require('tap')
-
-local test = tap.test('lj-49-bad-lightuserdata')
-test:plan(2)
-
-local testlightuserdata = require('testlightuserdata')
-
-test:ok(testlightuserdata.crafted_ptr())
-test:ok(testlightuserdata.mmaped_ptr())
-
-os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt b/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
deleted file mode 100644
index ec6bb62c..00000000
--- a/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
+++ /dev/null
@@ -1 +0,0 @@
-BuildTestCLib(testlightuserdata testlightuserdata.c)
--
2.34.1
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 6/6] test: rewrite lj-49-bad-lightuserdata test in C
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 6/6] test: rewrite lj-49-bad-lightuserdata " Sergey Kaplun via Tarantool-patches
@ 2023-05-19 12:40 ` Maxim Kokryashkin via Tarantool-patches
0 siblings, 0 replies; 29+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-19 12:40 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 4944 bytes --]
Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
This patch rewrites the aforementioned test with the usage libtest
recently introduced. The approach is similar to the previous patch.
Nevertheless, glibc `assert()` is used to check the correctness
of the `mmap()` usage.
Part of tarantool/tarantool#7900
---
.../lj-49-bad-lightuserdata.test.c} | 47 ++++++++++---------
test/tarantool-tests/CMakeLists.txt | 1 -
.../lj-49-bad-lightuserdata.test.lua | 11 -----
.../lj-49-bad-lightuserdata/CMakeLists.txt | 1 -
4 files changed, 25 insertions(+), 35 deletions(-)
rename test/{tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c => tarantool-c-tests/lj-49-bad-lightuserdata.test.c} (55%)
delete mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
delete mode 100644 test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c b/test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
similarity index 55%
rename from test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
rename to test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
index 1b909fc6..a9cc4763 100644
--- a/test/tarantool-tests/lj-49-bad-lightuserdata/testlightuserdata.c
+++ b/test/tarantool-c-tests/lj-49-bad-lightuserdata.test.c
@@ -1,16 +1,21 @@
-#include <lua.h>
-#include <lauxlib.h>
+#include "lua.h"
+#include "lauxlib.h"
#include <sys/mman.h>
#include <unistd.h>
-#undef NDEBUG
-#include <assert.h>
+#include "test.h"
+#include "utils.h"
#define START ((void *)-1)
-static int crafted_ptr(lua_State *L)
+/* XXX: Still need normal assert to validate mmap correctness. */
+#undef NDEBUG
+#include <assert.h>
+
+static int crafted_ptr(void *test_state)
{
+ lua_State *L = test_state;
/*
* We know that for arm64 at least 48 bits are available.
* So emulate manually push of lightuseradata within
@@ -18,15 +23,15 @@ static int crafted_ptr(lua_State *L)
*/
void *longptr = (void *)(1llu << 48);
lua_pushlightuserdata(L, longptr);
- assert(longptr == lua_topointer(L, -1));
+ assert_ptr_equal(longptr, lua_topointer(L, -1));
/* Clear our stack. */
lua_pop(L, 0);
- lua_pushboolean(L, 1);
- return 1;
+ return TEST_EXIT_SUCCESS;
}
-static int mmaped_ptr(lua_State *L)
+static int mmaped_ptr(void *test_state)
{
+ lua_State *L = test_state;
/*
* If start mapping address is not NULL, then the kernel
* takes it as a hint about where to place the mapping, so
@@ -38,24 +43,22 @@ static int mmaped_ptr(lua_State *L)
-1, 0);
if (mmaped != MAP_FAILED) {
lua_pushlightuserdata(L, mmaped);
- assert(mmaped == lua_topointer(L, -1));
+ assert_ptr_equal(mmaped, lua_topointer(L, -1));
assert(munmap(mmaped, pagesize) == 0);
}
/* Clear our stack. */
lua_pop(L, 0);
- lua_pushboolean(L, 1);
- return 1;
+ return TEST_EXIT_SUCCESS;
}
-static const struct luaL_Reg testlightuserdata[] = {
- {"crafted_ptr", crafted_ptr},
- {"mmaped_ptr", mmaped_ptr},
- {NULL, NULL}
-};
-
-LUA_API int luaopen_testlightuserdata(lua_State *L)
+int main(void)
{
- luaL_register(L, "testlightuserdata", testlightuserdata);
- return 1;
+ lua_State *L = utils_lua_init();
+ const struct test_unit tgroup[] = {
+ test_unit_new(crafted_ptr),
+ test_unit_new(mmaped_ptr)
+ };
+ const int test_result = test_run_group(tgroup, L);
+ utils_lua_close(L);
+ return test_result;
}
-
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index b1c7207f..527905b6 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -61,7 +61,6 @@ add_subdirectory(gh-5813-resolving-of-c-symbols/gnuhash)
add_subdirectory(gh-5813-resolving-of-c-symbols/stripped)
add_subdirectory(gh-6098-fix-side-exit-patching-on-arm64)
add_subdirectory(gh-6189-cur_L)
-add_subdirectory(lj-49-bad-lightuserdata)
add_subdirectory(lj-416-xor-before-jcc)
add_subdirectory(lj-601-fix-gc-finderrfunc)
add_subdirectory(lj-727-lightuserdata-itern)
diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua b/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
deleted file mode 100644
index 94a743c7..00000000
--- a/test/tarantool-tests/lj-49-bad-lightuserdata.test.lua
+++ /dev/null
@@ -1,11 +0,0 @@
-local tap = require('tap')
-
-local test = tap.test('lj-49-bad-lightuserdata')
-test:plan(2)
-
-local testlightuserdata = require('testlightuserdata')
-
-test:ok(testlightuserdata.crafted_ptr())
-test:ok(testlightuserdata.mmaped_ptr())
-
-os.exit(test:check() and 0 or 1)
diff --git a/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt b/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
deleted file mode 100644
index ec6bb62c..00000000
--- a/test/tarantool-tests/lj-49-bad-lightuserdata/CMakeLists.txt
+++ /dev/null
@@ -1 +0,0 @@
-BuildTestCLib(testlightuserdata testlightuserdata.c)
--
2.34.1
[-- Attachment #2: Type: text/html, Size: 6077 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Sergey Kaplun via Tarantool-patches
` (5 preceding siblings ...)
2023-05-18 20:44 ` [Tarantool-patches] [PATCH v2 luajit 6/6] test: rewrite lj-49-bad-lightuserdata " Sergey Kaplun via Tarantool-patches
@ 2023-05-19 14:29 ` Maxim Kokryashkin via Tarantool-patches
2023-05-20 8:38 ` Sergey Kaplun via Tarantool-patches
6 siblings, 1 reply; 29+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-05-19 14:29 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 5071 bytes --]
Hi!
Thanks for the comments.
>
>>The whole idea of the patch-set introduce module for LuaJIT C tests. It
>>also, can be used for unit tests.
>>* The first patch is the prerequisite for the patch-set. It fixes
>> LD_LIBRARY_PATH definition.
>>* The 2nd and 3d patches provides an API and helper for writing the tests.
>>* The last 3 patches rewrite existing tests that should be written in C in
>> the proper way.
>>
>>Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-tarantool-c-tests
>>PR: https://github.com/tarantool/tarantool/pull/8444
>>Related Issue:
>>* https://github.com/tarantool/tarantool/issues/7900
>>* https://github.com/tarantool/tarantool/issues/781
>>
>>Thanks Maxim, for the review!
>>
>>I've fixed some Maxim comments and suggestions for the previous series.
>>Some ignorable comments about wording are ignored:).
>>
>>Also, see answers for your questions below:
>>
>>> >+if(NOT PROVE)
>>> >+ message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
>>> >+ return()
>>> >+endif()
>>> There is the same check in the test/tarantool-tests/CMakeLists.txt. Maybe
>>> we should move it to the higher-level CMake so the lower-level CMakeLists
>>> inherit it.
>>
>>I agree it maybe done, but not within this particular patchsett, so
>>ignoring for now.
>Don’t see any reason to postpone it, tbh. New module requires the same
>checks as the already present one. It seems logical to do necessary changes
>in this patchset.
>>
>>> >+#define test_run_group(t_arr, t_state) \
>>> >+ _test_run_group(__func__, t_arr, lengthof(t_arr), t_state)
>>> Is there any reason for it to be a macro and not a function wrapper?
>>> I believe it is better to use the functions when possible, since they are
>>> easier to support and debug.
>>
>>Just for the convenience in usage of __func__ macro as a test group name.
>Oh, I see, thx.
>>
>>> >+/* Need for `strchr()` in diagnostic parsing. */
>>> `strchr()` is not safe, despite the fact it searches till `\0`.
>>> We should at least replace it with `memchr()`, which has
>>> the explicit constraint for buffer length.
>>> >+#include <string.h>
>>
>>Yes, but:
>>1) We use it only for our test code, where we set this `\0` directly to
>>mark EOL.
>>2) It's simplier than use several marks in buffer.
>>So, ignoring for now.
>Ok
>>
>>> >+# vim: ft=cmake expandtab shiftwidth=2: tabstop=2:
>>> That change is not necessary.
>>
>>Yes, but more convenient to use in vim -- since our usual codestyle isn't 4
>>tabs as its default for CMake. :)
>>Still we don't use it anywhere (unfortunately), so removed.
>>
>>> >+ bail_out("failed to translate Lua code snippet");
>>> Why `bail_out` and not an assertion? Here and below.
>>
>>Assertion is for some thing we wnat to test and may fail.
>>Bail out usage is more specific:
>>| As an emergency measure a test script can decide that further tests are
>>| useless (e.g. missing dependencies) and testing should stop immediately.
>>| In that case the test script prints the magic words
>>See [1]. I think that loading Lua script helper (dependency) is
>>something like that.
>Thanks for the clarification.
>>
>>> >+#include "lj_arch.h"
>>> Side note: I don't like the approach with private headers, but
>>> I couldn't find any better way to check that. Maybe it is a good
>>> idea to implement a public C API function to get the information
>>> about OS and ARCH, since it is a really common to check them?
>>
>>I think, that this is the best option, espessialy if we want to write
>>some unit test for some specific module (I mean <src/lj_*>).
>>
>>Changes in v2:
>>1) use
>>
>>| int _test_run_group(const char *group_name, const struct test_unit tests[],
>>| size_t n_tests, void *test_state);
>>
>>instead of
>>
>>| int _test_run_group(const char *group_name, const struct test_unit *tests,
>>| size_t n_tests, void *test_state);
>>
>>2) `skip()` `skip_all()` and `todo()` helpers now return values to be
>>return to runner.
>>i.e. change usage from
>>| if (cond)
>>| skip("NIY");
>>to
>>| if (cond)
>>| return skip("NIY");
>>
>>`bail_out()` helper still just exits with error code, which corresponding
>>its standard specification.
>>
>>But now some parts of the code start to look "alya cringe":
>>| return todo("Need to replace backtrace with libunwind first");
>>| lua_State *L = test_state;
>>| utils_get_aux_lfunc(L);
>>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_OFF);
>>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_FLUSH);
>>| check_profile_func(L);
>>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_ON);
>>| return TEST_EXIT_SUCCESS;
>Well, what is cringe here? There are a few unreachable lines, but now it is
>obvious that those are skipped.
>>
>>(Yes, we want to use unconditional `todo()`).
>>So I commented the similar code, helper `check_profile_func()`, etc.
>>with `#if 0`.
>Do we really need to that though? Again, it is clearly visible that those are
>unreachable. Comment in `todo` is sufficient.
>
><snipped>
>--
>Best regards,
>Maxim Kokryashkin
>
[-- Attachment #2: Type: text/html, Size: 7056 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests
2023-05-19 14:29 ` [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking C tests Maxim Kokryashkin via Tarantool-patches
@ 2023-05-20 8:38 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 29+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2023-05-20 8:38 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Maxim!
Thanks for the reply!
On 19.05.23, Maxim Kokryashkin wrote:
>
> Hi!
> Thanks for the comments.
>
> >
> >>The whole idea of the patch-set introduce module for LuaJIT C tests. It
> >>also, can be used for unit tests.
> >>* The first patch is the prerequisite for the patch-set. It fixes
> >> LD_LIBRARY_PATH definition.
> >>* The 2nd and 3d patches provides an API and helper for writing the tests.
> >>* The last 3 patches rewrite existing tests that should be written in C in
> >> the proper way.
> >>
> >>Branch: https://github.com/tarantool/luajit/tree/skaplun/gh-noticket-tarantool-c-tests
> >>PR: https://github.com/tarantool/tarantool/pull/8444
> >>Related Issue:
> >>* https://github.com/tarantool/tarantool/issues/7900
> >>* https://github.com/tarantool/tarantool/issues/781
> >>
> >>Thanks Maxim, for the review!
> >>
> >>I've fixed some Maxim comments and suggestions for the previous series.
> >>Some ignorable comments about wording are ignored:).
> >>
> >>Also, see answers for your questions below:
> >>
> >>> >+if(NOT PROVE)
> >>> >+ message(WARNING "`prove' is not found, so tarantool-c-tests target is not generated")
> >>> >+ return()
> >>> >+endif()
> >>> There is the same check in the test/tarantool-tests/CMakeLists.txt. Maybe
> >>> we should move it to the higher-level CMake so the lower-level CMakeLists
> >>> inherit it.
> >>
> >>I agree it maybe done, but not within this particular patchsett, so
> >>ignoring for now.
> >Don’t see any reason to postpone it, tbh. New module requires the same
> >checks as the already present one. It seems logical to do necessary changes
> >in this patchset.
There are two testing suite (except the new one), that use this
approach:
* tarantool-tests
* lua-Harness-tests
So, for this patchset I just use the same beaten path.
Also, we may try to build tests-target (suite) independently, and in
this case the check should be in low-level CMake.
> >>
<snipped>
> >>
> >>> >+#include "lj_arch.h"
> >>> Side note: I don't like the approach with private headers, but
> >>> I couldn't find any better way to check that. Maybe it is a good
> >>> idea to implement a public C API function to get the information
> >>> about OS and ARCH, since it is a really common to check them?
> >>
> >>I think, that this is the best option, espessialy if we want to write
> >>some unit test for some specific module (I mean <src/lj_*>).
> >>
> >>Changes in v2:
> >>1) use
> >>
> >>| int _test_run_group(const char *group_name, const struct test_unit tests[],
> >>| size_t n_tests, void *test_state);
> >>
> >>instead of
> >>
> >>| int _test_run_group(const char *group_name, const struct test_unit *tests,
> >>| size_t n_tests, void *test_state);
> >>
> >>2) `skip()` `skip_all()` and `todo()` helpers now return values to be
> >>return to runner.
> >>i.e. change usage from
> >>| if (cond)
> >>| skip("NIY");
> >>to
> >>| if (cond)
> >>| return skip("NIY");
> >>
> >>`bail_out()` helper still just exits with error code, which corresponding
> >>its standard specification.
> >>
> >>But now some parts of the code start to look "alya cringe":
> >>| return todo("Need to replace backtrace with libunwind first");
> >>| lua_State *L = test_state;
> >>| utils_get_aux_lfunc(L);
> >>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_OFF);
> >>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_FLUSH);
> >>| check_profile_func(L);
> >>| (void)luaJIT_setmode(L, 0, LUAJIT_MODE_ENGINE | LUAJIT_MODE_ON);
> >>| return TEST_EXIT_SUCCESS;
> >Well, what is cringe here? There are a few unreachable lines, but now it is
> >obvious that those are skipped.
Yes, unreachable lines looks very strange to me.
> >>
> >>(Yes, we want to use unconditional `todo()`).
> >>So I commented the similar code, helper `check_profile_func()`, etc.
> >>with `#if 0`.
> >Do we really need to that though? Again, it is clearly visible that those are
> >unreachable. Comment in `todo` is sufficient.
I'm not sure about that, so I'll wait for the 2nd reviewer opinion.
> >
> ><snipped>
> >--
> >Best regards,
> >Maxim Kokryashkin
> >
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 29+ messages in thread