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

[Vladislav Shpilevoy <v.shpilevoy@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<const char *> &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?