Re: [PATCH] test: fix for TEST_OUTPUT_DIRECTORY

Ævar Arnfjörð Bjarmason wrote:

On Mon, Jun 14 2021, Felipe Contreras wrote:

quoted

Jeff King wrote:

quoted

quoted

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.

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".

Yeah, that's not good, but I wouldn't call it "worse".

quoted

quoted

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.

Do we need to bring Spock into this?:)

Hopefully not.

I think the following alternate/on-top patch (which you should feel free
to squash in with my SOB if you agree) solves the issue both of you are
noting.

It will barf if you run the tests under e.g. --tee with that environment
variable, but that's intentional. It's better to loudly error if we
don't have the expected test-results than to silently write in the wrong
place.

I'm not sure how that patch was supposed to work:

  'err' is not empty, it contains:
  mkdir: cannot create directory '/dev/null': File exists
  /home/felipec/dev/git.git/git/t/test-lib.sh: line 1133: /dev/null.counts: Permission denied
  not ok 5 - pretend we have a fully passing test suite
 
Does this assume the rest of the framework will be updated to properly
handle TEST_NO_RESULTS_OUTPUT?

Cheers.

-- 
Felipe Contreras