Re: [PATCH v2 09/27] userdiff tests: match full hunk headers

Johannes Sixt [off-list ref] writes:

quoted

  t/t4018-diff-funcname.sh                      |  7 +++---
  t/t4018/README                                | 22 +++++++++----------
  t/t4018/README.ctx                            |  1 +
  t/t4018/bash-arithmetic-function.ctx          |  1 +
  t/t4018/bash-bashism-style-compact.ctx        |  1 +
  [...and so on...]

This is what I meant by "without burdening test writers with lots of
subtleties".

I'm not a friend of this change :-(

I think you are going overboard with required test precision. To have
useful tests for userdiff patterns that demonstrate its features, 
authors should write *many* tests. The right balance should be on the
coverage of userdiff pattern features, not on the subtle details of
each and everyone of it. Requiring that many additional context files
makes it *really hard* to comply.

Yeah, the first time I saw the t4018 test framework appeared in my
tree, I truly appreciated its simplicity, how the test input file is
self-documenting and self-contained, with the clever use of "RIGHT",
"broken" and "ChangeMe" magic tokens, admired the cleverness of the
approach, and wished I was clever enough to invent that pattern to
apply to other tests myself.

A little new for each and every test for the miniscule gain of
checking which part of the function header line is extracted feels a
bit too much noise and rubs my sense of aesthetics, spoiled by the
existing t4018 tests, the wrong way.

This is a rough sketch of a different approach aiming for the same.
I converted only a few files, but I hope that this is enough to
illustrate the idea.

 t/t4018-diff-funcname.sh         | 17 ++++++++++++++---
 t/t4018/README                   |  9 ++++++---
 t/t4018/bash-arithmetic-function |  3 +++
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git c/t/t4018-diff-funcname.sh w/t/t4018-diff-funcname.sh
index 9675bc17db..dd79c99fc5 100755
--- c/t/t4018-diff-funcname.sh
+++ w/t/t4018-diff-funcname.sh

@@ -107,10 +107,21 @@ do
 	else
 		result=success
 	fi
-	test_expect_$result "hunk header: $i" "
+
+	test_expect_$result "hunk header: $i" '
+		HEAD=$(sed -n \
+			-e "s/^.*HEADER.*|\(.*\)right\(.*\)|.*/ \1RIGHT\2/p" "$i") &&
+
 		git diff -U1 $i >actual &&
-		grep '@@ .* @@.*RIGHT' actual
-	"
+
+		sed -ne "s/^@@[^@]*@@//p" actual |
+		if test -n "$HEAD"
+		then
+			grep -F "$HEAD"
+		else
+			grep "^.*RIGHT"
+		fi
+	'
 done
 
 test_done

diff --git c/t/t4018/README w/t/t4018/README
index 283e01cca1..bc3c11e083 100644
--- c/t/t4018/README
+++ w/t/t4018/README

@@ -10,9 +10,12 @@ The text that must appear in the hunk header must contain the word
 To mark a test case that highlights a malfunction, insert the word
 BROKEN in all lower-case somewhere in the file.
 
-This text is a bit twisted and out of order, but it is itself a
-test case for the default hunk header pattern. Know what you are doing
-if you change it.
+This text is a bit twisted and out of order, but it is itself a test
+case for the default hunk header pattern. Know what you are doing if
+you change it.  You can optionally force exactly what should be on
+the hunk header line by enclosing the expect text by vertical bars
+(but downcasing the word right in it) and place it on a line with
+HEADER on it, like |How to write right test cases|.
 
 BTW, this tests that the head line goes to the hunk header, not the line
 of equal signs.

diff --git c/t/t4018/bash-arithmetic-function w/t/t4018/bash-arithmetic-function
index c0b276cb50..935f18d96d 100644
--- c/t/t4018/bash-arithmetic-function
+++ w/t/t4018/bash-arithmetic-function

@@ -2,3 +2,6 @@ RIGHT() ((
 
     ChangeMe = "$x" + "$y"
 ))
+
+
+# HEADER |right()|

On Tue, Feb 16 2021, Johannes Sixt wrote:

Am 16.02.21 um 19:32 schrieb Junio C Hamano:

quoted

Johannes Sixt [off-list ref] writes:

quoted

quoted

   t/t4018-diff-funcname.sh                      |  7 +++---
   t/t4018/README                                | 22 +++++++++----------
   t/t4018/README.ctx                            |  1 +
   t/t4018/bash-arithmetic-function.ctx          |  1 +
   t/t4018/bash-bashism-style-compact.ctx        |  1 +
   [...and so on...]

This is what I meant by "without burdening test writers with lots of
subtleties".

I'm not a friend of this change :-(

I think you are going overboard with required test precision. To have
useful tests for userdiff patterns that demonstrate its features,
authors should write *many* tests. The right balance should be on the
coverage of userdiff pattern features, not on the subtle details of
each and everyone of it. Requiring that many additional context files
makes it *really hard* to comply.

Yeah, the first time I saw the t4018 test framework appeared in my
tree, I truly appreciated its simplicity, how the test input file is
self-documenting and self-contained, with the clever use of "RIGHT",
"broken" and "ChangeMe" magic tokens, admired the cleverness of the
approach, and wished I was clever enough to invent that pattern to
apply to other tests myself.
A little new for each and every test for the miniscule gain of
checking which part of the function header line is extracted feels a
bit too much noise and rubs my sense of aesthetics, spoiled by the
existing t4018 tests, the wrong way.
This is a rough sketch of a different approach aiming for the same.
I converted only a few files, but I hope that this is enough to
illustrate the idea.
  t/t4018-diff-funcname.sh         | 17 ++++++++++++++---
  t/t4018/README                   |  9 ++++++---
  t/t4018/bash-arithmetic-function |  3 +++
  3 files changed, 23 insertions(+), 6 deletions(-)

diff --git c/t/t4018-diff-funcname.sh w/t/t4018-diff-funcname.sh
index 9675bc17db..dd79c99fc5 100755
--- c/t/t4018-diff-funcname.sh
+++ w/t/t4018-diff-funcname.sh

@@ -107,10 +107,21 @@ do
  	else
  		result=success
  	fi
-	test_expect_$result "hunk header: $i" "
+
+	test_expect_$result "hunk header: $i" '
+		HEAD=$(sed -n \
+			-e "s/^.*HEADER.*|\(.*\)right\(.*\)|.*/ \1RIGHT\2/p" "$i") &&
+
  		git diff -U1 $i >actual &&
-		grep '@@ .* @@.*RIGHT' actual
-	"
+
+		sed -ne "s/^@@[^@]*@@//p" actual |
+		if test -n "$HEAD"
+		then
+			grep -F "$HEAD"
+		else
+			grep "^.*RIGHT"
+		fi
+	'
  done
    test_done

quoted

diff --git c/t/t4018/bash-arithmetic-function w/t/t4018/bash-arithmetic-function
index c0b276cb50..935f18d96d 100644
--- c/t/t4018/bash-arithmetic-function
+++ w/t/t4018/bash-arithmetic-function

@@ -2,3 +2,6 @@ RIGHT() ((
        ChangeMe = "$x" + "$y"
  ))
+
+
+# HEADER |right()|

Clever! Opt-in for those who desire precise tests.

Tests aren't only for testing a subjective "good enough" in the
estimation of the author of the code in question, but also for others
who later touch the same area and want to avoid regressions.

Which is why I think it's an anti-pattern to use "grep SOME-SUBSTR" in
lieu of test_cmp if we can easily do the latter.

So e.g. in this case part of my motivation is that this is one of the
things I want to look at porting to some general PCREv2 powered backend
regex matching library once some of the pickaxe work I have pending
lands.

I'm very interested in whether such a port subtly breaks existing
semantics, and it's not useful if such regressions are hidden because
someone who wrote a userdiff rule didn't think they needed to care about
exact matching for their own purposes.

I find this notion that patch authors who we'd expect to hack userdiff.c
and somehow understand the arcane rules of the list form of
diff.<func>.xfunction (which wasn't even documented until this series),
and to carefully read t/t4018/README to see how their test is parsed,
would be discouraged by some pretty plain shell syntax to pass in two
strings to be implausible.

Whenever I've tried to hack up userdiff.c rules my first stumbling block
has been that if you fail the previous test gave you no output, because
it used grep instead of test_cmp.

So you'd need to monkeypatch it to dump the value, or know about "-d"
and trash directory inspection. So much for sparing prospective authors
from the intricacies of our sh-based tests.

The old test format also forces you to try to mix real examples of a
programming language with our need to shove an all-caps "RIGHT"
identifier somewhere on the hunk line.

In languages like Go, Perl or Elisp such an identifier is somewhere
between so strongly discouraged that you'd never see one in the wild, or
in others even a syntax error.

It seems much simpler to just ask the author to paste in their snippet,
test it, and then then paste what they got on the @@ line as another
parameter to their test.

Am 16.02.21 um 19:32 schrieb Junio C Hamano:

Clever! Opt-in for those who desire precise tests.

-- Hannes