Re: [PATCH v3 1/5] t3200: improve test style
- From
Kristoffer Haugsbakk <code@khaugsbakk.name>
- Date
- Mar 5, 2024, 10:27 UTC
- Message-ID
- <166d2baa-933c-44f8-b6fb-94c8bce63a86@app.fastmail.com>
- In-Reply-To
- <xmqqplw9lbav.fsf@gitster.g>
On Tue, Mar 5, 2024, at 02:25, Junio C Hamano wrote:
> Especially the change to use "-\EOF" to make them align better > caused too many tests to be touched, but overall the result may have > become much easier to follow. Good job.
I reckon that this can be worth doing now as long as no other topics in `next` or `seen` happen to touch the same code. What do you think? I can evict hunks if they happen to overlap with other in-flight topics.
Show 19 quoted lines
>> -mv .git/config .git/config-saved >> - >> test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' ' >> + test_when_finished mv .git/config-saved .git/config && >> + mv .git/config .git/config-saved && >> git branch -m q q2 && >> git branch -m q2 q >> ' >> >> -mv .git/config-saved .git/config > > The above is a truly valuable clean-up. > > But I am not really sure if the paritcular condition is worth > testing in the first place these days. No configuration file means > we cannot even read the repository format version, and working under > such a condition is quite a bad promise that we would rather not to > having to keep. But that is an entirely different topic from what > this patch is doing.
Okay. I could undo this change and remove the test in its own commit?
Show 13 quoted lines
> >> -git config branch.s/s.dummy Hello >> - >> -test_expect_success 'git branch -m s/s s should work when s/t is deleted' ' >> +test_expect_success '(setup) git branch -m s/s s should work when s/t is deleted' ' >> + git config branch.s/s.dummy Hello && >> git branch --create-reflog s/s && >> git reflog exists refs/heads/s/s && >> git branch --create-reflog s/t && > > I do not know if the change of the title is warranted. It is doing > its own test, not just setup. It may be merely donw for the side > effect of making the step unskippable, but still ....
Sure, I’ll remove `(setup)`. The test name suggests that the test depends on the previous one in any case.