Re: [PATCH 03/15] t: use test_decode_color rather than literal ANSI codes

From: Stefan Beller <sbeller@google.com>
Date: Jul 13, 2017, 18:40 UTC
Message-ID: <CAGZ79kaE7wjFR-=DiPU5xwknS1nxDHR5+S6+UGPFTF0uNBA3dQ@mail.gmail.com>
In-Reply-To: <20170713145841.4win5tbx4efwjsa2@sigill.intra.peff.net>

On Thu, Jul 13, 2017 at 7:58 AM, Jeff King <peff@peff.net> wrote:

> I really only need t6300 and t6006 converted to build on for the rest of
> the series. But t4207 was easy to do. t4026 still uses raw codes, but
> converting it would be a pretty big job, so I punted.
>

I think it is good to have raw codes in at least one place to test that raw codes work, but then again it could be specific test calling out that this is tested.

Show 7 quoted lines

> @@ -59,7 +54,8 @@ EOF
>  # to this test since it does not contain any decoration, hence --first-parent
>  test_expect_success 'Commit Decorations Colored Correctly' '
>         git log --first-parent --abbrev=10 --all --decorate --oneline --color=always |
> -       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" >out &&
> +       sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" |
> +       test_decode_color >out &&

Just some thoughts:

This extension of the pipe-chain is not making it worse as gits exit code is already hidden.

The sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" is sort of funny, because I would have expected it to break in the future with e.g. the sha1 to longer hash conversion. But as we specify --abbrev=10, this seems future proof. In an ideal world this would be encapsulated in a function (c.f. t/diff-lib.sh).

Show 10 quoted lines

> @@ -61,8 +61,9 @@ test_format () {
>  # Feed to --format to provide predictable colored sequences.
>  AUTO_COLOR='%C(auto,red)foo%C(auto,reset)'
>  has_color () {
> -       printf '\033[31mfoo\033[m\n' >expect &&
> -       test_cmp expect "$1"
> +       test_decode_color <"$1" >decoded &&
> +       echo "<RED>foo<RESET>" >expect &&
> +       test_cmp expect decoded
>  }

Thanks for removing hard coded colors :)