From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 794B442F4AD for ; Thu, 11 Jun 2020 21:42:02 +0300 (MSK) References: <7ae0f96d0c88fe7bace0b7995b6cd9c606ed92bd.1591631501.git.sergeyb@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 11 Jun 2020 20:42:00 +0200 MIME-Version: 1.0 In-Reply-To: <7ae0f96d0c88fe7bace0b7995b6cd9c606ed92bd.1591631501.git.sergeyb@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 2/3] test: update unit tests to make them TAP-compliant List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org Since msgpuck test is fixed in a separate commit now, you can't remove msgpuck.result file here. Please, do it in the last commit, along with msgpuck update. Below is the diff without .result file removals + my comments. You could speed up the process, if the fixes would be split into more commits. Even commit-per-test would be fine. Or commit with similar changes, like 'replace all printf() with note() and diag()', etc. Because we could start pushing earlier, and I could stop re-checking already reviewed code. See 11 comments below. Although I didn't review carefully enough. Because found some fundamental and repeating problems, including similar to what I have already commented earlier. Also I didn't look at the next commit when reviewer this one. After I reviewed the next one, I see that a few places are fixed, but perhaps it is worth merging them into one commit. Otherwise it is really hard to review, and the changes are not atomic anyway - you already removed .result files, but still the tests are not TAP until the next commit. > diff --git a/test/unit/bloom.cc b/test/unit/bloom.cc > index 2e5b99412..7bd6814cc 100644 > --- a/test/unit/bloom.cc > +++ b/test/unit/bloom.cc > @@ -92,13 +94,16 @@ store_load_test() > if (fp_rate > p + 0.001) > fp_rate_too_big++; > } > - cout << "error_count = " << error_count << endl; > - cout << "fp_rate_too_big = " << fp_rate_too_big << endl; > + note("error_count = %d", error_count); > + note("fp_rate_too_big = %d", fp_rate_too_big); 1. Please, use %u. > + footer(); > } > > int > main(void) > { > + plan(0); > simple_test(); > store_load_test(); > + check_plan(); > } > diff --git a/test/unit/bps_tree.cc b/test/unit/bps_tree.cc > index ef374deb1..ecfb317c2 100644 > --- a/test/unit/bps_tree.cc > +++ b/test/unit/bps_tree.cc > @@ -581,7 +581,7 @@ bps_tree_debug_self_check() > > int res = test_debug_check_internal_functions(false); > if (res) > - printf("self test returned error %d\n", res); > + note("self test returned error %d", res); 2. AFAIU, we decided to use diag() for error messages. Lets be consistent and use diag() here. Or fail_if, since this particular case fits fail_if signature fine. > > test_debug_check_internal_functions(true); > > @@ -639,11 +639,11 @@ printing_test() > > for (type_t i = 0; i < rounds; i++) { > type_t v = rounds + i; > - printf("Inserting " TYPE_F "\n", v); > + note("Inserting " TYPE_F, v); > test_insert(&tree, v, 0); > test_print(&tree, TYPE_F); 3. The whole test is output based - it used test_print() to ensure that the tree is correct. I don't really know how to turn that into a TAP test. Perhaps we just should keep it diff based, with a .result file. And see if there are more such unit tests, which are written in C, but are not really TAP. Another option - print the tree into a FILE* different from stdout, read it back, and compare with a const string, defined in the same function. > v = rounds - i - 1; > - printf("Inserting " TYPE_F "\n", v); > + note("Inserting " TYPE_F, v); > test_insert(&tree, v, 0); > test_print(&tree, TYPE_F); > } > diff --git a/test/unit/bps_tree_iterator.cc b/test/unit/bps_tree_iterator.cc > index 5c800bc69..b0b08d3c6 100644 > --- a/test/unit/bps_tree_iterator.cc > +++ b/test/unit/bps_tree_iterator.cc > @@ -181,9 +179,9 @@ iterator_check() > if (has_this_key1 != has_this_key2) > fail("Exact flag is broken", "true"); > printf("Key %ld, %s range [%s, %s): ", key, > - has_this_key1 ? "not empty" : "empty", > - test_iterator_is_invalid(&begin) ? "eof" : "ptr", > - test_iterator_is_invalid(&end) ? "eof" : "ptr"); > + has_this_key1 ? "not empty" : "empty", > + test_iterator_is_invalid(&begin) ? "eof" : "ptr", > + test_iterator_is_invalid(&end) ? "eof" : "ptr"); 4. Unnecessary diff. Look, you could save lots of time of both of us, if you could do more attentive self reviews before sending a patch. To filter out such obvious problems before they reach the mailing list. > test_iterator runner = begin; > while (!test_iterator_are_equal(&tree, &runner, &end)) { > diff --git a/test/unit/coll.cpp b/test/unit/coll.cpp > index 8d7a3ff1e..1b2afb5d3 100644 > --- a/test/unit/coll.cpp > +++ b/test/unit/coll.cpp > @@ -30,14 +30,13 @@ void > test_sort_strings(vector &strings, struct coll *coll) > { > sort(strings.begin(), strings.end(), comp(coll)); > - cout << strings[0] << endl; > + note("%s", strings[0]); > for (size_t i = 1; i < strings.size(); i++) { > int cmp = coll->cmp(strings[i], strlen(strings[i]), > strings[i - 1], strlen(strings[i - 1]), > coll); > - cout << strings[i] > - << (cmp < 0 ? " LESS" : cmp > 0 ? " GREATER " : " EQUAL") > - << endl; > + note("# %s %s", strings[i], > + (cmp < 0 ? " LESS" : cmp > 0 ? " GREATER " : " EQUAL")); 5. This output was printed assuming, that if it changes, the test will fail. Now the change of output would happily mark the test as passed, even though it didn't. For example, if I will multiple comp result on -1. > } > }; > > @@ -54,62 +54,63 @@ manual_test() > def.icu.strength = COLL_ICU_STRENGTH_IDENTICAL; > struct coll *coll; > > - cout << " -- default ru_RU -- " << endl; > + note("-- default ru_RU --"); > coll = coll_new(&def); > - assert(coll != NULL); > + fail_if(coll != NULL); 6. This crashes in 100% cases. Because coll is always not NULL, and this is correct. > strings = {"Б", "бб", "е", "ЕЕЕЕ", "ё", "Ё", "и", "И", "123", "45" }; > test_sort_strings(strings, coll); > coll_unref(coll); > > diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc > index b0dfc9b14..8568346e8 100644 > --- a/test/unit/fiber.cc > +++ b/test/unit/fiber.cc > @@ -177,17 +178,20 @@ fiber_name_test() > > fiber_set_name(fiber(), long_name); > > - note("fiber name is truncated: %s.\n", fiber_name(fiber())); > + note("fiber name is truncated: %s", fiber_name(fiber())); 7. These notes were a part of the test. Now if the truncation wouldn't work, the test still will pass, even though it shouldn't. > + > footer(); > } > diff --git a/test/unit/queue.c b/test/unit/queue.c > index 3caec1a41..e53856b28 100644 > --- a/test/unit/queue.c > +++ b/test/unit/queue.c > @@ -59,11 +58,11 @@ void test0() > { > header(); > struct elem_queue queue = STAILQ_HEAD_INITIALIZER(queue); > - printf("Initialized: %s\n", queue2str(&queue)); > + note("Initialized: %s", queue2str(&queue)); > STAILQ_INIT(&queue); > - printf("STAILQ_INIT: %s\n", queue2str(&queue)); > + note("STAILQ_INIT: %s", queue2str(&queue)); > STAILQ_REVERSE(&queue, elem, entry); > - printf("STAILQ_REVERSE: %s\n", queue2str(&queue)); > + note("STAILQ_REVERSE: %s", queue2str(&queue)); > footer(); > } > > @@ -75,9 +74,9 @@ void test1() > struct elem_queue queue = STAILQ_HEAD_INITIALIZER(queue); > el1.val = 1; > STAILQ_INSERT_TAIL(&queue, &el1, entry); > - printf("STAILQ_INIT: %s\n", queue2str(&queue)); > + note("STAILQ_INIT: %s", queue2str(&queue)); > STAILQ_REVERSE(&queue, elem, entry); > - printf("STAILQ_REVERSE: %s\n", queue2str(&queue)); > + note("STAILQ_REVERSE: %s", queue2str(&queue)); > footer(); > } > > @@ -91,9 +90,9 @@ void test2() > el2.val = 2; > STAILQ_INSERT_TAIL(&queue, &el1, entry); > STAILQ_INSERT_TAIL(&queue, &el2, entry); > - printf("STAILQ_INIT: %s\n", queue2str(&queue)); > + note("STAILQ_INIT: %s", queue2str(&queue)); > STAILQ_REVERSE(&queue, elem, entry); > - printf("STAILQ_REVERSE: %s\n", queue2str(&queue)); > + note("STAILQ_REVERSE: %s", queue2str(&queue)); > footer(); > } > > @@ -108,9 +107,9 @@ void test3() > STAILQ_INSERT_TAIL(&queue, &el1, entry); > STAILQ_INSERT_TAIL(&queue, &el2, entry); > STAILQ_INSERT_TAIL(&queue, &el3, entry); > - printf("STAILQ_INIT: %s\n", queue2str(&queue)); > + note("STAILQ_INIT: %s", queue2str(&queue)); > STAILQ_REVERSE(&queue, elem, entry); > - printf("STAILQ_REVERSE: %s\n", queue2str(&queue)); > + note("STAILQ_REVERSE: %s", queue2str(&queue)); > footer(); > } > > @@ -122,44 +121,46 @@ void test_splice() > struct elem_queue queue1 = STAILQ_HEAD_INITIALIZER(queue1); > struct elem_queue queue2 = STAILQ_HEAD_INITIALIZER(queue2); > STAILQ_SPLICE(&queue1, STAILQ_FIRST(&queue1), entry, &queue2); > - printf("q1: %s\n", queue2str(&queue1)); > - printf("q2: %s\n", queue2str(&queue2)); > + note("q1: %s", queue2str(&queue1)); > + note("q2: %s", queue2str(&queue2)); > STAILQ_SPLICE(&queue2, STAILQ_FIRST(&queue2), entry, &queue1); > - printf("q1: %s\n", queue2str(&queue1)); > - printf("q2: %s\n", queue2str(&queue2)); > + note("q1: %s", queue2str(&queue1)); > + note("q2: %s", queue2str(&queue2)); > el1.val = 1; > el2.val = 2; > el3.val = 3; > STAILQ_INSERT_TAIL(&queue1, &el1, entry); > STAILQ_INSERT_TAIL(&queue1, &el2, entry); > STAILQ_INSERT_TAIL(&queue1, &el3, entry); > - printf("STAILQ_INIT: %s\n", queue2str(&queue1)); > + note("STAILQ_INIT: %s", queue2str(&queue1)); > STAILQ_SPLICE(&queue1, STAILQ_FIRST(&queue1), entry, &queue2); > - printf("q1: %s\n", queue2str(&queue1)); > - printf("q2: %s\n", queue2str(&queue2)); > + note("q1: %s", queue2str(&queue1)); > + note("q2: %s", queue2str(&queue2)); > STAILQ_SPLICE(&queue2, STAILQ_FIRST(&queue2), entry, &queue1); > - printf("q1: %s\n", queue2str(&queue1)); > - printf("q2: %s\n", queue2str(&queue2)); > + note("q1: %s", queue2str(&queue1)); > + note("q2: %s", queue2str(&queue2)); > STAILQ_SPLICE(&queue1, STAILQ_NEXT(STAILQ_FIRST(&queue1), entry), > entry, &queue2); > - printf("q1: %s\n", queue2str(&queue1)); > - printf("q2: %s\n", queue2str(&queue2)); > + note("q1: %s", queue2str(&queue1)); > + note("q2: %s", queue2str(&queue2)); > STAILQ_SPLICE(&queue2, STAILQ_NEXT(STAILQ_FIRST(&queue2), entry), > entry, &queue1); > - printf("q1: %s\n", queue2str(&queue1)); > - printf("q2: %s\n", queue2str(&queue2)); > + note("q1: %s", queue2str(&queue1)); > + note("q2: %s", queue2str(&queue2)); > STAILQ_SPLICE(&queue2, STAILQ_FIRST(&queue2), entry, &queue1); > - printf("q1: %s\n", queue2str(&queue1)); > - printf("q2: %s\n", queue2str(&queue2)); > + note("q1: %s", queue2str(&queue1)); > + note("q2: %s", queue2str(&queue2)); 8. Ditto. All these printfs were a part of the test. > footer(); > } > > int main() > { > + plan(0); > test0(); > test1(); > test2(); > test3(); > test_splice(); > + check_plan(); > return 0; > } > diff --git a/test/unit/rmean.cc b/test/unit/rmean.cc > index 50db96f6d..31f1f6c92 100644 > --- a/test/unit/rmean.cc > +++ b/test/unit/rmean.cc > @@ -5,16 +5,16 @@ > > int print_stat(const char *name, int rps, int64_t total, void* ctx) > { > - printf("%s: rps %d, total %d%c", name, rps, (int)total, > - name[2] == '2' ? '\n' : '\t'); > + note("%s: rps %d, total %d%c", name, rps, (int)total, > + name[2] == '2' ? '\n' : '\t'); 9. Ditto. > return 0; > } > > diff --git a/test/unit/scramble.c b/test/unit/scramble.c > index 8f1ee55af..65270bb4c 100644 > --- a/test/unit/scramble.c > +++ b/test/unit/scramble.c > @@ -40,16 +40,16 @@ test_scramble() > > scramble_reencode(new_scramble, scramble, salt, remote_salt, hash2); > > - printf("%d\n", scramble_check(new_scramble, remote_salt, hash2)); > + note("%d", scramble_check(new_scramble, remote_salt, hash2)); > > password = "wrongpass"; > scramble_prepare(scramble, salt, password, strlen(password)); > > - printf("%d\n", scramble_check(scramble, salt, hash2) != 0); > + note("%d", scramble_check(scramble, salt, hash2) != 0); > > scramble_prepare(scramble, salt, password, 0); > > - printf("%d\n", scramble_check(scramble, salt, hash2) != 0); > + note("%d", scramble_check(scramble, salt, hash2) != 0); 10. Ditto. > } > > void > diff --git a/test/unit/vclock.cc b/test/unit/vclock.cc > index 15e9f9379..2af367da0 100644 > --- a/test/unit/vclock.cc > +++ b/test/unit/vclock.cc > @@ -412,6 +412,5 @@ main(void) > test_tostring(); > test_fromstring(); > test_fromstring_invalid(); > - > - return check_plan(); > + check_plan(); 11. The main function returns int in this file. Why did you remove the return?