[Tarantool-patches] [PATCH v2 2/3] test: update unit tests to make them TAP-compliant
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Jun 9 02:55:55 MSK 2020
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<const char *> &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<DIMENSION> &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.
More information about the Tarantool-patches
mailing list