From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 60B7E469710 for ; Mon, 8 Jun 2020 18:51:00 +0300 (MSK) Date: Mon, 8 Jun 2020 18:49:51 +0300 From: Sergey Bronnikov Message-ID: <20200608154951.GD53062@pony.bronevichok.ru> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Vladislav Shpilevoy Cc: o.piskunov@tarantool.org, tarantool-patches@dev.tarantool.org, alexander.turenko@tarantool.org 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@