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

[Vladislav Shpilevoy]

Thanks for the patch!

There is still not deleted file unit/sql-bitvec.result.
Seems like its test file was removed some time ago, and
the result file was forgotten.

Also there is unit/csv.result. It has a test. Why didn't
you delete this file? Is the test output TAP compliant?

After I run the tests, unit/msgpack.result is generated
again. Test-run says

	[002] unit/msgpack.test                                               
	[002] TAP13 parse failed: Missing plan in the TAP source
	[002] [ new ]

See 8 comments below.

> diff --git a/test/unit/bit.c b/test/unit/bit.c
> index da514484d..d18aa5579 100644
> --- a/test/unit/bit.c
> +++ b/test/unit/bit.c
> @@ -29,20 +29,22 @@ test_ctz_clz(void)
>  		uint64_t val64 = vals[i];
>  		uint32_t val32 = (uint32_t) vals[i];
>  
> -		printf("bit_ctz_u64(%" PRIu64 ") => %d\n", val64,
> +		note("bit_ctz_u64(%" PRIu64 ") => %d", val64,
>  			bit_ctz_u64(val64));
> -		printf("bit_clz_u64(%" PRIu64 ") => %d\n", val64,
> +		note("bit_clz_u64(%" PRIu64 ") => %d", val64,
>  			bit_clz_u64(val64));

1. Previously the multiline expressions where aligned by the
parent expression printf(). Now they are not aligned. Please,
align. Multiline function call should be aligned by the column
after the opening parenthesis, not including it. Here and in all
other places, of this and other files.

>  
>  		if (vals[i] > UINT32_MAX)
>  			continue;
>  
> -		printf("bit_ctz_u32(%" PRIu32 ") => %d\n", val32,
> +		note("bit_ctz_u32(%" PRIu32 ") => %d", val32,
>  			bit_ctz_u32(val32));
> -		printf("bit_clz_u32(%" PRIu32 ") => %d\n", val32,
> +		note("bit_clz_u32(%" PRIu32 ") => %d", val32,
>  			bit_clz_u32(val32));
>  	}
>  
> +	ok(1, "test ctz clz");

2. Seems like plan(0) works perfectly fine, so you don't need
to add the dummy ok() calls. Moreover, you can delete plan()+check_plan(),
and it also works fine. Because header() and footer() are just comments.
They don't oblige you to have a plan. That should reduce diff size and
simplify the patch.

> +	check_plan();
>  	footer();
>  }
> @@ -107,36 +115,42 @@ static void
>  
>  static inline void
>  test_index_print(const int *start, const int *end)
>  {
> +	/*
>  	for (const int *cur = start; cur < end; cur++) {
>  		printf("%d ", *cur);
>  	}
> +	*/

3. WTF is that?

>  }
>  
>  static void
>  test_index(void)
>  {
>  	header();
> +	plan(1);
>  
>  	int indexes[sizeof(int64_t) * CHAR_BIT + 1];
>  
> @@ -144,18 +158,20 @@ 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);
>  		test_index_print(indexes, bit_index_u64(val64, indexes, -1));
> -		printf("\n");
> +		//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");
> +		//printf("\n");

4. ???
Seems like the patch is really raw. Please, get rid of the commented
code. It should either work, or should not exist.

>  	}
>  
> +	ok(1, "test index");
> +	check_plan();
>  	footer();
>  }
> @@ -202,7 +222,9 @@ test_bit_iter_empty(void)
>  
>  	bit_iterator_init(&it, NULL, 0, false);
>  	fail_unless(bit_iterator_next(&it) == SIZE_MAX);
> -
> +	

5. Trailing tab symbol. It is highlighted in bright red
color in the console, when you do 'git diff/show'.

> +	ok(1, "test bit iter empty");
> +	check_plan();
>  	footer();
>  }
> @@ -217,12 +240,15 @@ test_bitmap_size(void)
>  	fail_unless(bitmap_size(sizeof(long) * CHAR_BIT * 4) == sizeof(long) * 4);
>  	fail_unless(bitmap_size(sizeof(long) * CHAR_BIT * 4 - 1) == sizeof(long) * 4);
>  	fail_unless(bitmap_size(sizeof(long) * CHAR_BIT * 9 / 2) == sizeof(long) * 5);
> +	ok(1, "test bitmap size");
> +	check_plan();
>  	footer();
>  }
>  
>  int
>  main(void)
>  {
> +	plan(8);

6. I see that to some main() functions you added only plan() + check_plan().
To some you added header() + footer(). Lets be consistent and add all the
outputs: header(), plan(), check_plan(), and footer().

> diff --git a/test/unit/bloom.cc b/test/unit/bloom.cc
> index 2e5b99412..23a36487c 100644
> --- a/test/unit/bloom.cc
> +++ b/test/unit/bloom.cc
> @@ -45,14 +47,16 @@ 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);
> +	ok(1, "%s", __func__);
> +	check_plan();

7. There is a header(), but no footer().

>  }
>  
>  void
>  store_load_test()
>  {
> -	cout << "*** " << __func__ << " ***" << endl;
> +	plan(1);

8. Why did you add header() to the previous test, and to this
one you didn't? Find and fix all the other similar inconsistencies
in the patch.

I will review the rest, when the comments above are done and expanded
to the whole patch.