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

Am 28.02.21 um 17:07 schrieb Ævar Arnfjörð Bjarmason:

On Sun, Feb 28 2021, Johannes Sixt wrote:

quoted

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.

I missed that the descriptions are gone now that many test cases are
shoved into a single file when the simple framework is used. That is
indeed a disadvantage. But please keep in mind that code is more often
read than written. *If* we have to grow a new framework, then it must
not suffer from unreadability.

 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.

As I said, the config in perl.sh is only used for a multi-line pattern
test. That is dispensable as there is already a similar test with custom
patterns. And the tests for custom patterns can be moved to the main
test file entirely.

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.

That sounds like an improvement.

-- Hannes