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

[Junio C Hamano <gitster@pobox.com>]

Anthony Wang <anthonywang513@gmail.com> writes:

>> >     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.

>> > > +             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?

> 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?").