Re: [GSoC] [PATCH v5 1/1] t9811: Improve test coverage and clarity

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 9, 2025, 13:29 UTC
Message-ID: <xmqqa58ptqd9.fsf@gitster.g>
In-Reply-To: <CAOSofofyfDpFqFzApwMnmYdFb4nk8DS7WwWmYKLPbe+okOE=Tw@mail.gmail.com>

Anthony Wang <anthonywang513@gmail.com> writes:

Show 5 quoted lines

>> >     The tests grep tagnames they expect to exist from "git tag"
>>
>> s/tagnames/tag names/ perhaps?
>
> How does "t9811: be more precise to check importing of tags" sound?

Excellent.

Show 9 quoted lines

>> > > +             git tag &&
>> > > +             git show-ref --verify refs/tags/TAG_F1_1 &&
>> > > +             git show-ref --verify refs/tags/TAG_F1_2 &&
>> > > +             test_must_fail git show-ref --verify refs/tags/TAG_F1_ONLY &&
>>
>> Do we still need the standalone `git tag` invocation above?
>
> The original intent of the patch was to expose the exit code of
> `git tag`.

Is it? I somehow thought that "git tag" is not what is being tested by this script. Rather, it assumed that "git tag" works perfectly well, and validated what "git p4" left in the resulting repository based on that assumption, i.e. "git tag" works perfectly well to tell us what tags are in the repository.

It is true that "git tag" to list all available tags may fail, but then catching that is outside the scope of this script no? It is even more so, since now we do not even depend on the correct operation of "git tag" anymore to validate what "git p4" did---we now use "show-ref --verify" for that, so we do not even care if "git tag" segfaults in this part of the test, no?

Show 5 quoted lines

> However, because in this case the test itself does not correctly test
> for the intended behavior, we should modify because we are already
> touching this piece of code. Is this correct? Would it then be desired
> to check the rest of the tests in this file for further oversights and
> correct them as well, or would that be overstepping boundaries?

Just like any real world problems, there unfortunately is no bright red line between "yeah these are related enough and in the same spot and it is better to clean up while we are at it" and "that is way too much for this single topic" that can be described in a textbook. A rule of thumb I personally use is to put me in the shoes of an imaginary typical Git developer with moderate competence, who hasn't seen or worked on a particular part of the system being updated. If I can easily imagine that the developer can clearly see a need for clean up (in this case, "the part of the code only tests positive results and forgets about negative check") while fixing something else (in this case, "use of 'git tag' piped to 'grep' has at least two problems, loss of exit code and false match") and the additional effort would be smaller than 10-20 minutes, I'd say it would be worth doing and anything larger would be better to leave to another day, but a lot of ingredients in that statement are very much subjective (starting from "what's the average competence level we expect from our people?").