Re: [PATCH v4 4/4] t0302: test credential-store support for XDG_CONFIG_HOME

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 23, 2015, 01:56 UTC
Message-ID: <CAPig+cShTKEVovMRWWPVn7cVMUF7J+ZT_eK4OfOGgn2H4H0TdA@mail.gmail.com>
In-Reply-To: <CACRoPnTXH+HDD42H2ca7b+OPF15v2TxM+y+NRRi2CS_Xg3tGEw@mail.gmail.com>

On Sat, Mar 21, 2015 at 1:46 AM, Paul Tan <pyokagan@gmail.com> wrote:

Show 25 quoted lines

> On Thu, Mar 19, 2015 at 3:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Mar 18, 2015 at 3:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
>>> index f61b40c..5b0a666 100755
>>> --- a/t/t0302-credential-store.sh
>>> +++ b/t/t0302-credential-store.sh
>>> @@ -6,4 +6,115 @@ test_description='credential-store tests'
>>> +test_expect_success 'when xdg credentials file does not exist, only write to ~/.git-credentials; do not create xdg file' '
>>
>> These test descriptions are quite long, often mirroring in prose what
>> the test body already says more concisely. I generally try to keep
>> descriptions short while still being descriptive enough to give a
>> general idea about what is being tested. I've rewritten a few test
>> descriptions (below) to be very short, in fact probably too terse; but
>> perhaps better descriptions would lie somewhere between the two
>> extremes. First example, for this test:
>>
>>     "!xdg: >.git-credentials !xdg"
>>
> I will make the test descriptions shorter. However, I don't think the
> test descriptions need to be so terse such that a separate key is
> required. e.g. I will shorten the above to "when xdg file does not
> exist, it's not created.", or even terser: "when xdg file not exists,
> it's not created.". I don't think symbols should be used, as many
> other test descriptions do not use them.

Your updated test descriptions all sound fine.

Show 10 quoted lines

>>> +XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME && helper_test store
>>> +unset XDG_CONFIG_HOME
>>
>> It's hard to spot the "helper_test store" at the end of line. I'd
>> place it on a line by itself so that it is easy to see that it is
>> wrapped by the setting and unsetting of the environment variable.
>
> Thanks, will fix. Although now it looks weird that the "export" is the
> only one with a continuation on a single line, so I split all of them
> so that they each have their own line.

An &&-chain outside of a test is not meaningful in this case, so I meant either this:

    XDG_CONFIG_HOME="$HOME/xdg"
    export XDG_CONFIG_HOME
    helper_test store
    unset XDG_CONFIG_HOME

or, slightly more compact (using && just to combine the assignment and export on one line):

    XDG_CONFIG_HOME="$HOME/xdg" && export XDG_CONFIG_HOME
    helper_test store
    unset XDG_CONFIG_HOME