Re: [Tarantool-patches] [PATCH v2 luajit 2/6] test: introduce module for C tests

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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