Re: [PATCH v2 03/21] t/test-lib-functions.sh: generalize test_cmp_rev

> Hi,
>
> On 04/15/2016 10:00 PM, Junio C Hamano wrote:
>> Stephan Beyer <s-beyer@gmx.net> writes:
>> 
>>> test_cmp_rev() took exactly two parameters, the expected revision
>>> and the revision to test. This commit generalizes this function
>>> such that it takes any number of at least two revisions: the
>>> expected one and a list of actual ones. The function returns true
>>> if and only if at least one actual revision coincides with the
>>> expected revision.
>> 
>> There may be cases where you want to find the expected one among
>> various things you actually have (which is what the above talks
>> about; it is like "list-what-I-actually-got | grep what-i-want"),
>> but an equally useful use case would be "I would get only one
>> outcome from test, I anticipate one of these things, all of which is
>> OK, but I cannot dictate which one of them should come out" (it is
>> like "list-what-I-can-accept | grep what-I-actually-got").
>
> I see that these are strictly speaking (slightly) different semantics
> but in the end it boils down to be the same, or am I missing anything?
>
>> I am not enthused by the new test that implements the "match one
>> against multi" check only in one way among these possible two to
>> squat on a very generic name, test_cmp_rev.
>> 
>> The above _may_ appear a non-issue until you realize one thing that
>> is there to help those who debug the tests, which is ...
>> 
>>> While at it, the side effect of generating two (temporary) files
>>> is removed.
>> 
>> That is not strictly a side effect.  test_cmp allows you to see what
>> was expected and what you actually had when the test failed (we
>> always compare expect with actual and not the other way around, so
>> that "diff -u expect actual" would show how the actual behaviour
>> diverted from our expectation in a natural way).
>
> I was referring to *generating the files* as a side effect. I did not
> even think about the fact that "diff" in the original code does not only
> return an exit code but that it also generates output that can be used
> as "helpful diagnostic information" (referring to Eric Sunshine's mail
> here). I was not aware that the Git tests should -- besides testing --
> already include "tools" for easier debugging in case of a failure... So
> dropping this information was not intentional.
>
>> Something with the semantics of these two:
>> 
>> 	test_revs_have_expected () {
>>         	expect=$1
>> 		shift
>> 		git rev-parse "$@" | grep -e "$expect" >/dev/null && return
>> 		echo >&2 "The expected '$1' is not found in:"
>>                 printf >&2 " '%s'\n", "$@"
>>                 return 1
>> 	}
>> 
>> 	test_rev_among_expected () {
>> 		actual=$1
>>                 shift
>> 		git rev-parse "$@" | grep -e "$actual" >/dev/null && return
>> 		echo >&2 "'$1' is not among expected ones:"
>>                 printf >&2 " '%s'\n", "$@"
>>                 return 1
>> 	}
>> 
>> might be more appropriate.
>
> Ah! That's what I meant above. The code is copy&paste besides variable
> naming and the output "title". Such code duplication for the sake of
> "easier debugging" in case of a failure?
>
> Also I wonder if test authors in the future would really know *which*
> one is the right one to use.