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

[Stefan Beller <sbeller@google.com>]

On Thu, May 5, 2016 at 7:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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 <string>     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(&label, arg, colon - arg);
> +       item = string_list_insert(expect, strbuf_detach(&label, 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(&buf, 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(&buf);

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 <options>",
>                 NULL
>         };
> +       struct string_list expect = STRING_LIST_INIT_NODUP;
>         struct option options[] = {
>                 OPT_BOOL(0, "yes", &boolean, "get a boolean"),
>                 OPT_BOOL('D', "no-doubt", &boolean, "begins with 'no-'"),
> @@ -86,34 +142,38 @@ int main(int argc, char **argv)
>                 OPT__VERBOSE(&verbose, "be verbose"),
>                 OPT__DRY_RUN(&dry_run, "dry run"),
>                 OPT__QUIET(&quiet, "be quiet"),
> +               OPT_CALLBACK(0, "expect", &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(&expect, &ret, "Callback: \"%s\", %d",
> +                    (arg ? arg : "not set"), unset);
>         }
> -       printf("boolean: %d\n", boolean);
> -       printf("integer: %d\n", integer);
> -       printf("magnitude: %lu\n", magnitude);
> -       printf("timestamp: %lu\n", timestamp);
> -       printf("string: %s\n", string ? string : "(not set)");
> -       printf("abbrev: %d\n", abbrev);
> -       printf("verbose: %d\n", verbose);
> -       printf("quiet: %d\n", quiet);
> -       printf("dry run: %s\n", dry_run ? "yes" : "no");
> -       printf("file: %s\n", file ? file : "(not set)");
> +       show(&expect, &ret, "boolean: %d", boolean);
> +       show(&expect, &ret, "integer: %d", integer);
> +       show(&expect, &ret, "magnitude: %lu", magnitude);
> +       show(&expect, &ret, "timestamp: %lu", timestamp);
> +       show(&expect, &ret, "string: %s", string ? string : "(not set)");
> +       show(&expect, &ret, "abbrev: %d", abbrev);
> +       show(&expect, &ret, "verbose: %d", verbose);
> +       show(&expect, &ret, "quiet: %d", quiet);
> +       show(&expect, &ret, "dry run: %s", dry_run ? "yes" : "no");
> +       show(&expect, &ret, "file: %s", file ? file : "(not set)");
>
>         for (i = 0; i < list.nr; i++)
> -               printf("list: %s\n", list.items[i].string);
> +               show(&expect, &ret, "list: %s", list.items[i].string);
>
>         for (i = 0; i < argc; i++)
> -               printf("arg %02d: %s\n", i, argv[i]);
> +               show(&expect, &ret, "arg %02d: %s", i, argv[i]);
>
>         return 0;

    return ret; ? Otherwise `ret` is unused.

>  }