Re: [PATCH 05/20] userdiff tests: list builtin drivers via test-tool
flat view
From: Eric Sunshine <hidden>
Date: 2021-02-15 01:25:23
On Sun, Feb 14, 2021 at 7:56 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Change the userdiff test to list the builtin drivers via the test-tool, using the new for_each_userdiff_driver() API function. [...] I only need the "list-builtin-drivers "argument here, but let's add "list-custom-drivers" and "list-drivers" too, just because it's easy. Signed-off-by: Ævar Arnfjörð Bjarmason <redacted> ---diff --git a/t/helper/test-userdiff.c b/t/helper/test-userdiff.c@@ -0,0 +1,32 @@ +static int userdiff_list_builtin_drivers_cb(struct userdiff_driver *driver, + enum userdiff_driver_type type, + void *priv) +{ + puts(driver->name); + return 0; +}
Nit: The word "builtin" in the name of this callback function made me search the patch for its cousin function which would be used for custom drivers. It was only after reading on that I discovered that this one function is used for both builtin and custom drivers. (Caught me off guard, but not itself worth a re-roll.)
quoted hunk ↗ jump to hunk
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh@@ -8,6 +8,10 @@ test_description='Test custom diff function name patterns' test_expect_success 'setup' ' + test-tool userdiff list-builtin-drivers >builtin-drivers && + test_file_not_empty builtin-drivers && + builtin_drivers=$(cat builtin-drivers) &&
Nit: I get why you are using test_file_not_empty() for its diagnostic
value, but it does confuse the reader a bit to see the file
"builtin-drivers" created for no particularly good reason. This could
easily have been:
builtin_drivers=$(test-tool userdiff list-builtin-drivers) &&
test -n "$builtin_drivers" &&
Subjective and not worth a re-roll.