Derrick Stolee <stolee@xxxxxxxxx> writes: > On 8/21/2025 4:00 AM, Patrick Steinhardt wrote: >> On Wed, Aug 20, 2025 at 06:39:54PM +0000, Derrick Stolee via GitGitGadget wrote: >>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh >>> index 611755cc139b..1998d9bf291c 100755 >>> --- a/t/t7700-repack.sh >>> +++ b/t/t7700-repack.sh >>> @@ -838,4 +838,47 @@ test_expect_success '-n overrides repack.updateServerInfo=true' ' >> >> Tiny nit: I would've probably squashed this patch into the second patch, >> as we usually don't use the add-failing-test-and-then-fix-it-later >> dance. On the other hand though it gives some nice context, so I >> ultimately don't mind it all that much. So please feel free to ignore >> this nit. > > I'm probably the person who is always asking folks to create a test > that either fails or demonstrates the "before" behavior before making > the actual change that updates the case. This allows the ability to > "test the test" by checking it in place to confirm that it is indeed > failing. > Using test_expect_failure allows us to avoid breaking bisect. Yes, you can develop that way, but on the reviewing and receiving end, the second patch that shows only the change from expect_failure to expect_success pushes the more important "what behaviour was this thing testing?" out of the hunk context, if you split them into two. If we really wanted to verify the claim that without the fix this was broken and we have a test to demonstrate a failure on the receiving end, we can "checkout" paths touched by that commit outside t/ from HEAD^ to build to see how the system behaved without the code change just fine, so such a split does not buy us much. Unless there is a strong reason not to, please always present such test in the same patch as the code change to fix that breakage. Thanks.
