Re: [PATCH v2] t/Makefile: add a rule to re-run previously-failed tests

[Jeff King <peff@peff.net>]

On Mon, Aug 29, 2016 at 03:46:05PM +0200, Johannes Schindelin wrote:

> Note that we need to be careful to inspect only the *newest* entries in
> test-results/: this directory contains files of the form
> tNNNN-<name>-<pid>.counts and is only removed wholesale when running the
> *entire* test suite, not when running individual tests. We ensure that
> with a little sed magic on `ls -t`'s output that simply skips lines
> when the file name was seen earlier.

Hmm, interesting. Your approach seems reasonable, but I have to wonder
if writing the pid in the first place is sane.

I started to write up my reasoning in this email, but realized it was
rapidly becoming the content of a commit message. So here is that
commit.

-- >8 --
Subject: [PATCH] test-lib: drop PID from test-results/*.count

Each test run generates a "count" file in t/test-results
that stores the number of successful, failed, etc tests.
If you run "t1234-foo.sh", that file is named as
"t/test-results/t1234-foo-$$.count"

The addition of the PID there is serving no purpose, and
makes analysis of the count files harder.

The presence of the PID dates back to 2d84e9f (Modify
test-lib.sh to output stats to t/test-results/*,
2008-06-08), but no reasoning is given there. Looking at the
current code, we can see that other files we write to
test-results (like *.exit and *.out) do _not_ have the PID
included. So the presence of the PID does not meaningfully
allow one to store the results from multiple runs anyway.

Moreover, anybody wishing to read the *.count files to
aggregate results has to deal with the presence of multiple
files for a given test (and figure out which one is the most
recent based on their timestamps!). The only consumer of
these files is the aggregate.sh script, which arguably gets
this wrong. If a test is run multiple times, its counts will
appear multiple times in the total (I say arguably only
because the desired semantics aren't documented anywhere,
but I have trouble seeing how this behavior could be
useful).

So let's just drop the PID, which fixes aggregate.sh, and
will make new features based around the count files easier
to write.

Note that since the count-file may already exist (when
re-running a test), we also switch the "cat" from appending
to truncating. The use of append here was pointless in the
first place, as we expected to always write to a unique file.

Signed-off-by: Jeff King <peff@peff.net>
---
The presence of the append, combined with the way aggregate.sh is
written makes me wonder if the intent was to store multiple run results
for each test in a single file (and aggregate would just report the last
one). Which _still_ makes the use of the PID wrong. But again, I don't
see much use for it.

 t/test-lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index d731d66..eada492 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -687,9 +687,9 @@ test_done () {
 		test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
 		mkdir -p "$test_results_dir"
 		base=${0##*/}
-		test_results_path="$test_results_dir/${base%.sh}-$$.counts"
+		test_results_path="$test_results_dir/${base%.sh}.counts"
 
-		cat >>"$test_results_path" <<-EOF
+		cat >"$test_results_path" <<-EOF
 		total $test_count
 		success $test_success
 		fixed $test_fixed
-- 
2.10.0.rc2.123.ga991f9e