Web lists-archives.com

Re: [PATCH v7 1/3] read-cache: add strcmp_offset function




On Fri, Apr 07, 2017 at 09:20:45PM +0000, git@xxxxxxxxxxxxxxxxx wrote:

> diff --git a/t/helper/test-strcmp-offset.c b/t/helper/test-strcmp-offset.c
> new file mode 100644
> index 0000000..03e1eef
> --- /dev/null
> +++ b/t/helper/test-strcmp-offset.c
> @@ -0,0 +1,64 @@
> +#include "cache.h"
> +
> +struct test_data {
> +	const char *s1;
> +	const char *s2;
> +	size_t expected_first_change; /* or strlen() when equal */
> +};
> +
> +static struct test_data data[] = {
> +	{ "abc", "abc", 3 },
> +	{ "abc", "def", 0 },
> +
> +	{ "abc", "abz", 2 },
> +
> +	{ "abc", "abcdef", 3 },
> +
> +	{ "abc\xF0zzz", "abc\xFFzzz", 3 },
> +
> +	{ NULL, NULL, 0 }
> +};

I've been thinking a bit on the comments on earlier rounds regarding C
tests. I think in the early days, we generally had plumbing that exposed
the C interfaces in a pretty transparent way. I.e., it was reasonable to
unit-test update-index, because you pretty much know how input to it
will map to the code and data structures used.

These days we have more C infrastructure, and it's sometimes hard to
tickle the exact inputs to those modules through plumbing commands. So I
don't really object to adding module-specific helpers that make it easy
to unit test these underlying modules.

I'm not sure how I feel about sticking test data in the helpers, though.
A higher level language like shell is actually pretty good for passing
data around. Passing in the input makes it much easier to prod a helper
with new test cases, see its output, run it in a debugger for a specific
case, etc.

It also integrates better with our test suite. For instance, here:

> +	if (r_tst_sign != r_exp_sign) {
> +		error("FAIL: '%s' vs '%s', result expect %d, observed %d\n",
> +			  sa, sb, r_exp_sign, r_tst_sign);
> +		failed = 1;
> +	}
> +
> +	if (offset != expected_first_change) {
> +		error("FAIL: '%s' vs '%s', offset expect %lu, observed %lu\n",
> +			  sa, sb, expected_first_change, offset);
> +		failed = 1;
> +	}
> +
> +	return failed;
> +}

You're essentially rebuilding a test harness just for this one function,
and the regular test harness only knows "did anything fail", and nothing
about specific tests.

Perhaps something like:

 t/helper/test-strcmp-offset.c | 66 +++++++----------------------------
 t/t0065-strcmp-offset.sh      | 17 +++++++--
 2 files changed, 26 insertions(+), 57 deletions(-)

diff --git a/t/helper/test-strcmp-offset.c b/t/helper/test-strcmp-offset.c
index 03e1eef8a..1fdf4d137 100644
--- a/t/helper/test-strcmp-offset.c
+++ b/t/helper/test-strcmp-offset.c
@@ -1,64 +1,22 @@
 #include "cache.h"
 
-struct test_data {
-	const char *s1;
-	const char *s2;
-	size_t expected_first_change; /* or strlen() when equal */
-};
-
-static struct test_data data[] = {
-	{ "abc", "abc", 3 },
-	{ "abc", "def", 0 },
-
-	{ "abc", "abz", 2 },
-
-	{ "abc", "abcdef", 3 },
-
-	{ "abc\xF0zzz", "abc\xFFzzz", 3 },
-
-	{ NULL, NULL, 0 }
-};
-
-int try_pair(const char *sa, const char *sb, size_t expected_first_change)
+int cmd_main(int argc, const char **argv)
 {
-	int failed = 0;
-	int r_exp, r_tst, r_exp_sign, r_tst_sign;
+	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 differnt CRTs behave differently, only rely on signs
 	 * of the result values.
 	 */
-	r_exp = strcmp(sa, sb);
-	r_exp_sign = ((r_exp < 0) ? -1 : ((r_exp == 0) ? 0 : 1));
-
-	r_tst = strcmp_offset(sa, sb, &offset);
-	r_tst_sign = ((r_tst < 0) ? -1 : ((r_tst == 0) ? 0 : 1));
-
-	if (r_tst_sign != r_exp_sign) {
-		error("FAIL: '%s' vs '%s', result expect %d, observed %d\n",
-			  sa, sb, r_exp_sign, r_tst_sign);
-		failed = 1;
-	}
-
-	if (offset != expected_first_change) {
-		error("FAIL: '%s' vs '%s', offset expect %lu, observed %lu\n",
-			  sa, sb, expected_first_change, offset);
-		failed = 1;
-	}
-
-	return failed;
-}
-
-int cmd_main(int argc, const char **argv)
-{
-	int failed = 0;
-	int k;
-
-	for (k=0; data[k].s1; k++) {
-		failed += try_pair(data[k].s1, data[k].s2, data[k].expected_first_change);
-		failed += try_pair(data[k].s2, data[k].s1, data[k].expected_first_change);
-	}
-
-	return failed;
+	result = result < 0 ? -1 :
+		 result > 0 ? 1 :
+		 0;
+	printf("%d %"PRIuMAX"\n", result, (uintmax_t)offset);
+	return 0;
 }
diff --git a/t/t0065-strcmp-offset.sh b/t/t0065-strcmp-offset.sh
index 0176c8c92..8c167d24b 100755
--- a/t/t0065-strcmp-offset.sh
+++ b/t/t0065-strcmp-offset.sh
@@ -4,8 +4,19 @@ test_description='Test strcmp_offset functionality'
 
 . ./test-lib.sh
 
-test_expect_success run_helper '
-	test-strcmp-offset
-'
+while read s1 s2 expect
+do
+	test_expect_success "strcmp_offset($s1, $s2)" '
+		echo "$expect" >expect &&
+		test-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
+$(printf "abc\360zzz") $(printf "abc\377zzz") -1 3
+EOF
 
 test_done