Re: [PATCH v2 12/27] userdiff tests: rewrite hunk header test infrastructure

Am 15.02.21 um 21:06 schrieb Ævar Arnfjörð Bjarmason:

On Mon, Feb 15 2021, Johannes Sixt wrote:

quoted

Am 15.02.21 um 16:44 schrieb Ævar Arnfjörð Bjarmason:

quoted

+test_diff_funcname 'bash: bashism style compact' \
+	8<<\EOF_HUNK 9<<\EOF_TEST
+function RIGHT {
+EOF_HUNK
+function RIGHT {
+    function InvalidSyntax{
+        :
+        echo 'ChangeMe'
+    }
+}
+EOF_TEST
+
+test_diff_funcname 'bash: bashism style function' \
+	8<<\EOF_HUNK 9<<\EOF_TEST
+function RIGHT {
+EOF_HUNK
+function RIGHT {
+    :
+    echo 'ChangeMe'
+}
+EOF_TEST
[...]

That is not my dream of "simple". But I'm not a userdiff author
anymore, so...

I don't know, yet, where this is heading to what the advantage is. At
any rate,[...]

I originally started writing this because I noticed I could break the
userdiff.c patterns and still have all tests pass, i.e. if you screw up
the capture grouping you can go from:
     
     @@ -2,3 +2,3 @@ function        RIGHT   (       )       {
     @@ -2,3 +2,3 @@ RIGHT   (       )

to:
     
     @@ -2,3 +2,3 @@ function        RIGHT   (       )       {
     @@ -2,3 +2,3 @@          RIGHT  (       )
     
And we wouldn't care because we just "grep 'RIGHT'". In this case we
really care about the difference between "^[ \t]*(.*)$" and a broken
"^([ \t]*.*)$" so not having the tests structurally hide the difference
makes sense.

But why care? The second version is not wrong. If it's not pretty, the pattern author will notice soon enough. I regard avoiding the regression you cite less important than the simplicity of adding new test cases.

quoted

[...] "trivial to add new tests" was also the case when each test case
was in its own file[...]

"trivial to add new tests by adding new optional parameters to the
function". I.e. aside from the s/grep/test_cmp/ change in 09/27 the
existing tests were OK if you wanted to test exactly what they expected,
and no more.

I did not understand what you meant with "new optional parameters" and I still do not and why it is desirable.

I think it just makes sense to have a test helper function instead and
little bit of boilerplate, as seen e.g. in 14/27 and later in the series
we can add new test modes and set per-test config without needing the
top-level dispatch loop to be aware of it.

quoted

[...] Without the boilerplate!

I realize that's a matter of taste, i.e. when to come up with some
custom format v.s. writng a function.

FWIW as someone who didn't author the format I've come across it N times
over the years and each time ended up being more confused than when
reading any custom test function we have.

For those you can usually just look at the definition/arguments, whereas
this always required a careful read of t4018-diff-funcname.sh.

I also find it easier to have one ~160 line file in my editor than ~150
lines spread over 15 files, as in the recent addition of bash support in
2ff6c34612 (userdiff: support Bash, 2020-10-22).

I wouldn't mind having all test cases in a single file. But then it should be one file in the language that is being tested, not shell script with foreign text between the lines. Adding test cases should be easy for authors; they should not have to be proficient in shell scripting (and knwo about 130 lines of CodingGuidelines).

-- Hannes