Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests
Date: Wed, 24 May 2023 09:41:33 +0300	[thread overview]
Message-ID: <ZG2xnVkUKy1vW4LJ@root> (raw)
In-Reply-To: <467c6552-2cc8-7097-6d86-bb8c9628bb84@tarantool.org>

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

  reply	other threads:[~2023-05-24  6:45 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-18 20:44 [Tarantool-patches] [PATCH v2 luajit 0/6] Revorking " 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-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
2023-05-29 14:37       ` 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
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 [this message]
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
2023-05-31 13:32             ` Sergey Kaplun 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
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
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
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
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
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZG2xnVkUKy1vW4LJ@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox