Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 14, 2021, 08:39 UTC
Message-ID: <60c715dd9939e_436208f3@natae.notmuch>
In-Reply-To: <YMcIv7q1ctyJ0EZn@coredump.intra.peff.net>

Jeff King wrote:

Show 12 quoted lines

> On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:
> 
> > > Unfortunately, this isn't a complete solution.
> > 
> > Software will never be perfect.
> > 
> > We don't need to wait for a perfect solution, all we need is something
> > better than the current siuation.
> 
> Sure, but if you don't fully understand the situation (e.g., that --root
> and TEST_OUTPUT_DIRECTORY are not equivalent), then you may end up
> revisiting the incomplete fix later,

The fact that you may end up revisiting a solution is a fact for *all* changes (including 2d14e13c56 (test output: respect $TEST_OUTPUT_DIRECTORY, 2013-04-29)).

> or even making things worse (as this patch did).

I think breaking the test suite is objectively worse than having a few extra files in the output directory, but to each his own.

Show 14 quoted lines

> > > I think solving the whole issue would require a mechanism for passing
> > > TEST_OUTPUT_DIRECTORY in a way that can't be overridden (whether in an
> > > environment variable or the command-line).
> > 
> > Why do we even have TEST_OUTPUT_DIRECTORY in GIT-BUILD-OPTIONS? Looking
> > for a reason there's 2d14e13c56 (test output: respect
> > $TEST_OUTPUT_DIRECTORY, 2013-04-29), there it says it's for
> > valgrind/analyze.sh.
> > 
> > I don't know who uses that script, or how. There's no documentaion,
> > nothing on the mailing list, and nothing found on Google.
> 
> Perhaps 268fac6919 (Add a script to coalesce the valgrind outputs,
> 2009-02-04) is enlightening.

That makes it clearer.

Show 6 quoted lines

> I don't know if anybody still uses it these days, though. I suspect it's
> outlived its usefulness, in that we would typically not have any
> valgrind errors at all (so coalescing them is not that interesting).
> 
> Possibly folks investigating leak-checking via valgrind could find it
> useful, but even there I think LSan is a much better path forward.

Yeah, but even if they do run this tool, they can set TEST_OUTPUT_DIRECTORY manually.

The needs of the few should not otweight needs of the many.

Show 17 quoted lines

> > So maybe:
> > 
> > diff --git a/Makefile b/Makefile
> > index c3565fc0f8..2e25489569 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2790,9 +2790,6 @@ GIT-BUILD-OPTIONS: FORCE
> >         @echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
> >         @echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
> >         @echo X=\'$(X)\' >>$@+
> > -ifdef TEST_OUTPUT_DIRECTORY
> > -       @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
> > -endif
> 
> I don't personally have any problem with that. It does mean that "make
> t1234-foo.sh" will behave differently than "./t1234-foo.sh", but that is
> already true if you set GIT_TEST_OPTS.

Only if you haven't changed TEST_OUTPUT_DIRECTORY since the last time you ran 'make' on the top level directory.

And of course if somebody really wants their environment to be honored, that's what "make -e t1234-foo.sh" is for.

Cheers.

-- 
Felipe Contreras