# [GSoC][PATCH] t: port reftable/basics_test.c to the unit testing 9 messages from 2024-05-28 to 2024-05-29. Participants: Chandra Pratap, Christian Couder, Patrick Steinhardt. Thread: https://gitlist.dev/t/61555 ## Chandra Pratap, 2024-05-28 11:30 Subject: [GSoC][PATCH] t: port reftable/basics_test.c to the unit testing Message-ID: <20240528113856.8348-1-chandrapratap3519@gmail.com> URL: https://gitlist.dev/e/20240528113856.8348-1-chandrapratap3519%40gmail.com ``` In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit testing framework written entirely in C was introduced to the Git project aimed at simplifying testing and reducing test run times. Currently, tests for the raftable refs-backend are performed by a custom testing framework defined by reftable/test_framework.{c, h}. Port reftable/basics_test.c to the unit testing framework and improve upon the ported test. Mentored-by: Patrick Steinhardt Mentored-by: Christian Couder Signed-off-by: Chandra Pratap ``` ## Chandra Pratap, 2024-05-28 11:30 Subject: [PATCH 1/2] t: move reftable/basics_test.c to the unit testing framework Message-ID: <20240528113856.8348-2-chandrapratap3519@gmail.com> URL: https://gitlist.dev/e/20240528113856.8348-2-chandrapratap3519%40gmail.com In-Reply-To: <20240528113856.8348-1-chandrapratap3519@gmail.com> ``` reftable/basics_test.c exercise the functions defined in reftable/basics.{c, h}. Migrate reftable/basics_test.c to the unit testing framework. Migration involves refactoring the tests to use the unit testing framework instead of reftable's test framework. Mentored-by: Patrick Steinhardt Mentored-by: Christian Couder Signed-off-by: Chandra Pratap --- Makefile | 2 +- t/helper/test-reftable.c | 1 - .../unit-tests/t-reftable-basics.c | 50 ++++++++----------- 3 files changed, 21 insertions(+), 32 deletions(-) rename reftable/basics_test.c => t/unit-tests/t-reftable-basics.c (60%) diff --git a/Makefile b/Makefile index 8f4432ae57..36188ca256 100644 --- a/Makefile +++ b/Makefile @@ -1337,6 +1337,7 @@ THIRD_PARTY_SOURCES += sha1dc/% UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGRAMS += t-mem-pool UNIT_TEST_PROGRAMS += t-prio-queue +UNIT_TEST_PROGRAMS += t-reftable-basics UNIT_TEST_PROGRAMS += t-strbuf UNIT_TEST_PROGRAMS += t-trailer UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) @@ -2671,7 +2672,6 @@ REFTABLE_OBJS += reftable/stack.o REFTABLE_OBJS += reftable/tree.o REFTABLE_OBJS += reftable/writer.o -REFTABLE_TEST_OBJS += reftable/basics_test.o REFTABLE_TEST_OBJS += reftable/block_test.o REFTABLE_TEST_OBJS += reftable/dump.o REFTABLE_TEST_OBJS += reftable/merged_test.o diff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c index bae731669c..9160bc5da6 100644 --- a/t/helper/test-reftable.c +++ b/t/helper/test-reftable.c @@ -5,7 +5,6 @@ int cmd__reftable(int argc, const char **argv) { /* test from simple to complex. */ - basics_test_main(argc, argv); record_test_main(argc, argv); block_test_main(argc, argv); tree_test_main(argc, argv); diff --git a/reftable/basics_test.c b/t/unit-tests/t-reftable-basics.c similarity index 60% rename from reftable/basics_test.c rename to t/unit-tests/t-reftable-basics.c index 997c4d9e01..b6088e1ddd 100644 --- a/reftable/basics_test.c +++ b/t/unit-tests/t-reftable-basics.c @@ -1,16 +1,6 @@ -/* -Copyright 2020 Google LLC - -Use of this source code is governed by a BSD-style -license that can be found in the LICENSE file or at -https://developers.google.com/open-source/licenses/bsd -*/ - -#include "system.h" - -#include "basics.h" -#include "test_framework.h" -#include "reftable-tests.h" +#include "test-lib.h" +#include "reftable/system.h" +#include "reftable/basics.h" struct integer_needle_lesseq_args { int needle; @@ -42,9 +32,8 @@ static void test_binsearch(void) {11, 5}, {9000, 5}, }; - size_t i = 0; - for (i = 0; i < ARRAY_SIZE(testcases); i++) { + for (size_t i = 0; i < ARRAY_SIZE(testcases); i++) { struct integer_needle_lesseq_args args = { .haystack = haystack, .needle = testcases[i].needle, @@ -52,14 +41,14 @@ static void test_binsearch(void) size_t idx; idx = binsearch(ARRAY_SIZE(haystack), &integer_needle_lesseq, &args); - EXPECT(idx == testcases[i].expected_idx); + check_int(idx, ==, testcases[i].expected_idx); } } static void test_names_length(void) { char *a[] = { "a", "b", NULL }; - EXPECT(names_length(a) == 2); + check_int(names_length(a), ==, 2); } static void test_parse_names_normal(void) @@ -67,9 +56,9 @@ static void test_parse_names_normal(void) char in[] = "a\nb\n"; char **out = NULL; parse_names(in, strlen(in), &out); - EXPECT(!strcmp(out[0], "a")); - EXPECT(!strcmp(out[1], "b")); - EXPECT(!out[2]); + check_str(out[0], "a"); + check_str(out[1], "b"); + check(!out[2]); free_names(out); } @@ -78,8 +67,8 @@ static void test_parse_names_drop_empty(void) char in[] = "a\n\n"; char **out = NULL; parse_names(in, strlen(in), &out); - EXPECT(!strcmp(out[0], "a")); - EXPECT(!out[1]); + check_str(out[0], "a"); + check(!out[1]); free_names(out); } @@ -89,17 +78,18 @@ static void test_common_prefix(void) struct strbuf s2 = STRBUF_INIT; strbuf_addstr(&s1, "abcdef"); strbuf_addstr(&s2, "abc"); - EXPECT(common_prefix_size(&s1, &s2) == 3); + check_int(common_prefix_size(&s1, &s2), ==, 3); strbuf_release(&s1); strbuf_release(&s2); } -int basics_test_main(int argc, const char *argv[]) +int cmd_main(int argc, const char *argv[]) { - RUN_TEST(test_common_prefix); - RUN_TEST(test_parse_names_normal); - RUN_TEST(test_parse_names_drop_empty); - RUN_TEST(test_binsearch); - RUN_TEST(test_names_length); - return 0; + TEST(test_common_prefix(), "common_prefix_size works"); + TEST(test_parse_names_normal(), "parse_names works for basic input"); + TEST(test_parse_names_drop_empty(), "parse_names drops empty string"); + TEST(test_binsearch(), "binary search with binsearch works"); + TEST(test_names_length(), "names_length retuns size of a NULL-terminated string array"); + + return test_done(); } -- 2.45.GIT ``` ## Chandra Pratap, 2024-05-28 11:30 Subject: [PATCH 2/2] t: improve upon reftable/basics_test.c in the unit testing framework Message-ID: <20240528113856.8348-3-chandrapratap3519@gmail.com> URL: https://gitlist.dev/e/20240528113856.8348-3-chandrapratap3519%40gmail.com In-Reply-To: <20240528113856.8348-1-chandrapratap3519@gmail.com> ``` Enhance the new test for reftable/basics.{c, h} in the unit testing framework. The enhancements include: - Move tests for functions in reftable/basics.{c, h} from reftable/record_test.c and reftable/stack_test.c to the new unit test. - Add tests for functions that are not currently tested, like put_be16. - Improve the test-cases for the already existing tests. Mentored-by: Patrick Steinhardt Mentored-by: Christian Couder Signed-off-by: Chandra Pratap --- reftable/record_test.c | 37 --------------- reftable/stack_test.c | 25 ----------- t/unit-tests/t-reftable-basics.c | 77 +++++++++++++++++++++++++++----- 3 files changed, 65 insertions(+), 74 deletions(-) diff --git a/reftable/record_test.c b/reftable/record_test.c index c158ee79ff..58290bdba3 100644 --- a/reftable/record_test.c +++ b/reftable/record_test.c @@ -64,31 +64,6 @@ static void test_varint_roundtrip(void) } } -static void test_common_prefix(void) -{ - struct { - const char *a, *b; - int want; - } cases[] = { - { "abc", "ab", 2 }, - { "", "abc", 0 }, - { "abc", "abd", 2 }, - { "abc", "pqr", 0 }, - }; - - int i = 0; - for (i = 0; i < ARRAY_SIZE(cases); i++) { - struct strbuf a = STRBUF_INIT; - struct strbuf b = STRBUF_INIT; - strbuf_addstr(&a, cases[i].a); - strbuf_addstr(&b, cases[i].b); - EXPECT(common_prefix_size(&a, &b) == cases[i].want); - - strbuf_release(&a); - strbuf_release(&b); - } -} - static void set_hash(uint8_t *h, int j) { int i = 0; @@ -258,16 +233,6 @@ static void test_reftable_log_record_roundtrip(void) strbuf_release(&scratch); } -static void test_u24_roundtrip(void) -{ - uint32_t in = 0x112233; - uint8_t dest[3]; - uint32_t out; - put_be24(dest, in); - out = get_be24(dest); - EXPECT(in == out); -} - static void test_key_roundtrip(void) { uint8_t buffer[1024] = { 0 }; @@ -411,9 +376,7 @@ int record_test_main(int argc, const char *argv[]) RUN_TEST(test_reftable_ref_record_roundtrip); RUN_TEST(test_varint_roundtrip); RUN_TEST(test_key_roundtrip); - RUN_TEST(test_common_prefix); RUN_TEST(test_reftable_obj_record_roundtrip); RUN_TEST(test_reftable_index_record_roundtrip); - RUN_TEST(test_u24_roundtrip); return 0; } diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 7889f818d1..6f6af11e53 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -102,29 +102,6 @@ static void test_read_file(void) (void) remove(fn); } -static void test_parse_names(void) -{ - char buf[] = "line\n"; - char **names = NULL; - parse_names(buf, strlen(buf), &names); - - EXPECT(NULL != names[0]); - EXPECT(0 == strcmp(names[0], "line")); - EXPECT(NULL == names[1]); - free_names(names); -} - -static void test_names_equal(void) -{ - char *a[] = { "a", "b", "c", NULL }; - char *b[] = { "a", "b", "d", NULL }; - char *c[] = { "a", "b", NULL }; - - EXPECT(names_equal(a, a)); - EXPECT(!names_equal(a, b)); - EXPECT(!names_equal(a, c)); -} - static int write_test_ref(struct reftable_writer *wr, void *arg) { struct reftable_ref_record *ref = arg; @@ -1048,8 +1025,6 @@ static void test_reftable_stack_compaction_concurrent_clean(void) int stack_test_main(int argc, const char *argv[]) { RUN_TEST(test_empty_add); - RUN_TEST(test_names_equal); - RUN_TEST(test_parse_names); RUN_TEST(test_read_file); RUN_TEST(test_reflog_expire); RUN_TEST(test_reftable_stack_add); diff --git a/t/unit-tests/t-reftable-basics.c b/t/unit-tests/t-reftable-basics.c index b6088e1ddd..53fce33d53 100644 --- a/t/unit-tests/t-reftable-basics.c +++ b/t/unit-tests/t-reftable-basics.c @@ -51,36 +51,87 @@ static void test_names_length(void) check_int(names_length(a), ==, 2); } +static void test_names_equal(void) +{ + char *a[] = { "a", "b", "c", NULL }; + char *b[] = { "a", "b", "d", NULL }; + char *c[] = { "a", "b", NULL }; + + check(names_equal(a, a)); + check(!names_equal(a, b)); + check(!names_equal(a, c)); +} + static void test_parse_names_normal(void) { - char in[] = "a\nb\n"; + char in1[] = "line\n"; + char in2[] = "a\nb\nc"; char **out = NULL; - parse_names(in, strlen(in), &out); + parse_names(in1, strlen(in1), &out); + check_str(out[0], "line"); + check(!out[1]); + free_names(out); + + parse_names(in2, strlen(in2), &out); check_str(out[0], "a"); check_str(out[1], "b"); - check(!out[2]); + check_str(out[2], "c"); + check(!out[3]); free_names(out); } static void test_parse_names_drop_empty(void) { - char in[] = "a\n\n"; + char in[] = "a\n\nb\n"; char **out = NULL; parse_names(in, strlen(in), &out); check_str(out[0], "a"); - check(!out[1]); + /* simply '\n' should be dropped as empty string */ + check_str(out[1], "b"); + check(!out[2]); free_names(out); } static void test_common_prefix(void) { - struct strbuf s1 = STRBUF_INIT; - struct strbuf s2 = STRBUF_INIT; - strbuf_addstr(&s1, "abcdef"); - strbuf_addstr(&s2, "abc"); - check_int(common_prefix_size(&s1, &s2), ==, 3); - strbuf_release(&s1); - strbuf_release(&s2); + struct strbuf a = STRBUF_INIT; + struct strbuf b = STRBUF_INIT; + struct { + const char *a, *b; + int want; + } cases[] = { + {"abcdef", "abc", 3}, + { "abc", "ab", 2 }, + { "", "abc", 0 }, + { "abc", "abd", 2 }, + { "abc", "pqr", 0 }, + }; + + for (size_t i = 0; i < ARRAY_SIZE(cases); i++) { + strbuf_addstr(&a, cases[i].a); + strbuf_addstr(&b, cases[i].b); + check_int(common_prefix_size(&a, &b), ==, cases[i].want); + strbuf_reset(&a); + strbuf_reset(&b); + } + strbuf_release(&a); + strbuf_release(&b); +} + +static void test_be_roundtrip(void) +{ + uint32_t in = 0x112233; + uint8_t dest[3]; + uint32_t out; + /* test put_be24 and get_be24 roundtrip */ + put_be24(dest, in); + out = get_be24(dest); + check_int(in, ==, out); + /* test put_be16 and get_be16 roundtrip */ + in = 0xfef1; + put_be16(dest, in); + out = get_be16(dest); + check_int(in, ==, out); } int cmd_main(int argc, const char *argv[]) @@ -90,6 +141,8 @@ int cmd_main(int argc, const char *argv[]) TEST(test_parse_names_drop_empty(), "parse_names drops empty string"); TEST(test_binsearch(), "binary search with binsearch works"); TEST(test_names_length(), "names_length retuns size of a NULL-terminated string array"); + TEST(test_names_equal(), "names_equal compares NULL-terminated string arrays"); + TEST(test_be_roundtrip(), "put_be24, get_be24 and put_be16 work"); return test_done(); } -- 2.45.GIT ``` ## Christian Couder, 2024-05-28 12:29 Subject: Re: [GSoC][PATCH] t: port reftable/basics_test.c to the unit testing Message-ID: URL: https://gitlist.dev/e/CAP8UFD02%2BbDsCxZACSfuSuHWhEeKBH4boxu%2Bic6xnDo8hMZJqA%40mail.gmail.com In-Reply-To: <20240528113856.8348-1-chandrapratap3519@gmail.com> ``` On Tue, May 28, 2024 at 1:46 PM Chandra Pratap wrote: > > In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit > testing framework written entirely in C was introduced to the Git project > aimed at simplifying testing and reducing test run times. > > Currently, tests for the raftable refs-backend are performed by a custom s/raftable/reftable/ > testing framework defined by reftable/test_framework.{c, h}. Port > reftable/basics_test.c to the unit testing framework and improve upon > the ported test. ``` ## Christian Couder, 2024-05-28 12:34 Subject: Re: [PATCH 1/2] t: move reftable/basics_test.c to the unit testing framework Message-ID: URL: https://gitlist.dev/e/CAP8UFD2ObwdQVE9Ta%2Bth6O-FC9cTR8sNxsrSQWBXosjP4ccqSQ%40mail.gmail.com In-Reply-To: <20240528113856.8348-2-chandrapratap3519@gmail.com> ``` On Tue, May 28, 2024 at 1:45 PM Chandra Pratap wrote: > diff --git a/reftable/basics_test.c b/t/unit-tests/t-reftable-basics.c > similarity index 60% > rename from reftable/basics_test.c > rename to t/unit-tests/t-reftable-basics.c > index 997c4d9e01..b6088e1ddd 100644 > --- a/reftable/basics_test.c > +++ b/t/unit-tests/t-reftable-basics.c > @@ -1,16 +1,6 @@ > -/* > -Copyright 2020 Google LLC > - > -Use of this source code is governed by a BSD-style > -license that can be found in the LICENSE file or at > -https://developers.google.com/open-source/licenses/bsd > -*/ Are we sure it's Ok to remove the above Copyright? Or is it safer to just move it over (and perhaps add our own)? ``` ## Christian Couder, 2024-05-28 12:46 Subject: Re: [PATCH 2/2] t: improve upon reftable/basics_test.c in the unit testing framework Message-ID: URL: https://gitlist.dev/e/CAP8UFD02Ck0M2AOmTtJO3gHr3w%2B3Of7BAAa0Rbc%3DCtMms60nyA%40mail.gmail.com In-Reply-To: <20240528113856.8348-3-chandrapratap3519@gmail.com> ``` On Tue, May 28, 2024 at 1:40 PM Chandra Pratap wrote: > > Enhance the new test for reftable/basics.{c, h} in the unit testing > framework. The enhancements include: > - Move tests for functions in reftable/basics.{c, h} from > reftable/record_test.c and reftable/stack_test.c to the new unit test. It might be easier for readers if one patch moved tests from reftable/record_test.c and a separate patch moved tests from reftable/stack_test.c. > - Add tests for functions that are not currently tested, like put_be16. This could perhaps be done in a separate patch. > - Improve the test-cases for the already existing tests. Are these improvements to tests that were moved in patch 1/2 of this series? If that's the case, these improvements could be in a separate patch too. ``` ## Patrick Steinhardt, 2024-05-29 06:00 Subject: Re: [PATCH 2/2] t: improve upon reftable/basics_test.c in the unit testing framework Message-ID: URL: https://gitlist.dev/e/ZlbEanuHHlwyaRfT%40tanuki In-Reply-To: ``` On Tue, May 28, 2024 at 02:46:17PM +0200, Christian Couder wrote: > On Tue, May 28, 2024 at 1:40 PM Chandra Pratap > wrote: > > > > Enhance the new test for reftable/basics.{c, h} in the unit testing > > framework. The enhancements include: > > - Move tests for functions in reftable/basics.{c, h} from > > reftable/record_test.c and reftable/stack_test.c to the new unit test. > > It might be easier for readers if one patch moved tests from > reftable/record_test.c and a separate patch moved tests from > reftable/stack_test.c. > > > - Add tests for functions that are not currently tested, like put_be16. > > This could perhaps be done in a separate patch. > > > - Improve the test-cases for the already existing tests. > > Are these improvements to tests that were moved in patch 1/2 of this > series? If that's the case, these improvements could be in a separate > patch too. Some of the test cases are extended to also cover additional edge cases. But I agree that this is easy to miss, and also think that this patch should be split up further. Patrick ``` ## Patrick Steinhardt, 2024-05-29 06:00 Subject: Re: [PATCH 1/2] t: move reftable/basics_test.c to the unit testing framework Message-ID: URL: https://gitlist.dev/e/ZlbEcHL3fEFXbt-H%40tanuki In-Reply-To: <20240528113856.8348-2-chandrapratap3519@gmail.com> ``` On Tue, May 28, 2024 at 05:00:02PM +0530, Chandra Pratap wrote: > --- a/reftable/basics_test.c > +++ b/t/unit-tests/t-reftable-basics.c > @@ -1,16 +1,6 @@ > -/* > -Copyright 2020 Google LLC > - > -Use of this source code is governed by a BSD-style > -license that can be found in the LICENSE file or at > -https://developers.google.com/open-source/licenses/bsd > -*/ > - > -#include "system.h" > - > -#include "basics.h" > -#include "test_framework.h" > -#include "reftable-tests.h" > +#include "test-lib.h" > +#include "reftable/system.h" There is no need to include "reftable/system.h" explicitly as it is already transitively included via "reftable/basics.h". The same should be true for all other reftable headers. Patrick ``` ## Patrick Steinhardt, 2024-05-29 06:00 Subject: Re: [PATCH 1/2] t: move reftable/basics_test.c to the unit testing framework Message-ID: URL: https://gitlist.dev/e/ZlbEdZNbrVNEHFLw%40tanuki In-Reply-To: ``` On Tue, May 28, 2024 at 02:34:05PM +0200, Christian Couder wrote: > On Tue, May 28, 2024 at 1:45 PM Chandra Pratap > wrote: > > > diff --git a/reftable/basics_test.c b/t/unit-tests/t-reftable-basics.c > > similarity index 60% > > rename from reftable/basics_test.c > > rename to t/unit-tests/t-reftable-basics.c > > index 997c4d9e01..b6088e1ddd 100644 > > --- a/reftable/basics_test.c > > +++ b/t/unit-tests/t-reftable-basics.c > > @@ -1,16 +1,6 @@ > > -/* > > -Copyright 2020 Google LLC > > - > > -Use of this source code is governed by a BSD-style > > -license that can be found in the LICENSE file or at > > -https://developers.google.com/open-source/licenses/bsd > > -*/ > > Are we sure it's Ok to remove the above Copyright? Or is it safer to > just move it over (and perhaps add our own)? Good question indeed. We should retain the copyright header given that these source files have a different license compared to the rest of the codebase. Patrick ```