Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 0/3] Make unit tests TAP-compliant
Date: Thu, 11 Jun 2020 20:42:07 +0200	[thread overview]
Message-ID: <37228968-21b2-46a7-1c2a-db1a39f42f5e@tarantool.org> (raw)
In-Reply-To: <cover.1591631501.git.sergeyb@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.

      parent reply	other threads:[~2020-06-11 18:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-08 15:58 sergeyb
2020-06-08 15:58 ` [Tarantool-patches] [PATCH v2 1/3] test: update unit test lib to produce TAP-compliant output sergeyb
2020-06-08 23:55   ` Vladislav Shpilevoy
2020-06-09 18:25     ` Sergey Bronnikov
2020-06-08 15:58 ` [Tarantool-patches] [PATCH v2 2/3] test: update unit tests to make them TAP-compliant sergeyb
2020-06-08 23:55   ` Vladislav Shpilevoy
2020-06-09 16:04     ` Sergey Bronnikov
2020-06-11 18:42   ` Vladislav Shpilevoy
2020-06-08 15:58 ` [Tarantool-patches] [PATCH v2 3/3] msgpuck: bump version to make test output TAP-compliant sergeyb
2020-06-08 16:03 ` [Tarantool-patches] [PATCH v2 4/3][msgpuck] test: " Sergey Bronnikov
2020-06-11 18:42 ` [Tarantool-patches] [PATCH v2 4/3] test: add additional checks Vladislav Shpilevoy
2020-06-11 18:42 ` Vladislav Shpilevoy [this message]

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=37228968-21b2-46a7-1c2a-db1a39f42f5e@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 0/3] Make unit tests TAP-compliant' \
    /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