Re: [PATCH 3/3] t-ctype: do one test per class and char

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 4, 2024, 10:00 UTC
Message-ID: <CAP8UFD2M2+x-pRX2nzCXbLW=nKcW4_RWc9qua5q-fU8QbGu1oA@mail.gmail.com>
In-Reply-To: <bd48f19b-0600-4e64-835b-98d3a97bb7f2@web.de>

On Sat, Mar 2, 2024 at 11:00 PM René Scharfe <l.s.r@web.de> wrote:

>
> Am 27.02.24 um 11:04 schrieb Christian Couder:

Show 19 quoted lines

> > Yeah, I know about TAP harnesses like prove, but the most
> > straightforward way to run the unit tests is still `make unit-tests`
> > in the t/ directory. Also when you add or change some tests, it's a
> > good idea to run `make unit-tests` to see what the output is, so you
> > still have to see that output quite often when you work on tests and
> > going through 3598 of mostly useless output instead of just 14 isn't
> > nice.
>
> I was starting the programs from t/unit-tests/bin/ individually because
> I didn't know 'make unit-tests' exists.  This is much nicer, thank you!
> Especially after adding 'DEFAULT_UNIT_TEST_TARGET = unit-tests-prove' to
> config.mak to complement the 'DEFAULT_TEST_TARGET = prove' I added long
> ago.  It would be even nicer if the former was the default when the
> latter is set.
>
> As unit tests are added, their output is surely going to grow to
> multiple screens with or without prove, no?  So someone writing or
> debugging tests will still go back to starting then individually
> eventually.

When t-ctype will be run individually from t/unit-tests/bin/, for example when adding or debugging ctype tests, it would still be better if there are only 14 lines in its output rather than 3598.

Show 11 quoted lines

> The size of the output in itself is not a problem, I assume, but that
> most of it is useless -- details of successful tests are uninteresting.
> A test harness can aggregate the output, but prove annoyed me when used
> with the regular tests by also aggregating error output and only showing
> the numbers of failed tests.  Finding their names involved running the
> test script again without prove.  Turns out it has an option for that.
> Added 'GIT_PROVE_OPTS = --failures' to config.mak as well, will see if
> it helps.
>
> Is it too much to ask developers to use a test harness?  Perhaps: It's
> yet another dependency and not enabled by default.

Yeah, it's a dependency, and when running CI tests, it's sometimes better and simpler to have the canonical output rather than having the output processed by a test harness.

Also if we add some verbose or immediate modes (like -v and -i with the shell test framework) or perhaps other kinds of modes (checking for memory leaks or other errors using existing tools for example), these modes might not interact nicely with test harnesses but still be useful. Requiring to always use a test harness might restrict us for no good reason.

And anyway it doesn't make sense to have meaningful messages as second arguments to the TEST() macros if we always want to use a test harness that just discards them. Either:

  - we decide that we will always use some test harness, and then we
might just want to remove that second argument and yeah we can have
thousands of tests output lines from a single binary, or
  - we acknowledge that we don't always want to use a test harness,
and then we want a relatively short and meaningful output from a
single binary.

> What's the right level of aggregation and how do we achieve it?
> Grouping by class is natural and follows the test definition.  We
> could stop after patch 2.  Dunno.

I am Ok with just removing this patch like you did in v2. Thanks.