| Message ID | 20240519204530.12258-3-shyamthakkar001@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GSoC,v2] t/: port helper/test-strcmp-offset.c to unit-tests/t-strcmp-offset.c | expand |
Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes: > The v2 only adjusts the formatting to be in line with the style > described in CodingGuidelines. And it is also rebased on 'next' to > avoid Makefile conflicts. Please do not base a new topic on 'next', as I will NOT be applying it on top of 'next'. Imagine what it takes for a topic to graduate to 'master', if it started from the tip of 'next'? You have to wait for ALL other topics that are in 'next' to graduate. Instead, new things are to be built on 'master' and fixes are to be built on the oldest maintenance track the fix is relevant. After building such a base, you can and should make a trial merge of your topic into 'next' and 'seen' to see if your work interferes with work by somebody else, which often gives you a good learning opportunity. Unless the conflicts are severe and is impractical, in which case see Documentation/SubmittingPatches and look for "Under truly exceptional circumstances". But the conflict in Makefile about UNIT_TEST_PROGRAMS in this case hadly qualifies as one. Anyway, thanks for a patch. Now for the patch text itself. The original looked like this. > index d8473cf2fc..0000000000 > --- a/t/helper/test-strcmp-offset.c > +++ /dev/null > @@ -1,23 +0,0 @@ > -#include "test-tool.h" > -#include "read-cache-ll.h" > - > -int cmd__strcmp_offset(int argc UNUSED, const char **argv) > -{ > - int result; > - size_t offset; > - > - if (!argv[1] || !argv[2]) > - die("usage: %s <string1> <string2>", argv[0]); > - > - result = strcmp_offset(argv[1], argv[2], &offset); > - > - /* > - * Because different CRTs behave differently, only rely on signs > - * of the result values. > - */ > - result = (result < 0 ? -1 : > - result > 0 ? 1 : > - 0); > - printf("%d %"PRIuMAX"\n", result, (uintmax_t)offset); > - return 0; > -} It used to print the result and the discovered offset to the standard output, which is used in the comparison of the calling test script. Now we are doing the check ourselves here, that part needs to be different. But other than that, there shouldn't be any change. > +static void check_strcmp_offset(const char *string1, const char *string2, > + int expect_result, uintmax_t expect_offset) > +{ > + size_t offset; > + int result = strcmp_offset(string1, string2, &offset); > + > + /* > + * Because different CRTs behave differently, only rely on signs of the > + * result values. > + */ > + result = (result < 0 ? -1 : > + result > 0 ? 1 : > + 0); > + > + check_int(result, ==, expect_result); > + check_uint((uintmax_t)offset, ==, expect_offset); > +} Subtle differences that do not seem to have any good reason to be there relative to the original, namely, the order of declarations of two local variables, how 'result' is initialized, how the exact same comment is line-wrapped differently. If there weren't such changes, the output from "git show --color-moved" would have allowed readers' eyes coast over them, but because of them, they need to read most of the lines. That's an inefficient use of their time. The test used to be > -while read s1 s2 expect > -do > - test_expect_success "strcmp_offset($s1, $s2)" ' > - echo "$expect" >expect && > - test-tool strcmp-offset "$s1" "$s2" >actual && > - test_cmp expect actual > - ' > -done <<-EOF > -abc abc 0 3 > -abc def -1 0 > -abc abz -1 2 > -abc abcdef -1 3 > -EOF so a misbehaving "strcmp-offset abc abc" that gives different result can be seen by showing say -abc abc 0 3 +abc abc -1 2 if it incorrectly say that the first difference is at the offset 2 and reported that the first "abc" sorts before the second "abc". With the new code, how would the failing test look like? > +#define TEST_STRCMP_OFFSET(string1, string2, expect_result, expect_offset) \ > + TEST(check_strcmp_offset(string1, string2, expect_result, \ > + expect_offset), \ > + "strcmp_offset(%s, %s) works", #string1, #string2) That's a neat way to use the #stringification. Without it the message will say strcmp_offset(abc, abc) works but it is properly C-quoted, i.e. strcmp_offset("abc", "abc") works If the second string were "abc\t", then the benefit of using #string2 would become even more prominent. Having said that, output from overly generic: > + check_int(result, ==, expect_result); > + check_uint((uintmax_t)offset, ==, expect_offset); used in the check_strcmp_offset() function is not all that readable, especially given that it comes _before_ the test title. It shows something like # check "result == expect_result" failed # left: -1 # right: 0 Hopefully it would automatically improve as the framework gets improved, perhaps to say # check "result == expect_result" failed # result = -1, expect_result = 0 In any case, it is a general problem of the current unit test framework and outside the scope of this patch. > +int cmd_main(int argc, const char **argv) n> +{ > + TEST_STRCMP_OFFSET("abc", "abc", 0, 3); > + TEST_STRCMP_OFFSET("abc", "def", -1, 0); > + TEST_STRCMP_OFFSET("abc", "abz", -1, 2); > + TEST_STRCMP_OFFSET("abc", "abcdef", -1, 3); > + > + return test_done(); > +} Looking good.
Junio C Hamano <gitster@pobox.com> writes: > Please do not base a new topic on 'next', as I will NOT be applying > it on top of 'next'. > ... > Unless the conflicts are severe and is impractical, in which case > see Documentation/SubmittingPatches and look for "Under truly > exceptional circumstances". But the conflict in Makefile about > UNIT_TEST_PROGRAMS in this case hadly qualifies as one. > > Anyway, thanks for a patch. I've backported the patch to apply to "master" and queued it on its own topic, so that it no longer has to wait for all other topic in 'next'. The Makefile looks like the attached, which is just with trivial difference in the context. We only need to remove strcmp-offset from the TEST_BUILTIN_OBJS and instead add a corresponding one to UNIT_TEST_PROGRAMS, and that does not change no matter what other test-*.o are added to the former or t-* are added to the latter. We may want to sort the UNIT_TEST_PROGRAMS list alphabetically at some point, by the way. Thanks. diff --git a/Makefile b/Makefile index cf504963c2..1afa112706 100644 --- a/Makefile +++ b/Makefile @@ -839,7 +839,6 @@ TEST_BUILTINS_OBJS += test-sha1.o TEST_BUILTINS_OBJS += test-sha256.o TEST_BUILTINS_OBJS += test-sigchain.o TEST_BUILTINS_OBJS += test-simple-ipc.o -TEST_BUILTINS_OBJS += test-strcmp-offset.o TEST_BUILTINS_OBJS += test-string-list.o TEST_BUILTINS_OBJS += test-submodule-config.o TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o @@ -1338,6 +1337,7 @@ UNIT_TEST_PROGRAMS += t-mem-pool UNIT_TEST_PROGRAMS += t-strbuf UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGRAMS += t-prio-queue +UNIT_TEST_PROGRAMS += t-strcmp-offset UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
On Mon, 20 May 2024, Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Please do not base a new topic on 'next', as I will NOT be applying > > it on top of 'next'. > > ... > > Unless the conflicts are severe and is impractical, in which case > > see Documentation/SubmittingPatches and look for "Under truly > > exceptional circumstances". But the conflict in Makefile about > > UNIT_TEST_PROGRAMS in this case hadly qualifies as one. Will make sure for the next time. > > Anyway, thanks for a patch. > > I've backported the patch to apply to "master" and queued it on its > own topic, so that it no longer has to wait for all other topic in > 'next'. The Makefile looks like the attached, which is just with > trivial difference in the context. We only need to remove > strcmp-offset from the TEST_BUILTIN_OBJS and instead add a > corresponding one to UNIT_TEST_PROGRAMS, and that does not change no > matter what other test-*.o are added to the former or t-* are added > to the latter. > > We may want to sort the UNIT_TEST_PROGRAMS list alphabetically at > some point, by the way. That was aleady done in 'la/hide-trailer-info' (next). Thanks.
diff --git a/Makefile b/Makefile index 8f4432ae57..59d98ba688 100644 --- a/Makefile +++ b/Makefile @@ -839,7 +839,6 @@ TEST_BUILTINS_OBJS += test-sha1.o TEST_BUILTINS_OBJS += test-sha256.o TEST_BUILTINS_OBJS += test-sigchain.o TEST_BUILTINS_OBJS += test-simple-ipc.o -TEST_BUILTINS_OBJS += test-strcmp-offset.o TEST_BUILTINS_OBJS += test-string-list.o TEST_BUILTINS_OBJS += test-submodule-config.o TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o @@ -1338,6 +1337,7 @@ UNIT_TEST_PROGRAMS += t-ctype UNIT_TEST_PROGRAMS += t-mem-pool UNIT_TEST_PROGRAMS += t-prio-queue UNIT_TEST_PROGRAMS += t-strbuf +UNIT_TEST_PROGRAMS += t-strcmp-offset UNIT_TEST_PROGRAMS += t-trailer UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS)) UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS)) diff --git a/t/helper/test-strcmp-offset.c b/t/helper/test-strcmp-offset.c deleted file mode 100644 index d8473cf2fc..0000000000 --- a/t/helper/test-strcmp-offset.c +++ /dev/null @@ -1,23 +0,0 @@ -#include "test-tool.h" -#include "read-cache-ll.h" - -int cmd__strcmp_offset(int argc UNUSED, const char **argv) -{ - int result; - size_t offset; - - if (!argv[1] || !argv[2]) - die("usage: %s <string1> <string2>", argv[0]); - - result = strcmp_offset(argv[1], argv[2], &offset); - - /* - * Because different CRTs behave differently, only rely on signs - * of the result values. - */ - result = (result < 0 ? -1 : - result > 0 ? 1 : - 0); - printf("%d %"PRIuMAX"\n", result, (uintmax_t)offset); - return 0; -} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index f6fd0fe491..7ad7d07018 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -78,7 +78,6 @@ static struct test_cmd cmds[] = { { "sha256", cmd__sha256 }, { "sigchain", cmd__sigchain }, { "simple-ipc", cmd__simple_ipc }, - { "strcmp-offset", cmd__strcmp_offset }, { "string-list", cmd__string_list }, { "submodule", cmd__submodule }, { "submodule-config", cmd__submodule_config }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 868f33453c..d14b3072bd 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -71,7 +71,6 @@ int cmd__oid_array(int argc, const char **argv); int cmd__sha256(int argc, const char **argv); int cmd__sigchain(int argc, const char **argv); int cmd__simple_ipc(int argc, const char **argv); -int cmd__strcmp_offset(int argc, const char **argv); int cmd__string_list(int argc, const char **argv); int cmd__submodule(int argc, const char **argv); int cmd__submodule_config(int argc, const char **argv); diff --git a/t/t0065-strcmp-offset.sh b/t/t0065-strcmp-offset.sh deleted file mode 100755 index 94e34c83ed..0000000000 --- a/t/t0065-strcmp-offset.sh +++ /dev/null @@ -1,22 +0,0 @@ -#!/bin/sh - -test_description='Test strcmp_offset functionality' - -TEST_PASSES_SANITIZE_LEAK=true -. ./test-lib.sh - -while read s1 s2 expect -do - test_expect_success "strcmp_offset($s1, $s2)" ' - echo "$expect" >expect && - test-tool strcmp-offset "$s1" "$s2" >actual && - test_cmp expect actual - ' -done <<-EOF -abc abc 0 3 -abc def -1 0 -abc abz -1 2 -abc abcdef -1 3 -EOF - -test_done diff --git a/t/unit-tests/t-strcmp-offset.c b/t/unit-tests/t-strcmp-offset.c new file mode 100644 index 0000000000..fe4c2706b1 --- /dev/null +++ b/t/unit-tests/t-strcmp-offset.c @@ -0,0 +1,35 @@ +#include "test-lib.h" +#include "read-cache-ll.h" + +static void check_strcmp_offset(const char *string1, const char *string2, + int expect_result, uintmax_t expect_offset) +{ + size_t offset; + int result = strcmp_offset(string1, string2, &offset); + + /* + * Because different CRTs behave differently, only rely on signs of the + * result values. + */ + result = (result < 0 ? -1 : + result > 0 ? 1 : + 0); + + check_int(result, ==, expect_result); + check_uint((uintmax_t)offset, ==, expect_offset); +} + +#define TEST_STRCMP_OFFSET(string1, string2, expect_result, expect_offset) \ + TEST(check_strcmp_offset(string1, string2, expect_result, \ + expect_offset), \ + "strcmp_offset(%s, %s) works", #string1, #string2) + +int cmd_main(int argc, const char **argv) +{ + TEST_STRCMP_OFFSET("abc", "abc", 0, 3); + TEST_STRCMP_OFFSET("abc", "def", -1, 0); + TEST_STRCMP_OFFSET("abc", "abz", -1, 2); + TEST_STRCMP_OFFSET("abc", "abcdef", -1, 3); + + return test_done(); +}