Re: [PATCH] test-lib-functions: fix test_subcommand_inexact

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 24, 2022, 15:42 UTC
Message-ID: <72c54461-8af7-29fc-04da-f435adee9bbe@github.com>
In-Reply-To: <xmqqfsn8p8nr.fsf@gitster.g>

On 3/23/2022 7:10 PM, Junio C Hamano wrote:

Show 24 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
> 
>> On Wed, Mar 23, 2022 at 10:55:37AM -0400, Derrick Stolee wrote:
>>>> So, perhaps #3 ;-)?
>>>
>>> I'll default to #3 (do nothing), but if this shows up again
>>> I'll plan on adding a comment to the helper to be clear on
>>> how "inexact" the helper really is.
>>
>> I wonder if we could sidestep the whole issue with
>> test_subcommand_inexact by testing this behavior by looking at the
>> contents of the packs themselves.
>>
>> If we have a kept pack, and then add some new objects, and run "git
>> repack --write-midx -adb", the new pack should not contain any of the
>> objects found in the old (kept) pack. And that's the case after this
>> patch, but was broken before it.
> 
> Sounds quite sensible.
> 
> Instead of saying "we are happy as long as we internally run this
> command, as that _should_ give us the desired outcome", we check the
> resulting packs ourselves, and we do not really care how the
> "repack" command gave us that desired outcome.

Sounds good. It's all about a balance: using test_subcommand[_inexact] gives us a way to check "Did we trigger this other command that we trust works correctly from other tests?" without the more complicated work of doing a full post-condition check. It's a bit more of a unit- level check than most Git tests.

The full post-condition check requires more test code, but that's not really a problem. The problem comes in if that test is now too rigid to future changes in that subcommand. What if the post-conditions change in a subtle way because of the subcommand does something differently, but in a way that is not of importance to the top command?

In this specific case, the test name says that it "packs non-kept objects", so we can do more here to validate that post-condition that we care about.

As I'm looking at Taylor's test case example, the one thing I notice is that there is only one pack-file before the repack. It would be good to have a non-kept packfile get repacked in the process, not just the loose objects added by the test_commit. I'll take a look at what can be done here.

Thanks, -Stolee