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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-03-15 21:29, Eric Sunshine wrote:
> 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.)

This looks nice, thanks again.  It keeps the expected results and
the test execution in a single "block", making it a bit easier to
keep track of different tests and their expected results.