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

[Sergey Bronnikov]

Hi, Vladislav

thanks for review!

On 00:52 Thu 04 Jun , Vladislav Shpilevoy wrote:
> 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.

Removed in a branch and updated version of patches.

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

Actually the problem with this test that it uses complex output and I
thought it is better to keep it as is. Then I look on it again and fixed
TAP output, changes were small.

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

It was my bad. This test uses header from msgpuck library which required
small fix. But I forgot to mention it or at least update submodule with
msgpuck in a patchset. Fixed in a new version.

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

Missed it, new version contains properly aligned lines.

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

My main idea was to report function names and testcases in a TAP output.
However there is no other sense in these ok()'s. We discussed it again
with you separately and I have removed all dummy ok() calls.

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

Ah, missed it.

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

Removed extra tab.

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

In a new version I kept only plan() and check_plan().

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

-- 
sergeyb@