From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 C4196469710 for ; Tue, 9 Jun 2020 02:55:57 +0300 (MSK) References: <7ae0f96d0c88fe7bace0b7995b6cd9c606ed92bd.1591631501.git.sergeyb@tarantool.org> From: Vladislav Shpilevoy Message-ID: <3ca7c655-a6d8-5c41-5eb6-a222ca9cefab@tarantool.org> Date: Tue, 9 Jun 2020 01:55:55 +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: 7bit 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 Thanks for the patch! What about small/ library? It also contains tests, and they are not TAP complaint too. For example, when I remove all .result files from unit/ folder, and run the tests, I get rlist.result created. See 15 comments below. > diff --git a/test/unit/bit.c b/test/unit/bit.c > index 1dae3671c..0da655f61 100644 > --- a/test/unit/bit.c > +++ b/test/unit/bit.c > @@ -29,18 +28,18 @@ test_ctz_clz(void) > uint64_t val64 = vals[i]; > uint32_t val32 = (uint32_t) vals[i]; > > - printf("bit_ctz_u64(%" PRIu64 ") => %d\n", val64, > - bit_ctz_u64(val64)); > - printf("bit_clz_u64(%" PRIu64 ") => %d\n", val64, > - bit_clz_u64(val64)); > + note("bit_ctz_u64(%" PRIu64 ") => %d", val64, > + bit_ctz_u64(val64)); 1. Alignment is still not ideal - it should be by the opening parenthesis. Not by the latter + 1. > + note("bit_clz_u64(%" PRIu64 ") => %d", val64, > + bit_clz_u64(val64)); > > if (vals[i] > UINT32_MAX) > continue; > > - printf("bit_ctz_u32(%" PRIu32 ") => %d\n", val32, > - bit_ctz_u32(val32)); > - printf("bit_clz_u32(%" PRIu32 ") => %d\n", val32, > - bit_clz_u32(val32)); > + note("bit_ctz_u32(%" PRIu32 ") => %d", val32, > + bit_ctz_u32(val32)); > + note("bit_clz_u32(%" PRIu32 ") => %d", val32, > + bit_clz_u32(val32)); > } > > footer(); > @@ -144,14 +143,14 @@ test_index(void) > uint64_t val64 = vals[i]; > uint32_t val32 = (uint32_t) vals[i]; > > - printf("bit_index_u64(%" PRIu64 ", *, -1) => ", val64); > + printf("# bit_index_u64(%" PRIu64 ", *, -1) => ", val64); 2. Why did you use note() in some places, but kept printf() + added manual '#' in other places? > test_index_print(indexes, bit_index_u64(val64, indexes, -1)); > printf("\n"); > > if (vals[i] > UINT32_MAX) > continue; > > - printf("bit_index_u32(%" PRIu32 ", *, -1) => ", val32); > + printf("# bit_index_u32(%" PRIu32 ", *, -1) => ", val32); > test_index_print(indexes, bit_index_u32(val32, indexes, -1)); > printf("\n"); > } > diff --git a/test/unit/bitset_index.c b/test/unit/bitset_index.c > index c077fae49..8f9363e71 100644 > --- a/test/unit/bitset_index.c > +++ b/test/unit/bitset_index.c > @@ -291,6 +287,7 @@ void test_equals_simple(void) > int main(void) > { > setbuf(stdout, NULL); > + plan(0); 3. Check_plan() is missing. > > test_size_and_count(); > test_resize(); > diff --git a/test/unit/bloom.cc b/test/unit/bloom.cc > index 2e5b99412..ebd5d18d8 100644 > --- a/test/unit/bloom.cc > +++ b/test/unit/bloom.cc > @@ -13,7 +14,7 @@ uint32_t h(uint32_t i) > void > simple_test() > { > - cout << "*** " << __func__ << " ***" << endl; > + note(__func__); 4. Why did you trim '***'? Besides, it looks exactly like header() (not counting padding), so you probably could use it here. Check the result output if it does not become too ugly with the padding. > srand(time(0)); > uint32_t error_count = 0; > uint32_t fp_rate_too_big = 0; > @@ -45,14 +46,13 @@ simple_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); 5. error_count and fp_rate_too_big are uint32_t. So for this type you usually need %u. > } > > void > store_load_test() > { > - cout << "*** " << __func__ << " ***" << endl; 6. Why did you remove this print, but kept the previous one, looking exactly the same? > srand(time(0)); > uint32_t error_count = 0; > uint32_t fp_rate_too_big = 0; > 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"); 7. Why did you keep it as printf()? Besides, I see more printf()s in this file, which don't have '#' in the beginning. How don't they screw TAP format? > test_iterator runner = begin; > while (!test_iterator_are_equal(&tree, &runner, &end)) { > elem_t *elem = test_iterator_get_elem(&tree, &runner); > diff --git a/test/unit/coll.cpp b/test/unit/coll.cpp > index 8d7a3ff1e..d5e9724af 100644 > --- a/test/unit/coll.cpp > +++ b/test/unit/coll.cpp > @@ -30,12 +30,13 @@ void > test_sort_strings(vector &strings, struct coll *coll) > { > sort(strings.begin(), strings.end(), comp(coll)); > - cout << strings[0] << endl; > + cout << "# " << strings[0] << endl; 8. Why don't you use note()? > 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] > + cout << "# " > + << strings[i] > << (cmp < 0 ? " LESS" : cmp > 0 ? " GREATER " : " EQUAL") > << endl; > } > diff --git a/test/unit/guard.cc b/test/unit/guard.cc > index a2953b829..71b116f09 100644 > --- a/test/unit/guard.cc > +++ b/test/unit/guard.cc > @@ -53,6 +53,7 @@ main_f(va_list ap) > > int main() > { > + plan(0); 9. Why didn't you add check_plan()? > memory_init(); > fiber_init(fiber_cxx_invoke); > fiber_attr_create(&default_attr); > diff --git a/test/unit/int96.cc b/test/unit/int96.cc > index 29dbbcbf5..09424d144 100644 > --- a/test/unit/int96.cc > +++ b/test/unit/int96.cc > @@ -15,50 +13,50 @@ test() > int96_set_unsigned(&num1, a); > int96_set_unsigned(&num2, a); > int96_invert(&num2); > - check(int96_is_neg_int64(&num2)); > - check(int96_extract_neg_int64(&num2) == int64_t(-a)); > - check(int96_is_uint64(&num)); > - check(int96_extract_uint64(&num) == 0); > + is(int96_is_neg_int64(&num2), true, "int96_is_neg_int64()"); 10. is(..., true) is the same as ok(...). But probably a less intrusive option would be to change macro 'check' to use fail_if() or ok() instead of its custom printer. Then the patch for this would consist of 3 lines. > + is(int96_extract_neg_int64(&num2), int64_t(-a), "int96_extract_neg_int64()"); > + is(int96_is_uint64(&num), true, "int96_is_uint64()"); > + is(int96_extract_uint64(&num), 0, "int96_extract_uint64()"); > int96_add(&num, &num1); > - check(int96_is_uint64(&num)); > - check(int96_extract_uint64(&num) == a); > + is(int96_is_uint64(&num), true, "int96_is_uint64()"); > + is(int96_extract_uint64(&num), a, "int96_extract_uint64()"); > int96_add(&num, &num1); > - check(int96_is_uint64(&num)); > - check(int96_extract_uint64(&num) == a * 2); > + is(int96_is_uint64(&num), true, "int96_is_uint64()"); > + is(int96_extract_uint64(&num), a * 2, "int96_extract_uint64()"); > for (int i = 1; i < 1000; i++) { > for(int j = 0; j < i; j++) { > int96_add(&num, &num1); > - check(!int96_is_uint64(&num) && !int96_is_neg_int64(&num)); > + fail_if(!(!int96_is_uint64(&num) && !int96_is_neg_int64(&num))); > } > for(int j = 0; j < i - 1; j++) { > int96_add(&num, &num2); > - check(!int96_is_uint64(&num) && !int96_is_neg_int64(&num)); > + fail_if(!(!int96_is_uint64(&num) && !int96_is_neg_int64(&num))); > } > int96_add(&num, &num2); > - check(int96_is_uint64(&num)); > - check(int96_extract_uint64(&num) == a * 2); > + fail_if(!int96_is_uint64(&num)); > + fail_if(int96_extract_uint64(&num) != a * 2); > } > int96_add(&num, &num2); > - check(int96_is_uint64(&num)); > - check(int96_extract_uint64(&num) == a); > + is(int96_is_uint64(&num), true, "int96_is_uint64()"); > + is(int96_extract_uint64(&num), a, "int96_extract_uint64()"); > int96_add(&num, &num2); > - check(int96_is_uint64(&num)); > - check(int96_extract_uint64(&num) == 0); > + is(int96_is_uint64(&num), true, "int96_is_uint64()"); > + is(int96_extract_uint64(&num), 0, "int96_extract_uint64()"); > int96_add(&num, &num2); > - check(int96_is_neg_int64(&num)); > - check(int96_extract_neg_int64(&num) == int64_t(-a)); > + is(int96_is_neg_int64(&num), true, "int96_is_neg_int64()"); > + is(int96_extract_neg_int64(&num), int64_t(-a), "int96_extract_neg_int64()"); > for (int i = 1; i < 1000; i++) { > for(int j = 0; j < i; j++) { > int96_add(&num, &num2); > - check(!int96_is_uint64(&num) && !int96_is_neg_int64(&num)); > + fail_if(!(!int96_is_uint64(&num) && !int96_is_neg_int64(&num))); > } > for(int j = 0; j < i - 1; j++) { > int96_add(&num, &num1); > - check(!int96_is_uint64(&num) && !int96_is_neg_int64(&num)); > + fail_if(!(!int96_is_uint64(&num) && !int96_is_neg_int64(&num))); > } > int96_add(&num, &num1); > - check(int96_is_neg_int64(&num)); > - check(int96_extract_neg_int64(&num) == int64_t(-a)); > + fail_if(!(int96_is_neg_int64(&num))); > + fail_if(int96_extract_neg_int64(&num) != int64_t(-a)); > } > > footer(); > @@ -67,5 +65,7 @@ test() > int > main(int, const char **) > { > + plan(14); > test(); > + check_plan(); 11. Seems like it is better to add plan and check_plan to the test() function. Because main() does not do any tests. > } > diff --git a/test/unit/rmean.cc b/test/unit/rmean.cc > index 50db96f6d..7dea28e46 100644 > --- a/test/unit/rmean.cc > +++ b/test/unit/rmean.cc > @@ -5,7 +5,7 @@ > > int print_stat(const char *name, int rps, int64_t total, void* ctx) > { > - printf("%s: rps %d, total %d%c", name, rps, (int)total, > + note("%s: rps %d, total %d%c", name, rps, (int)total, > name[2] == '2' ? '\n' : '\t'); 12. Bad alignment. Hint: previous alignemt was good. You need to do the same: put 'name[2]' right after '(' of the previous line. > return 0; > } > diff --git a/test/unit/rope.c b/test/unit/rope.c > index 5ab4c51f7..990e7cc9b 100644 > --- a/test/unit/rope.c > +++ b/test/unit/rope.c > @@ -4,7 +4,7 @@ > static void > test_rope_extract(struct rope *rope, rope_size_t pos) > { > - printf("extract pos = %zu: ", (size_t) pos); > + printf("# extract pos = %zu: ", (size_t) pos); 13. Why not note()? > struct rope_node *node = rope_extract_node(rope, pos); > rope_check(rope); > str_print(node->data, node->leaf_size); > diff --git a/test/unit/rtree_multidim.cc b/test/unit/rtree_multidim.cc > index e09f6190b..779cf3873 100644 > --- a/test/unit/rtree_multidim.cc > +++ b/test/unit/rtree_multidim.cc > @@ -348,14 +348,14 @@ test_select_neigh(const CBoxSet &set, const struct rtree *tree) > } > } > if (res1.size() != res2.size()) { > - printf("%s result size differ %d %d\n", __func__, > - (int)res1.size(), (int)res2.size()); > + diag("%s result size differ %d %d\n", __func__, > + (int)res1.size(), (int)res2.size()); > } else { > for (size_t i = 0; i < res1.size(); i++) > if (res1[i].id != res2[i].id && > res1[i].box.Distance2(box) != > res2[i].box.Distance2(box)) > - printf("%s result differ!\n", __func__); > + diag("%s result differ!\n", __func__); 14. When you print diag(), the test won't fail even if reaches this point. So we will see it 'pass' in test-run even though it does not pass. It should be fail_if(), or ok(), or is(), or something similar. Otherwise all is considered as success. Please validate all the other fail print places in this patch, that their fail actually fails the test. > } > rtree_iterator_destroy(&iterator); > > @@ -526,12 +526,16 @@ rand_test() > > rtree_destroy(&tree); > > + note("\tDIMENSION: %u, page size: %u, max fill good: %d\n", > + DIMENSION, tree.page_size, tree.page_max_fill >= 10); 15. Why did you add this? There wasn't a print here previously.