Re: [Tarantool-patches] [PATCH v2 2/3] test: update unit tests to make them TAP-compliant

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hi, Vlad

thanks for review. I have answered to your comments and updated patches
in a branch.

On 01:55 Tue 09 Jun , Vladislav Shpilevoy wrote:
> 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.

small is a separate repository and would be better to fix tests in scope
of separate issue - https://github.com/tarantool/small/issues/26

> 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.

Fixed in a branch.

> > +		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?

note() adds a comment with newline (\n) to a TAP output.  Some tests
contains a complex output produced by a set of printf's with and without
newline inside loops and conditions. In such case it is better to keep
code as is without replacing printf()'s by note()'s.

One more note: according to TAP specification all lines must start with
"ok", "not ok" or "#". But our test executor `test-run.py` simply
ignores lines without "#".

> >  		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.

Added.

> >  
> >  	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.

Because there is no special meaning in this "****". Well, replaced it
with header().

> >  	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.

Agree, missed it because focused on formatting output only.
Fixed in a branch.

> >  }
> >  
> >  void
> >  store_load_test()
> >  {
> > -	cout << "*** " << __func__ << " ***" << endl;
> 
> 6. Why did you remove this print, but kept the previous one,
> looking exactly the same?

Replaced with header().

> >  	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?

Actually it is not TAP-compliant. But test-run.py doesn't complain about
such lines. See explanation above.

> >  		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()?

Replaced it with 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()?

Missed it, added 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.

Updated macro and reverted previus changes with is()'s.

> > +	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.

Agree, moved plan()/check_plan() to a function

> >  }
> > 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.

Fixed.

> >  	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()?

replaced with 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.

You are right, diag() is the same as note(), it doesn't change test
status. For unknown reasons author of this test decided to print about
it and don't fail testcase itself.

> 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.

We discussed it privately and decided:

- add unconditional fail() to diag() to make it more strict
- review tests for such cases where we warn, but don't update test
status and add diag()/fail() or similar TAP-checks there
- review tests and add extra checks or make them more strict
in a separate commit

Perhaps we should add additional checks to queue.c and stailq.c tests.
But implementations of singly-linked lists and tail queue covered by
these tests were imported from FreeBSD without changes and probably we
can keep tests as is.

> >  	}
> >  	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.

removed

-- 
sergeyb@