Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests

[Junio C Hamano]

Stefan Beller  writes:

>> +   if (!item)
>> +   ; /* not among entries being checked */
>> +   else {
>> +   if (strcmp((const char *)item->util, buf.buf)) {
>> +   printf("expected '%s', got '%s'\n",
>> +  (char *)item->util, buf.buf);
>> +   *status = 1;
>> +   }
>> +   }
>> +   }
>> +   strbuf_reset();
>
> strbuf_release ?

Thanks for spotting a leak.

I originally had the buf as static, as all generated strings are
short and of similar length, in an attempt to reuse the already
allocated storage instead of allocating it from scratch every call.

>>
>> return 0;
>
> return ret; ? Otherwise `ret` is unused.

This, too.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests

[Junio C Hamano]

Stefan Beller  writes:

>> +   *colon = '\0';
>> +   item = string_list_lookup(expect, buf.buf);
>> +   *colon = ':';
>
> I have been staring at this for a good couple of minutes and wondered if this
> low level string manipulation is really the best way to do it.

It just shows that string_list API was not designed as richly as
others, compared to say the more complete API like strbuf.  If it
had a  variant, I wouldn't have needed the "temporary
termination to get a string" hack.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests

[Stefan Beller]

On Thu, May 5, 2016 at 7:57 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> instead of filtering afterwards, i.e. each strbuf_add is guarded by
>> an
>>
>>  if (is_interesting_output(...))
>> strbuf_add(...)
>
> That's a good approach.
>
> The implementation gets a bit trickier than the previous one, but it
> would look like this.  Discard 2/3 and 3/3 and replace them with
> this one.
>
> The external interface on the input side is no different, but on the
> output side, this version has "expected '%s', got '%s'" error, in
> the same spirit as the output from "test_cmp", added in.
>
> Instead of checking the entire output line-by-line for each expected
> output (in case you did not notice, you can give --expect='quiet: 3'
> --expect='abbrev: 7' and both must match), this one will check each
> output line against each expected pattern.  We wouldn't have too
> many entries in the variable dump and we wouldn't be taking too many
> --expect options, so the matching performance would not matter,
> though.
>
>
>  t/t0040-parse-options.sh |  1 +
>  test-parse-options.c | 88 
> 
>  2 files changed, 75 insertions(+), 14 deletions(-)
>
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index dbaee55..d678fbf 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -45,6 +45,7 @@ Standard options
>  -v, --verbose be verbose
>  -n, --dry-run dry run
>  -q, --quiet   be quiet
> +--expect  expected output in the variable dump
>
>  EOF
>
> diff --git a/test-parse-options.c b/test-parse-options.c
> index b5f4e90..e3f25df 100644
> --- a/test-parse-options.c
> +++ b/test-parse-options.c
> @@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const 
> char *arg, int unset)
> return 0;
>  }
>
> +static int collect_expect(const struct option *opt, const char *arg, int 
> unset)
> +{
> +   struct string_list *expect;
> +   struct string_list_item *item;
> +   struct strbuf label = STRBUF_INIT;
> +   const char *colon;
> +
> +   if (!arg || unset)
> +   die("malformed --expect option");
> +
> +   expect = (struct string_list *)opt->value;
> +   colon = strchr(arg, ':');
> +   if (!colon)
> +   die("malformed --expect option, lacking a colon");
> +   strbuf_add(, arg, colon - arg);
> +   item = string_list_insert(expect, strbuf_detach(, NULL));
> +   if (item->util)
> +   die("malformed --expect option, duplicate %s", label.buf);
> +   item->util = (void *)arg;
> +   return 0;
> +}
> +
> +__attribute__((format (printf,3,4)))
> +static void show(struct string_list *expect, int *status, const char *fmt, 
> ...)
> +{
> +   struct string_list_item *item;
> +   struct strbuf buf = STRBUF_INIT;
> +   va_list args;
> +
> +   va_start(args, fmt);
> +   strbuf_vaddf(, fmt, args);
> +   va_end(args);
> +
> +   if (!expect->nr)
> +   printf("%s\n", buf.buf);
> +   else {
> +   char *colon = strchr(buf.buf, ':');
> +   if (!colon)
> +   die("malformed output format, output lacking colon: 
> %s", fmt);
> +   *colon = '\0';
> +   item = string_list_lookup(expect, buf.buf);
> +   *colon = ':';

I have been staring at this for a good couple of minutes and wondered if this
low level string manipulation is really the best way to do it.

(It feels very C idiomatic, not using a lot of Gits own data
structures. I would have
expected some sort of skip_prefix just with partial regular expression or a
string_list_split_in_place for the splitting. But this "set and reset *colon"
seems to be optimal here)

> +   if (!item)
> +   ; /* not among entries being checked */
> +   else {
> +   if (strcmp((const char *)item->util, buf.buf)) {
> +   printf("expected '%s', got '%s'\n",
> +  (char *)item->util, buf.buf);
> +   *status = 1;
> +   }
> +   }
> +   }
> +   strbuf_reset();

strbuf_release ?

> +}
> +
>  int main(int argc, char **argv)
>  {
> const char *prefix = "prefix/";
> @@ -46,6 +101,7 @@ int main(int argc, char **argv)
> "test-parse-options ",
> NULL
> };
> +   struct string_list expect = STRING_LIST_INIT_NODUP;
> struct option options[] = {
> OPT_BOOL(0, "yes", , "get a boolean"),
> OPT_BOOL('D', "no-doubt", , "begins with 'no-'"),
> @@ -86,34 +142,38 @@ int main(int argc, char **argv)
> OPT__VERBOSE(, "be verbose"),
> OPT__DRY_RUN(_run, "dry run"),
> OPT__QUIET(, "be quiet"),
> + 

Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests

[Junio C Hamano]

Stefan Beller  writes:

> instead of filtering afterwards, i.e. each strbuf_add is guarded by
> an
>
>  if (is_interesting_output(...))
> strbuf_add(...)

That's a good approach.

The implementation gets a bit trickier than the previous one, but it
would look like this.  Discard 2/3 and 3/3 and replace them with
this one.

The external interface on the input side is no different, but on the
output side, this version has "expected '%s', got '%s'" error, in
the same spirit as the output from "test_cmp", added in.

Instead of checking the entire output line-by-line for each expected
output (in case you did not notice, you can give --expect='quiet: 3'
--expect='abbrev: 7' and both must match), this one will check each
output line against each expected pattern.  We wouldn't have too
many entries in the variable dump and we wouldn't be taking too many
--expect options, so the matching performance would not matter,
though.


 t/t0040-parse-options.sh |  1 +
 test-parse-options.c | 88 
 2 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index dbaee55..d678fbf 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -45,6 +45,7 @@ Standard options
 -v, --verbose be verbose
 -n, --dry-run dry run
 -q, --quiet   be quiet
+--expect  expected output in the variable dump
 
 EOF
 
diff --git a/test-parse-options.c b/test-parse-options.c
index b5f4e90..e3f25df 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -39,6 +39,61 @@ static int number_callback(const struct option *opt, const 
char *arg, int unset)
return 0;
 }
 
+static int collect_expect(const struct option *opt, const char *arg, int unset)
+{
+   struct string_list *expect;
+   struct string_list_item *item;
+   struct strbuf label = STRBUF_INIT;
+   const char *colon;
+
+   if (!arg || unset)
+   die("malformed --expect option");
+
+   expect = (struct string_list *)opt->value;
+   colon = strchr(arg, ':');
+   if (!colon)
+   die("malformed --expect option, lacking a colon");
+   strbuf_add(, arg, colon - arg);
+   item = string_list_insert(expect, strbuf_detach(, NULL));
+   if (item->util)
+   die("malformed --expect option, duplicate %s", label.buf);
+   item->util = (void *)arg;
+   return 0;
+}
+
+__attribute__((format (printf,3,4)))
+static void show(struct string_list *expect, int *status, const char *fmt, ...)
+{
+   struct string_list_item *item;
+   struct strbuf buf = STRBUF_INIT;
+   va_list args;
+
+   va_start(args, fmt);
+   strbuf_vaddf(, fmt, args);
+   va_end(args);
+
+   if (!expect->nr)
+   printf("%s\n", buf.buf);
+   else {
+   char *colon = strchr(buf.buf, ':');
+   if (!colon)
+   die("malformed output format, output lacking colon: 
%s", fmt);
+   *colon = '\0';
+   item = string_list_lookup(expect, buf.buf);
+   *colon = ':';
+   if (!item)
+   ; /* not among entries being checked */
+   else {
+   if (strcmp((const char *)item->util, buf.buf)) {
+   printf("expected '%s', got '%s'\n",
+  (char *)item->util, buf.buf);
+   *status = 1;
+   }
+   }
+   }
+   strbuf_reset();
+}
+
 int main(int argc, char **argv)
 {
const char *prefix = "prefix/";
@@ -46,6 +101,7 @@ int main(int argc, char **argv)
"test-parse-options ",
NULL
};
+   struct string_list expect = STRING_LIST_INIT_NODUP;
struct option options[] = {
OPT_BOOL(0, "yes", , "get a boolean"),
OPT_BOOL('D', "no-doubt", , "begins with 'no-'"),
@@ -86,34 +142,38 @@ int main(int argc, char **argv)
OPT__VERBOSE(, "be verbose"),
OPT__DRY_RUN(_run, "dry run"),
OPT__QUIET(, "be quiet"),
+   OPT_CALLBACK(0, "expect", , "string",
+"expected output in the variable dump",
+collect_expect),
OPT_END(),
};
int i;
+   int ret = 0;
 
argc = parse_options(argc, (const char **)argv, prefix, options, usage, 
0);
 
if (length_cb.called) {
const char *arg = length_cb.arg;
int unset = length_cb.unset;
-   printf("Callback: \"%s\", %d\n",
-  (arg ? arg : "not set"), unset);
+   show(, , "Callback: \"%s\", %d",
+(arg ? arg : "not set"), unset);
}
-   printf("boolean: %d\n", boolean);
-   printf("integer: %d\n", 

Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests

[Eric Sunshine]

On Thu, May 5, 2016 at 8:41 PM, Stefan Beller  wrote:
> On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano  wrote:
>> [...]
>> But the only thing this test cares about is if "quiet: 3" is in the
>> output.  We should be able to write the above 18 lines with just
>> four lines, like this:
>>
>> test_expect_success 'multiple quiet levels' '
>> test-parse-options --expect="quiet: 3" -q -q -q
>> '
>>
>> Teach the new --expect= option to test-parse-options helper.
>>
>> Signed-off-by: Junio C Hamano 
>> ---
>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
>> +/*
>> + * See if expect->string ("label: value") has a line in output that
>> + * begins with "label:", and if the line in output matches it.
>> + */
>> +static int match_line(struct string_list_item *expect, struct strbuf 
>> *output)
>> +{
>> +   [...]
>> +   const char *scan = output->buf;
>> +   [...]
>> +   while (scan < output->buf + output->len) {
>> +   const char *next;
>> +   scan = memmem(scan, output->buf + output->len - scan,
>> + label, label_len);
>> +   if (!scan)
>> +   return 0;
>> +   if (scan == output->buf || scan[-1] == '\n')
>
> Does scan[-1] work for the first line?

Take note of the short-circuiting '||' operator.

> On a philosophical level this patch series is adding a
> trailing "|grep $X" for the test-parse-options.
> I think such a grep pattern is a good thing because it is
> cheap to implement in unix like environments.
>
> This however is a lot of C code for finding specific subsets
> in the output, so it is not quite cheap. Then we could also go
> the non-wasteful way and instead check what to add to the strbuf
> instead of filtering afterwards, i.e. each strbuf_add is guarded by
> an
>
>  if (is_interesting_output(...))
> strbuf_add(...)

I agree that this is adds far more complexity than I had expected upon
reading Junio's suggestion about simplifying the t0040 tests. Patch 1
aside (which seems a desirable change), rather than patches 2 and 3, I
had expected to see only introduction of a minor helper function in
t0040; perhaps something like this:

options_expect () {
expect="$1" &&
shift &&
test-parse-options "$@" >actual &&
grep "$expect" actual
}

and tests updated like this:

options_expect "quiet: 3" -q -q -q
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] test-parse-options: --expect= option to simplify tests

[Stefan Beller]

On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano  wrote:
> Existing tests in t0040 follow a rather verbose pattern:
>
> cat >expect <<\EOF
> boolean: 0
> integer: 0
> magnitude: 0
> timestamp: 0
> string: (not set)
> abbrev: 7
> verbose: 0
> quiet: 3
> dry run: no
> file: (not set)
> EOF
>
> test_expect_success 'multiple quiet levels' '
> test-parse-options -q -q -q >output 2>output.err &&
> test_must_be_empty output.err &&
> test_cmp expect output
> '
>
> But the only thing this test cares about is if "quiet: 3" is in the
> output.  We should be able to write the above 18 lines with just
> four lines, like this:
>
> test_expect_success 'multiple quiet levels' '
> test-parse-options --expect="quiet: 3" -q -q -q
> '
>
> Teach the new --expect= option to test-parse-options helper.
>
> Signed-off-by: Junio C Hamano 
> ---
>  t/t0040-parse-options.sh |  1 +
>  test-parse-options.c | 68 
> +---
>  2 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index dbaee55..d678fbf 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -45,6 +45,7 @@ Standard options
>  -v, --verbose be verbose
>  -n, --dry-run dry run
>  -q, --quiet   be quiet
> +--expect  expected output in the variable dump
>
>  EOF
>
> diff --git a/test-parse-options.c b/test-parse-options.c
> index 3db4332..010f3b2 100644
> --- a/test-parse-options.c
> +++ b/test-parse-options.c
> @@ -14,6 +14,7 @@ static char *string = NULL;
>  static char *file = NULL;
>  static int ambiguous;
>  static struct string_list list;
> +static struct string_list expect;
>
>  static struct {
> int called;
> @@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const 
> char *arg, int unset)
> return 0;
>  }
>
> +/*
> + * See if expect->string ("label: value") has a line in output that
> + * begins with "label:", and if the line in output matches it.
> + */
> +static int match_line(struct string_list_item *expect, struct strbuf *output)
> +{
> +   const char *label = expect->string;
> +   const char *colon = strchr(label, ':');
> +   const char *scan = output->buf;
> +   size_t label_len, expect_len;
> +
> +   if (!colon)
> +   die("Malformed --expect value: %s", label);
> +   label_len = colon - label;
> +
> +   while (scan < output->buf + output->len) {
> +   const char *next;
> +   scan = memmem(scan, output->buf + output->len - scan,
> + label, label_len);
> +   if (!scan)
> +   return 0;
> +   if (scan == output->buf || scan[-1] == '\n')

Does scan[-1] work for the first line?

> +   break;
> +   next = strchr(scan + label_len, '\n');
> +   if (!next)
> +   return 0;
> +   scan = next + 1;
> +   }
> +
> +   /*
> +* scan points at a line that begins with the label we are
> +* looking for.  Does it match?
> +*/
> +   expect_len = strlen(expect->string);
> +
> +   if (output->buf + output->len <= scan + expect_len)
> +   return 0; /* value not long enough */
> +   if (memcmp(scan, expect->string, expect_len))
> +   return 0; /* does not match */
> +
> +   return (scan + expect_len < output->buf + output->len &&
> +   scan[expect_len] == '\n');
> +}
> +
> +static int show_expected(struct string_list *list, struct strbuf *output)
> +{
> +   struct string_list_item *expect;
> +   int found_mismatch = 0;
> +
> +   for_each_string_list_item(expect, list) {
> +   if (!match_line(expect, output))
> +   found_mismatch = 1;
> +   }
> +   return found_mismatch;
> +}
> +
>  int main(int argc, char **argv)
>  {
> const char *prefix = "prefix/";
> @@ -87,6 +144,8 @@ int main(int argc, char **argv)
> OPT__VERBOSE(, "be verbose"),
> OPT__DRY_RUN(_run, "dry run"),
> OPT__QUIET(, "be quiet"),
> +   OPT_STRING_LIST(0, "expect", , "string",
> +   "expected output in the variable dump"),
> OPT_END(),
> };
> int i;
> @@ -117,7 +176,10 @@ int main(int argc, char **argv)
> for (i = 0; i < argc; i++)
> strbuf_addf(, "arg %02d: %s\n", i, argv[i]);
>
> -   printf("%s", output.buf);
> -
> -   return 0;
> +   if (expect.nr)
> +   return show_expected(, );

On a philosophical level this patch series is adding a
trailing "|grep $X" for the 

[PATCH 3/3] test-parse-options: --expect= option to simplify tests

[Junio C Hamano]

Existing tests in t0040 follow a rather verbose pattern:

cat >expect <<\EOF
boolean: 0
integer: 0
magnitude: 0
timestamp: 0
string: (not set)
abbrev: 7
verbose: 0
quiet: 3
dry run: no
file: (not set)
EOF

test_expect_success 'multiple quiet levels' '
test-parse-options -q -q -q >output 2>output.err &&
test_must_be_empty output.err &&
test_cmp expect output
'

But the only thing this test cares about is if "quiet: 3" is in the
output.  We should be able to write the above 18 lines with just
four lines, like this:

test_expect_success 'multiple quiet levels' '
test-parse-options --expect="quiet: 3" -q -q -q
'

Teach the new --expect= option to test-parse-options helper.

Signed-off-by: Junio C Hamano 
---
 t/t0040-parse-options.sh |  1 +
 test-parse-options.c | 68 +---
 2 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index dbaee55..d678fbf 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -45,6 +45,7 @@ Standard options
 -v, --verbose be verbose
 -n, --dry-run dry run
 -q, --quiet   be quiet
+--expect  expected output in the variable dump
 
 EOF
 
diff --git a/test-parse-options.c b/test-parse-options.c
index 3db4332..010f3b2 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -14,6 +14,7 @@ static char *string = NULL;
 static char *file = NULL;
 static int ambiguous;
 static struct string_list list;
+static struct string_list expect;
 
 static struct {
int called;
@@ -40,6 +41,62 @@ static int number_callback(const struct option *opt, const 
char *arg, int unset)
return 0;
 }
 
+/*
+ * See if expect->string ("label: value") has a line in output that
+ * begins with "label:", and if the line in output matches it.
+ */
+static int match_line(struct string_list_item *expect, struct strbuf *output)
+{
+   const char *label = expect->string;
+   const char *colon = strchr(label, ':');
+   const char *scan = output->buf;
+   size_t label_len, expect_len;
+
+   if (!colon)
+   die("Malformed --expect value: %s", label);
+   label_len = colon - label;
+
+   while (scan < output->buf + output->len) {
+   const char *next;
+   scan = memmem(scan, output->buf + output->len - scan,
+ label, label_len);
+   if (!scan)
+   return 0;
+   if (scan == output->buf || scan[-1] == '\n')
+   break;
+   next = strchr(scan + label_len, '\n');
+   if (!next)
+   return 0;
+   scan = next + 1;
+   }
+
+   /*
+* scan points at a line that begins with the label we are
+* looking for.  Does it match?
+*/
+   expect_len = strlen(expect->string);
+
+   if (output->buf + output->len <= scan + expect_len)
+   return 0; /* value not long enough */
+   if (memcmp(scan, expect->string, expect_len))
+   return 0; /* does not match */
+
+   return (scan + expect_len < output->buf + output->len &&
+   scan[expect_len] == '\n');
+}
+
+static int show_expected(struct string_list *list, struct strbuf *output)
+{
+   struct string_list_item *expect;
+   int found_mismatch = 0;
+
+   for_each_string_list_item(expect, list) {
+   if (!match_line(expect, output))
+   found_mismatch = 1;
+   }
+   return found_mismatch;
+}
+
 int main(int argc, char **argv)
 {
const char *prefix = "prefix/";
@@ -87,6 +144,8 @@ int main(int argc, char **argv)
OPT__VERBOSE(, "be verbose"),
OPT__DRY_RUN(_run, "dry run"),
OPT__QUIET(, "be quiet"),
+   OPT_STRING_LIST(0, "expect", , "string",
+   "expected output in the variable dump"),
OPT_END(),
};
int i;
@@ -117,7 +176,10 @@ int main(int argc, char **argv)
for (i = 0; i < argc; i++)
strbuf_addf(, "arg %02d: %s\n", i, argv[i]);
 
-   printf("%s", output.buf);
-
-   return 0;
+   if (expect.nr)
+   return show_expected(, );
+   else {
+   printf("%s", output.buf);
+   return 0;
+   }
 }
-- 
2.8.2-505-gdbd0e1d

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html