From: Dragan Simic Date: Fri, 15 Mar 2024 21:42:11 GMT Subject: Re: [PATCH 3/4] t1300: add more tests for whitespace and inline comments Message-ID: In-Reply-To: On 2024-03-15 21:29, Eric Sunshine wrote: > On Fri, Mar 15, 2024 at 3:39 PM Eric Sunshine > 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.