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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 28, 2021, 16:07 UTC
Message-ID: <875z2ckwfl.fsf@evledraar.gmail.com>
In-Reply-To: <3519e86e-d9eb-a9e0-ed28-72d32271d2bf@kdbg.org>

On Sun, Feb 28 2021, Johannes Sixt wrote:

Show 33 quoted lines

> Am 27.02.21 um 08:47 schrieb Johannes Sixt:
>> Am 24.02.21 um 20:50 schrieb Ævar Arnfjörð Bjarmason:
>>> 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 =

Show 7 quoted lines

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