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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, May 5, 2016 at 8:41 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, May 5, 2016 at 2:50 PM, Junio C Hamano <gitster@pobox.com> 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=<string> option to test-parse-options helper.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>> 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