On 2024-03-15 21:29, Eric Sunshine wrote:
On Fri, Mar 15, 2024 at 3:39 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
