Re: [Tarantool-patches] [PATCH v2 0/3] Make unit tests TAP-compliant

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

On top of the branch I made some checks whether the tests didn't
become useless. And seems like they are. There is a few examples.

I made this:

====================
diff --git a/src/lib/bit/bit.h b/src/lib/bit/bit.h
index eb392b820..339462188 100644
--- a/src/lib/bit/bit.h
+++ b/src/lib/bit/bit.h
@@ -298,11 +298,11 @@ inline int
 bit_ctz_u64(uint64_t x)
 {
 #if   defined(HAVE_BUILTIN_CTZLL)
-	return __builtin_ctzll(x);
+	return __builtin_ctzll(x) + 1;
 #elif defined(HAVE_FFSLL)
-	return ffsll(x) - 1;
+	return ffsll(x);
 #else
-	CTZ_NAIVE(x, sizeof(uint64_t) * CHAR_BIT);
+	CTZ_NAIVE(x, sizeof(uint64_t) * CHAR_BIT) + 1;
 #endif
 }
 
====================

So I broke bit/ library. And bit.test passed. Test-run
reported 'pass'.

Then I tried this:

====================
diff --git a/test/unit/bitset_basic.c b/test/unit/bitset_basic.c
index 2befe6ff8..83f3c946b 100644
--- a/test/unit/bitset_basic.c
+++ b/test/unit/bitset_basic.c
@@ -16,7 +16,7 @@ void test_cardinality()
 	fail_unless(tt_bitset_cardinality(&bm) == 0);
 
 	size_t cnt = 0;
-	fail_if(tt_bitset_set(&bm, 10) < 0);
+	fail_if(tt_bitset_set(&bm, 10) < 0 || 1);
 	cnt++;
 	fail_if(tt_bitset_set(&bm, 15) < 0);
 	cnt++;

====================

The test passed in test-run, but didn't pass manually, of course.
This is what I got:

	Test failed: tt_bitset_set(&bm, 10) < 0 || 1 is true at tarantool/test/unit/bitset_basic.c:19, in function 'test_cardinality'

So clearly fail() and fail_if() don't affect the test correctness
from test-run's point of view.

Perhaps that could be workarounded by resurrecting the dummy
ok(true) in the end of each main() function, and add an panic to
plan() if it is 0. I don't know, didn't check. Also see one of
the cases below proving that it is not enough anyway.

Next I took bps_tree.cc and looked at it more carefully.
The test not only is still print-based, at least because of
printing_test(), but also it makes checks using test_debug_check().
Assuming, that it will crash in case of an error. But even if it
will crash, test-run will consider it 'pass'. Yeah, weird. But
the test became close to useless too.

Next I took cbus.c. I wanted to see what if a crash happens after
all the planned tests pass:

====================
--- a/test/unit/cbus.c
+++ b/test/unit/cbus.c
@@ -240,6 +240,7 @@ main()
 	note("finish main loop");
 	cbus_free();
 	fiber_free();
+	assert(false);
 	memory_free();

====================

The test 'passed' in test-run. So if our tests will crash somewhere
in the end, when they destroy allocated resources, or in an atexit()
callback, we won't see that crash. Unless run the test manually.

I tried to screw coll.cc, but it does not pass even as is. Crashes
in a fail_if(), where you used 'fail_if(coll != NULL)' instead of
'fail_if(coll == NULL)'. So you didn't check what this test outputs.

Based on these serious problems, and what I saw in the commits, I am
going to stop right here and give you more time on fixing the problems
of this patch. Please, don't hurry up with sending a new version. This
won't speed up the push process anyway. Likely on the contrary.

A first and foremost, the way you split it into commits is too hard
to review. Huge number of changes and files, and all of them I need to
rescan on each new patch version. So a first good step would be to find
which tests are already easy to convert, and do that for them. The tests,
having many changes, could be separated into different commits.

Also it seems test-run have problems, after all. If a crash is considered
fine. In private we discussed that it is ok, but now I clearly see it is
not. These problems also should be addressed, before fixing the tests.