From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 4B7F64696C0 for ; Thu, 4 Jun 2020 01:52:11 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Thu, 4 Jun 2020 00:52:09 +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 2/2] test: update unit tests to make them TAP-compliant List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org Cc: o.piskunov@tarantool.org, alexander.turenko@tarantool.org 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.