Re: [PATCH v3 2/2] Added tests for reset -

From: Sudhanshu Shekhar <sudshekhar02@gmail.com>
Date: Mar 10, 2015, 17:52 UTC
Message-ID: <CAODo60p_2deueOS5nYfHPTMas=GcHR4n8dNs8HJt-ZR+2SifzA@mail.gmail.com>
In-Reply-To: <vpqa8zlx9td.fsf@anie.imag.fr>

Hi,

On Tue, Mar 10, 2015 at 6:56 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 7 quoted lines

> Sudhanshu Shekhar <sudshekhar02@gmail.com> writes:
>
>> +test_expect_success 'reset - while having file named - and no previous branch' '
>
> I like having the expected behavior in the test name too. e.g. add
> "fails" at the end of the sentence.
>

Sure. I will update this in the next patch.

Show 13 quoted lines

>> +test_expect_success 'reset - in the presence of file named - with previous branch' '
>> +     echo "Unstaged changes after reset:" >expect &&
>> +     echo "M -" >>expect &&
>> +     echo "M 1" >>expect &&
>
> Here and elsewhere: why not
>
>         cat >expect <<-EOF
>         Unstaged changes after reset:
>         M -
>         M 1
>
> ?

I was confused whether to use cat or echo. I thought using cat will disrupt the indentation as the EOF needs to be on a single line. This is why I chose echo. Please let me know your thoughts on this.

Show 5 quoted lines

>> +     rm -rf no_previous &&
>
> That would be best done in a test_when_finished, so that the directory
> is removed regardless of whether the test failed before this line or
> not.

Thanks for pointing this out. I will update this accordingly.

>
> --
> Matthieu Moy
> http://www-verimag.imag.fr/~moy/

Regards, Sudhanshu