Re: [PATCH 4/5] t/t5520: modify tests to reduce common code

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> t/t5520: modify tests to reduce common code

As this is indeed a patch, "modify" is implied. Perhaps:

    t5520: factor out common code

> There exist three groups of tests which have repetitive lines of code.
>
> Introduce two functions test_rebase_autostash() and
> test_rebase_no_autostash() to reduce the number of lines. Also introduce
> loops to futher reduce the current implementation.

This patch is doing so much that it's difficult to review for
correctness. Taking [1] into consideration, better would be to split
it into at least three patches:

1. Factor out code into test_rebase_autostash() and modify the four
tests to call it.

2. Factor out code into test_rebase_autostash_fail() and modify the
three tests to call it.

3. Fold the two "pull $i (without --rebase) is illegal" tests into a for-loop.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289434/focus=289860

> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -9,6 +9,24 @@ modify () {
> +test_rebase_no_autostash () {
> +       git reset --hard before-rebase &&
> +       echo dirty >new_file &&
> +       git add new_file &&
> +       test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> +       test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err

In the spirit of patch 3/5 and [1], you could grep for a substring
rather than the full message, but that's a minor point, not worth a
re-roll.

    test_i18ngrep "uncommitted changes" err

> +}
> @@ -256,75 +274,39 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
> +for i in true false
> +       do
> +               test_expect_success "pull --rebase --autostash & rebase.autostash=$i" '
> +                       test_config rebase.autostash $i &&
> +                       test_rebase_autostash
> +               '
> +       done

I don't care too strongly, but I'm not convinced that this for-loop is
buying you much for these two cases since each test already has been
reduced to two simple lines, and the added abstraction of the for-loop
increases cognitive load a bit.

> +for i in --autostash --no-autostash
> +       do
> +               test_expect_success "pull $i (without --rebase) is illegal" '
> +                       test_must_fail git pull $i . copy 2>actual &&
> +                       test_i18ngrep "only valid with --rebase" actual
> +               '
> +       done

You might then ask why I suggested[1] the for-loop in this case but
not for the true/false case. Even though these are also two-line
tests, they are not quite as simple as two lines down to which the
true/false tests devolve. Anyhow, this alone is not worth a re-roll.