Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY

On Mon, Jun 14, 2021 at 06:55:03PM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

Indeed. With Felipe's original patch, the "test" target (but not
"prove") in t/Makefile will report, whether you set
TEST_OUTPUT_DIRECTORY or not:

  failed test(s): t1234 t2345

  fixed   0
  success 23243
  failed  2
  broken  221
  total   23647

though curiously it doesn't exit non-zero back to make (usually we'd
also see the failures from the individual make targets, and barf there).

Odd.

I think it's just that the aggregation script was never meant to signal
to "make". In a regular "make test" (not using prove), each individual
test script is a dependency than can fail on its own. That means a
failure of any of them will signal "make" to fail the overall operation.

Interestingly it means we will not run the "aggregate-results" at all in
that case, so we would not give the nice output (you can run "make
aggregate-results" yourself, though; it doesn't depend on the tests
running itself, but assumes you've already run them).

So arguably we could do something like this:

diff --git a/t/aggregate-results.sh b/t/aggregate-results.sh
index 7913e206ed..6198b2ef6b 100755
--- a/t/aggregate-results.sh
+++ b/t/aggregate-results.sh

@@ -44,3 +44,5 @@ printf "%-8s%d\n" success $success
 printf "%-8s%d\n" failed $failed
 printf "%-8s%d\n" broken $broken
 printf "%-8s%d\n" total $total
+
+test -z "$failed_tests"

but it makes "make aggregate-results" after the fact a little noisier. I
dunno. I don't really care that much about the output from this form of
the tests at all, since the "prove" output is _so_ much better, and I'd
highly recommend anybody use it instead.

The only thing preventing me from suggesting we get rid of the old
make-driven approach entirely is that there are probably platforms that
run the tests where "prove" is not available. And as long as it is not
generating wrong results (e.g., returning 0 when a test has failed), it
is doing that job OK.

quoted

I'm OK with this general approach. I do think it would be nice if we let
the environment supersede the on-disk GIT-BUILD-OPTIONS, which IMHO is
the real root of the problem (and possibly others), but that may be more
challenging to get right (I posted a patch earlier, but it does rely on
stuffing all of "set" into a variable, which makes me concerned some
less-able shells may complain).

Yeah I don't know and haven't dug into who wants all this combination of
GIT-BUILD-OPTIONS, passing things in the env, or passing things as
paramaters to make (sometimes under the same names).

To be clear, I doubt it's all that important. It would occasionally be
less surprising when trying to override the environment while running a
test script manually (which is after all, basically the same thing
that's happening here, just driven by a script). But if it's hard to do,
I'm OK with an easier solution provided it doesn't regress any other
cases.

quoted

It also means that t0000 can't test the results output (since we don't
write it), but I assume we don't do that now (I didn't actually try
running with your patch).

Yeah, but only in the trivial wrapper function, you can still write the
test script and check the output yourself.

Sort of. You can avoid its setting of TEST_NO_RESULTS_OUTPUT, but we're
still left without a way to reliably override TEST_OUTPUT_DIRECTORY.
Again, I'm OK punting on that for now if there are no such tests.

quoted

We do look at them elsewhere, though (in --tee as you noted, and I think
for --stress). I'd prefer to notice the "no results" flag explicitly
there and report something sensible, rather than getting:

If we edit every single current callsite instead of setting it to
something you can't write to then we're setting ourselves up for subtle
bugss when someone uses $TEST_RESULTS_DIR for something else.

I was thinking we'd still set it to /dev/null as a belt-and-suspenders
(so the worst case is just an ugly error message). But...

quoted

  mkdir: cannot create directory ‘/dev/null’: Not a directory

or similar.

Yeah that error sucks, but nobody will see it unless they're hacking on
the guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.

I think that's a good point. You're unlikely to stumble into
accidentally using TEST_NO_RESULTS_OUTPUT, so it might not be worth
caring about too much.

-Peff