Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments

From: Eric Sunshine <sunshine@sunshineco.com>
To: Dragan Simic <dsimic@manjaro.org>
Cc: git@vger.kernel.org, gitster@pobox.com, rsbecker@nexbridge.com,
	 github@seichter.de
Subject: Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments
Date: Fri, 15 Mar 2024 16:29:34 -0400	[thread overview]
Message-ID: <CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com> (raw)
In-Reply-To: <CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com>

On Fri, Mar 15, 2024 at 3:39 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> These days we try to place all test-related code inside a
> test_expect_success() context rather than having it standalone. In
> this case, since the file being created is (presumably) shared by
> multiple tests in this script, you may want to add a new test which
> performs this setup step.
>
> Taking all the above into account, perhaps:
>
>     test_expect_success 'setup whitespace' '
>         q_to_tab >.git/config <<-\EOF
>         [section]
>         solid = rock
>         sparse = bigQblue
>         ...
>         EOF
>
> Same comments apply to rest of patch.

To be clear, this case is special because the file being created is
shared by multiple tests, so it deserves being placed in its own
test_expect_success() invocation.

For the remaining cases where you're doing some set-up outside of
test_expect_success(), just move the set-up code into the
corresponding test_expect_success() invocation. For instance, rather
than:

    echo 'big               blue' > expect

    test_expect_success 'internal whitespace' '
        git config --get section.sparse > output &&
        test_cmp expect output
    '

do this:

    test_expect_success 'internal whitespace' '
        echo 'bigQblue' | q_to_tab >expect
        git config --get section.sparse >actual &&
        test_cmp expect actual
    '

(I changed "output" to "actual" above since the names "expect" and
"actual" are common in the tests.)