Re: [PATCH 4/5] Add tests for git-log --merges=show|hide|only

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Mar 22, 2015 at 2:28 PM, Koosha Khajehmoogahi <koosha@posteo.de> wrote:
> Subject: Add tests for git-log --merges=show|hide|only

Drop capitalization, mention area you're touching, followed by colon,
followed by short summary:

    t4202-log: add --merges= tests

More below.

> Signed-off-by: Koosha Khajehmoogahi <koosha@posteo.de>
> ---
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 5f2b290..ab6f371 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -428,6 +428,147 @@ cat > expect <<\EOF
>  * initial
>  EOF
>
> +cat > only_merges <<EOF
> +Merge tag 'reach'
> +Merge tags 'octopus-a' and 'octopus-b'
> +Merge branch 'tangle'
> +Merge branch 'side' (early part) into tangle
> +Merge branch 'master' (early part) into tangle
> +Merge branch 'side'
> +EOF

Current custom is to place this sort of setup code in its own test
rather than having it at top-level. So, the above and below 'cat's
should all go in a test named "setup --merges=" (or something better).
Prefixing EOF with '-' allows you to indent the here-doc content and
EOF so that it reads nicely inside a test. Also prefix EOF with '\' to
indicate that you do not expect interpolation within the here-doc.
Torsten already mentioned to drop the space following '>'. Finally,
within the setup test, chain them all together with &&. So:

    cat >only_merges <<-\EOF &&
    ...
    EOF

More below.

> +cat > only_commits <<EOF
> +seventh
> +octopus-b
> +octopus-a
> +reach
> +tangle-a
> +side-2
> +side-1
> +Second
> +sixth
> +fifth
> +fourth
> +third
> +second
> +initial
> +EOF
> +
> +cat > both_commits_merges <<EOF
> +Merge tag 'reach'
> +Merge tags 'octopus-a' and 'octopus-b'
> +seventh
> +octopus-b
> +octopus-a
> +reach
> +Merge branch 'tangle'
> +Merge branch 'side' (early part) into tangle
> +Merge branch 'master' (early part) into tangle
> +tangle-a
> +Merge branch 'side'
> +side-2
> +side-1
> +Second
> +sixth
> +fifth
> +fourth
> +third
> +second
> +initial
> +EOF

t4202 already does this sort of fragile hard-coding of "expected"
output, so this isn't a particularly strong objection, but it would be
more robust if you could create these "expected" lists dynamically by
issuing some git command other than the one you're testing. (That
could also be considered fodder for a follow-on patch if you don't
consider such a change worthwhile at this time.)

> +
> +test_expect_success 'log with config log.merges=show' '
> +       git config log.merges show &&
> +    git log --pretty=tformat:%s >actual &&

Torsten already mentioned botched indentation. Use (8-character wide)
tab for indentation everywhere.

> +       test_cmp both_commits_merges actual &&
> +    git config --unset log.merges

If the test_cmp fails (because the expected and actual output
differed), then the git-config --unset will never be invoked. To
ensure that the config setting is undone when the test finishes
(whether successful or not), use test_config().

The other option is to ensure that each test sets or clears log.merges
as appropriate at the start of the test, and then not bother about
resetting it. The shortcoming of this approach, however, is that any
tests added following these new tests won't necessarily know that
log.merges is set or that they may need to clear it. Consequently,
test_config() at the start of each of these tests is probably the
better approach.

More below.

> +'
> +
> +test_expect_success 'log with config log.merges=only' '
> +       git config log.merges only &&
> +    git log --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log with config log.merges=hide' '
> +       git config log.merges hide &&
> +    git log --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log with config log.merges=show with git log --no-merges' '
> +       git config log.merges show &&
> +    git log --no-merges --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log with config log.merges=hide with git log ---merges' '
> +       git config log.merges hide &&
> +    git log --merges --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=show' '

For these tests which expect log.merges to be unset, it would be more
robust, and the intent clearer, if you actually ensured that it was
unset. test_unconfig() at the start of the test would be the
appropriate way to unset the config. It would also make the tests more
self-contained, in case they are ever re-ordered.

More below.

> +    git log --merges=show --pretty=tformat:%s >actual &&
> +       test_cmp both_commits_merges actual
> +'
> +
> +test_expect_success 'log --merges=only' '
> +    git log --merges=only --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual
> +'
> +
> +test_expect_success 'log --merges=hide' '
> +    git log --merges=hide --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual
> +'
> +
> +test_expect_success 'log --merges=show with config log.merges=hide' '
> +       git config log.merges hide &&
> +    git log --merges=show --pretty=tformat:%s >actual &&
> +       test_cmp both_commits_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=show with config log.merges=only' '
> +       git config log.merges only &&
> +    git log --merges=show --pretty=tformat:%s >actual &&
> +       test_cmp both_commits_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=only with config log.merges=show' '
> +       git config log.merges show &&
> +    git log --merges=only --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=only with config log.merges=hide' '
> +       git config log.merges hide &&
> +    git log --merges=only --pretty=tformat:%s >actual &&
> +       test_cmp only_merges actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=hide with config log.merges=show' '
> +       git config log.merges show &&
> +    git log --merges=hide --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'
> +
> +test_expect_success 'log --merges=hide with config log.merges=only' '
> +       git config log.merges only &&
> +    git log --merges=hide --pretty=tformat:%s >actual &&
> +       test_cmp only_commits actual &&
> +    git config --unset log.merges
> +'

There's an awful lot of repetition in the implementation these new
tests. Such repetition invites errors when composing the tests and
makes them difficult to review (since it's easy to be sloppy in the
review as well). The situation could be improved by introducing a
helper function which performs the tests, controlled by a few
parameters. For instance, the helper could accept three arguments: (1)
optional value of --merges=, (2) optional value of log.merges, (3)
name of "expected" file. It also suggests that you should normalize
the names of the "expected" files ("expect_show", "expect_only",
"expect_hide") so that the 3rd argument can be short and sweet. Let's
say that the helper is test_log_merges(), then you would invoke it as:

    test_log_merges "" "" show
    test_log_merges "" show show
    test_log_merges "" only only
    ...
    test_log_merges hide "" hide
    ...
    test_log_merges only hide only

for all combination of --merges= and log.merges.

> +
>  test_expect_success 'log --graph with merge' '
>         git log --graph --date-order --pretty=tformat:%s |
>                 sed "s/ *\$//" >actual &&
> --
> 2.3.3.263.g095251d.dirty