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

Stephan Beyer [off-list ref] writes:

Hi,

On 04/15/2016 10:00 PM, Junio C Hamano wrote:

quoted

Stephan Beyer [off-list ref] writes:

quoted

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?

quoted

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

quoted

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.

quoted

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.

I saw that even you were originally confused about it ;-).

In your proposed log message, you talk about "the expected, and list
of actual ones", which can only mean "there may be multiple answers
from the command (e.g. "merge-base --all") and we only require that
one of the answers is the expected one", which is why among the two
necessary functions I listed "test_revs_have_expected" above first,
but I think most (if not all) of the invocations of the multi-match
form in your patch actually wanted "test_rev_among_expected"
variant, i.e. "there will be one answer from the command, but there
are multiple acceptable answers, all of them valid".

I do not think test authors who understands the reason why we always
say "test_cmp actual expect" and not the other way around will share
the same confusion (and now you were explained and understood why
"diff" in the original was given the expected and actual result in
that order, you no longer are confused wrt this).

And no, this is not "for the sake of easier debugging in case of a
failure".  It is about knowing what you are doing--are you going to
have multiple answers and making sure one right one appears in it,
or are you going to have one answer and allowing any one of multiple
valid ones?  These two are quite different things and it helps the
readers of the test to know which one is being used.