Re: [Tarantool-patches] [PATCH v2 4/3] test: add additional checks

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! You didn't send it, so I did. I pasted it
below.

See 6 comments below.

>     
>     With diff-based approach test oracles were in .result files.
>     After removing .result files test should be self-contained and
>     shoud include strict checks to detect wrong behaviour during test
>     execution.
>     
>     This commit brings additional test checks and allow diag() to fail a test.
>     
>     Part of #5000
>> diff --git a/test/unit/bloom.cc b/test/unit/bloom.cc
> index 7bd6814cc..7f91d97f7 100644
> --- a/test/unit/bloom.cc
> +++ b/test/unit/bloom.cc
> @@ -46,8 +46,8 @@ simple_test()
>  		if (fp_rate > p + 0.001)
>  			fp_rate_too_big++;
>  	}
> -	note("error_count = %u", error_count);
> -	note("fp_rate_too_big = %u", fp_rate_too_big);
> +	is(error_count, 0, "error_count = %u", error_count);
> +	is(fp_rate_too_big, 0, "fp_rate_too_big = %u", fp_rate_too_big);
>  	footer();
>  }
>  
> @@ -94,15 +94,15 @@ store_load_test()
>  		if (fp_rate > p + 0.001)
>  			fp_rate_too_big++;
>  	}
> -	note("error_count = %d", error_count);
> -	note("fp_rate_too_big = %d", fp_rate_too_big);
> +	is(error_count, 0, "error_count = %u", error_count);
> +	is(fp_rate_too_big, 0, "fp_rate_too_big = %u", fp_rate_too_big);
>  	footer();
>  }
>  
>  int
>  main(void)
>  {
> -	plan(0);
> +	plan(4);

1. Normally, if a test case consists of multiple checks, it has
its own header()/footer() + plan()/check_plan(). So here the
plan should be 2. Simple_test() and store_load_test() should
have their own header()/plan()/check_plan()/footer().

>  	simple_test();
>  	store_load_test();
>  	check_plan();
> diff --git a/test/unit/cbus_hang.c b/test/unit/cbus_hang.c
> index eac283890..458d79de6 100644
> --- a/test/unit/cbus_hang.c
> +++ b/test/unit/cbus_hang.c
> @@ -148,8 +148,8 @@ main_f(va_list ap)
>  	cbus_stop_loop(&pipe_from_main_to_hang);
>  	cpipe_destroy(&pipe_from_main_to_hang);
>  
> -	cord_join(&hang_worker);
> -	ok(true, "The hang worker has been joined");
> +	int rc = cord_join(&hang_worker);
> +	ok(!rc, "The hang worker has been joined");

2. Please, use 'is(rc, 0, ...)'. We don't use integers
as booleans. This is a part of the code style.

>  	alarm(0);
>  
> diff --git a/test/unit/csv.c b/test/unit/csv.c
> index b79bf8f39..32c927327 100644
> --- a/test/unit/csv.c
> +++ b/test/unit/csv.c

3. This test is marked as 'pass' in test-run, but when I run it
manually, this is what I get:

	Test failed: csv_get_error_status(&csv) == CSV_ER_INVALID is true at tarantool/test/unit/csv.c:154, in function 'test6'

> diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc
> index 8568346e8..365a7c17b 100644
> --- a/test/unit/fiber.cc
> +++ b/test/unit/fiber.cc
> @@ -75,19 +75,26 @@ fiber_join_test()
>  {
>  	header();
>  
> -	struct fiber *fiber = fiber_new_xc("join", noop_f);
> +	int rc;
> +	struct fiber *fiber = NULL;
> +	fiber = fiber_new_xc("join", noop_f);
> +	fail_if(!fiber);

4. Please, don't use '!' with non-boolean values. If you want
to check a pointer is not NULL, use != NULL. If you want to
check an integer is not 0, use != 0. I suggest you to find and
fix all the other similar places by yourself.

>  	fiber_set_joinable(fiber, true);
>  	fiber_wakeup(fiber);
> -	fiber_join(fiber);
> +	rc = fiber_join(fiber);
> +	fail_if(rc);
>  
> @@ -186,7 +203,7 @@ fiber_name_test()
>  static int
>  main_f(va_list ap)
>  {
> -	plan(0);
> +	plan(2);

5. The same as the comment about bloom.cc.

>  	fiber_name_test();
>  	fiber_join_test();
>  	fiber_stack_test();
> @@ -200,7 +217,9 @@ int main()
>  	memory_init();
>  	fiber_init(fiber_cxx_invoke);
>  	fiber_attr_create(&default_attr);
> -	struct fiber *main = fiber_new_xc("main", main_f);
> +	struct fiber *main = NULL;
> +	main = fiber_new_xc("main", main_f);
> +	fail_if(!main);
>  	fiber_wakeup(main);
>  	ev_run(loop(), 0);
>  	fiber_free();
> diff --git a/test/unit/scramble.c b/test/unit/scramble.c
> index 65270bb4c..296a08715 100644
> --- a/test/unit/scramble.c
> +++ b/test/unit/scramble.c

6. I added abort() in the middle of the test. I crashes, and test-run
happily reports 'pass'. So if any of our unit tests will now crash,
we won't notice that (at least when plan is 0).