Re: Missing file in 2.23 (p5302-pack-index.subtests)?

[Jeff King <peff@peff.net>]

On Mon, Aug 26, 2019 at 10:58:11PM -0400, Theodore Y. Ts'o wrote:

> On Mon, Aug 26, 2019 at 10:27:00PM -0400, Jeff King wrote:
> > > cannot open test-results/p5302-pack-index.subtests: No such file or directory at ./aggregate.perl line 153.
> > 
> > Implies that we're trying to _write_ to it, and that the problem is that
> > test-results doesn't exist. That should be set up by this part of
> > perf-lib:
> 
> Hmm....   test-results does exist after the failure:

Ah, never mind. I was thinking it was coming from this line in perf-lib:

   echo "$test_count" >>"$perf_results_dir"/$base.subtests

But it it is pretty clear from this line of your output:

> cannot open test-results/p5302-pack-index.subtests: No such file or directory at ./aggregate.perl line 153.

...that this is the aggregate script that is run afterwards complaining.
So for whatever reason, the actual p5302 script is exiting early, before
it writes anything into the subtests file (from the line above).

That subtests write is done in test_wrapper_(), which is only triggered
for actual timing tests, not for setup tests (like the repack step that
starts this script). So the plausible sequence of events here is:

  1. The first "test_expect_success repack" test fails for some reason.
     Try running "./p5302-pack-index -v -i -x" to see more output.

  2. After the initial failure, the script exits totally rather than
     continue. That's due to this line in perf-lib, which acts as if
     "-i" is always set:

       # Performance tests should never fail.  If they do, stop immediately
       immediate=t

     That makes sense, since any timings we do after a setup step fails
     would invalidate the result. And if it's not the _first_ such step
     that fails, we'll just have a truncated subtests file, and some
     timings will be missing. But when the first one fails, there's no
     file at all.

     We could probably leave a more consistent state in that case. E.g.,
     something like this:

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index b58a43ea43..cfac2c3cbe 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -48,7 +48,7 @@ export MODERN_GIT
 perf_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
 test -n "$GIT_PERF_SUBSECTION" && perf_results_dir="$perf_results_dir/$GIT_PERF_SUBSECTION"
 mkdir -p "$perf_results_dir"
-rm -f "$perf_results_dir"/$(basename "$0" .sh).subtests
+>"$perf_results_dir"/$(basename "$0" .sh).subtests
 
 die_if_build_dir_not_repo () {
 	if ! ( cd "$TEST_DIRECTORY/.." &&


     which would just quietly omit the rest of p5302. It is unfortunate
     that you wouldn't get some note in the output saying "hey, we
     didn't run some tests!".

     A lot of this has to do with the hackish way that the list of tests
     is generated. If you do something like:

       ./run HEAD^ HEAD p5302-pack-index.sh

     and the tests fail in HEAD^ but not HEAD, you'll get a nice table
     listing all of the tests, with a nil field for each one that didn't
     run in HEAD^.

     But that's because the second run, on HEAD, actually generated a
     correct p5302-pack-index.subtests file, that actually mentioned the
     tests! If the order were reversed, you'd get an empty list.

     So I think in the long run, it would be nice to have some way of
     generating the list that's more robust to failures. But I suspect
     doing that is going to be hard; a lot of this is rooted in the fact
     that there's no data structure with the set of tests, but literally
     an executable shell script that decides what to run.

So short answer: use "-v" to figure out why the repack is failing, and
the rest is just Git's perf suite being a bit hacky.

-Peff