Hi,
On 04/15/2016 10:00 PM, Junio C Hamano wrote:
> Stephan Beyer <s-beyer@xxxxxxx> 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. In the end, either one of these two will
just be used arbitrarily (and I wouldn't even think there's anything bad
about it, because it *is* the same logic). I think this distinction is
like having two algorithms doing the same but with a different name.
Something you do NOT really want.
So I'd vote against a distinction of these two "cases", but I have no
problem with re-adding "debug" information (like you did in your code
examples).
Thanks!
Stephan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html