Re: [PATCH v7 1/3] read-cache: add strcmp_offset function
- From
Jeff King <peff@peff.net>
- Date
- Apr 8, 2017, 10:11 UTC
- Message-ID
- <20170408101142.iztq3lxcaayw2w6q@sigill.intra.peff.net>
- In-Reply-To
- <20170407212047.64950-2-git@jeffhostetler.com>
On Fri, Apr 07, 2017 at 09:20:45PM +0000, git@jeffhostetler.com wrote:
Show 26 quoted lines
> 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:
Show 14 quoted lines
> + 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