From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-4.2 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_LOW, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 8B34C1F5AE for ; Mon, 14 Jun 2021 14:26:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234440AbhFNO1r (ORCPT ); Mon, 14 Jun 2021 10:27:47 -0400 Received: from cloud.peff.net ([104.130.231.41]:54848 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232809AbhFNO1q (ORCPT ); Mon, 14 Jun 2021 10:27:46 -0400 Received: (qmail 2357 invoked by uid 109); 14 Jun 2021 14:25:42 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Mon, 14 Jun 2021 14:25:42 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 1906 invoked by uid 111); 14 Jun 2021 14:25:42 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Mon, 14 Jun 2021 10:25:42 -0400 Authentication-Results: peff.net; auth=none Date: Mon, 14 Jun 2021 10:25:41 -0400 From: Jeff King To: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason Cc: Felipe Contreras , git@vger.kernel.org, Junio C Hamano , Carlo Marcelo Arenas =?utf-8?B?QmVsw7Nu?= , John Keeping , Johannes Schindelin Subject: Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY Message-ID: References: <20210609170520.67014-1-felipe.contreras@gmail.com> <60c627cac29b3_41f45208a7@natae.notmuch> <60c715dd9939e_436208f3@natae.notmuch> <8735tk22if.fsf@evledraar.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8735tk22if.fsf@evledraar.gmail.com> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Mon, Jun 14, 2021 at 11:33:12AM +0200, Ævar Arnfjörð Bjarmason wrote: > > I think breaking the test suite is objectively worse than having a few > > extra files in the output directory, but to each his own. > > We've got both in-tree and out-tree things that rely on e.g. the > *.counts in that directory to have a 1=1 mapping with "real" > tests. E.g. "make aggregate-results". 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). > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > index 2c6e34b9478..29bf67d49bf 100755 > --- a/t/t0000-basic.sh > +++ b/t/t0000-basic.sh > @@ -76,6 +76,12 @@ _run_sub_test_lib_test_common () { > # this variable, so we need a stable setting under which to run > # the sub-test. > sane_unset HARNESS_ACTIVE && > + > + # These tests should emit no metrics or output that > + # would normally go in the "test-results" directory. > + TEST_NO_RESULTS_OUTPUT=1 && > + export TEST_NO_RESULTS_OUTPUT && 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). 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). > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 54938c64279..9e9696a3185 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -252,8 +252,14 @@ TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}" > TEST_NAME="$(basename "$0" .sh)" > TEST_NUMBER="${TEST_NAME%%-*}" > TEST_NUMBER="${TEST_NUMBER#t}" > -TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results" > -TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX" > +if test -n "$TEST_NO_RESULTS_OUTPUT" > +then > + TEST_RESULTS_DIR=/dev/null > + TEST_RESULTS_BASE=/dev/null > +else > + TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results" > + TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX" > +fi I wondered about this use of /dev/null, since we'd generally use this as a directory, and writing to "/dev/null/foo" is going to throw an error. But... > TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX" > test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY" > case "$TRASH_DIRECTORY" in > @@ -1124,7 +1130,7 @@ test_done () { > > finalize_junit_xml > > - if test -z "$HARNESS_ACTIVE" > + if test -z "$HARNESS_ACTIVE$TEST_NO_RESULTS_OUTPUT" > then > mkdir -p "$TEST_RESULTS_DIR" ...here we would never look at those variables at all, so it is just a sentinel that would let us know the assumption has been violated. 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: mkdir: cannot create directory ‘/dev/null’: Not a directory or similar. -Peff