Re: [PATCH v3 00/35] 20210215154427.32693-1-avarab@gmail.com

On Sun, Feb 28 2021, Johannes Sixt wrote:

Am 27.02.21 um 08:47 schrieb Johannes Sixt:

quoted

Am 24.02.21 um 20:50 schrieb Ævar Arnfjörð Bjarmason:

quoted

Addresses feedback on v2. Since Junio & Johannes expressed a desire to
keep the existing test scheme in t4018/* it's still there, but it's
also possible to add *.sh tests in that directory to use the more
familiar test framework used elsewhere in the test suite.

The tests added here make use of it to e.g. supply custom -U<n>
arguments, set config before the tests etc.

I also improved that existing test support so you can have N tests in
one file with (mostly) the existing test syntax. See the "userdiff
tests: add a test with multiple tests in a LANG file" patch.

I've read through all patches and had a comment here and there. I like a
lot that we can now put more than one test into a single file.

However, I do not like the shell script version of tests, because the
syntax is so hard to read. Also, it looks to me that they are only
needed for a few tests that could just as well be coded as one-off tests
outside the framework.

I've now pulled avar/t4018-diff-hunk-header-regex-tests-3 from your
github repo and will check again if I missed some cruicial points.

I've now looked through the patch series again.

I appreciate that you dug through the history and discovered and fixed a
few deficiencies and loose ends. The way to throw all test cases for a
language driver into a single file I like a lot, too. The way to specify
expected results is manageable (modulo the dependency on the test
number, t4018, that Junio mentioned).

Yes, maybe just "HEADER:" or something would be better. I was trying to
come up with something guaranteed not to conflict with the language in
question. Maybe:

    => HEADER      = 
    => description = 

But I do not see the need for the framework provided by the new
test_diff_funcname. At the end of the series, it is only used by Perl
and custom driver tests. (I discount the new ADA and Ruby tests as they
can easily migrated to the simple test scheme.) But then the Perl tests
do not do anything special, either. The multi-line pattern test is just
a nice add-on but not strictly needed. In the end, the Perl test is just
as straight-forward as all others.

The benefit now is:

 1. Unlike the new plain-text "all test cases for a language driver" in
    the same file mechanism you can have test descriptions. The
    "description" in the golang one is just for show, you won't get
    anything informative from test-lib.sh when your test fails.

 2. I think having #1 beats not having test descriptions at all, or
     having to shove a description like the Ruby:

    "picks first "class/module/def" before changed context"

    into something that would make all the FS's we have to support
    happy.

 3. A test in the new perl.sh one sets config. I think in both that case
    and custom.sh it's more readable to carry such config setting with
    the test, rather than at a distance in the main setup of t4018-*.sh.

That being said I'd like to improve the syntax a bit, in particular
instead of having a wrapper for test_expect_success I think it makes
sense just to have the test call test_expect_success, and then provide a
couple of helper functions.