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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 15, 2024, 20:29 UTC
Message-ID: <CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com>
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:

Show 17 quoted lines

> 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.)