Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 30, 2016, 20:31 UTC
Message-ID: <CAPig+cQyHu1J=FYOtgsmi3ghuN7YyjNgAz-VgO06isfrS+kUSg@mail.gmail.com>
In-Reply-To: <CA+DCAeQPr2vxvm6MKiOLpDtmpC2d=RcvYhuFeimSn+xX2TAvtQ@mail.gmail.com>

On Wed, Mar 30, 2016 at 3:00 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

Show 26 quoted lines

> On Wed, Mar 30, 2016 at 2:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> With the exception of the missing --rebase argument, this is exactly
>> the same code as in test_rebase_autostash(), right? Rather than
>> repeating this code yet again, it might be nice to augment that
>> function to accept a (possibly) optional argument controlling whether
>> --rebase is used.
>
> Thanks for the idea. I have come up with something like this:
>
> test_pull () {
>         git reset --hard before-rebase &&
>         echo dirty >new_file &&
>         git add new_file &&
>         git pull $@ . copy &&
>         test_cmp_rev HEAD^ copy &&
>         test "$(cat new_file)" = dirty &&
>         test "$(cat file)" = "modified again"
> }
>
> test_pull_fail () {
>         git reset --hard before-rebase &&
>         echo dirty >new_file &&
>         git add new_file &&
>         test_must_fail git pull $@ . copy 2>err &&
>         test_i18ngrep "uncommitted changes." err
> }

Considering that these are specifically testing behavior related to autostashing, it might make sense to have "autostash" in the function names, but that's a very minor point.

Show 14 quoted lines

> test_expect_success 'pull --rebase succeeds with dirty working
> directory and rebase.autostash set' '
>         test_config rebase.autostash true &&
>         test_pull --rebase
> '
> [...]
> test_expect_success 'pull --no-autostash & pull.rebase=true' '
>         test_config pull.rebase true &&
>         test_pull_fail --no-autostash
> '
>
> I'm sorry if this is bit difficult to digest without diff output. I
> just wanted to
> know if the above mention functions looks suitable to you.

This is exactly what I had in mind for simplifying the tests, and it's perfectly easy to read in this form (a diff would be worse for this illustration).

One other possibility would be to make this all table-driven by collecting all of the above state information into a table and then feeding that into a function (either as its argument list or via stdin). For instance:

    test_autostash <<\-EOF
    ok,--rebase,rebase.autostash=true
    ok,--rebase --autostash,rebase.autostash=true
    ok,--rebase --autostash,rebase.autostash=false
    ok,--rebase --autostash,rebase.autostash=
    err,--rebase --no-autostash,rebase.autostash=true
    err,--rebase --no-autostash,rebase.autostash=false
    err,--rebase --no-autostash,rebase.autostash=
    ok,--autostash,pull.rebase=true
    err,--no-autostash,pull.rebase=true
   EOF

The function would loop over the input, split each line apart by setting IFS=, and then run the test based upon the state information. "ok" means autostash is expected to succeed, and err means it is expected to fail. The function would want to specially recognize the "foo.bar=" in the last argument in order to invoke test_unconfig() rather than test_config().

However, this may be a case of diminishing returns. The tests as you illustrated them are sufficiently simple and easy to grok that the table-driven approach may not add much value (aside from making it easier to see at a glance if any cases were omitted).