From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6E93D42EF5D for ; Thu, 11 Jun 2020 21:42:06 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Thu, 11 Jun 2020 20:42:05 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 4/3] test: add additional checks List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org, tarantool-patches@dev.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).