From: Felipe Contreras <hidden> Date: 2021-06-09 17:05:39
The test_atexit unit test relies on the specific location of the
generated files.
When TEST_OUTPUT_DIRECTORY is unset, _run_sub_test_lib_test_common sets
it to pwd, which is two levels under the pwd of the parent unit test,
and the parent can find the generated files just fine.
But when TEST_OUTPUT_DIRECTORY is set, it's stored in GIT-BUILD-OPTIONS,
and even though _run_sub_test_lib_test_common correctly overrides it,
when the child script is run, it sources GIT-BUILD-OPTIONS, and
TEST_OUTPUT_DIRECTORY is overridden.
Effectively both the parent and child scripts output to the same
directory.
make TEST_OUTPUT_DIRECTORY=/tmp/foobar GIT-BUILD-OPTIONS &&
make -C t t0000-basic.sh
We could simply revert 2d14e13c56 (test output: respect
$TEST_OUTPUT_DIRECTORY, 2013-04-29), but presumably it was done for some
reason.
On the other hand we could follow the alternate path suggested in
6883047071 (t0000: set TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28):
pass the --root parameter to the child scripts.
The alternate solution works, so let's do that instead.
Presumably this was broken since 900721e15c (test-lib: introduce
'test_atexit', 2019-03-13).
Cc: John Keeping <redacted>
Cc: Jeff King <redacted>
Cc: Johannes Schindelin <redacted>
Signed-off-by: Felipe Contreras <redacted>
---
t/t0000-basic.sh | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2021-06-13 04:42:44
On Wed, Jun 09, 2021 at 12:05:20PM -0500, Felipe Contreras wrote:
The test_atexit unit test relies on the specific location of the
generated files.
When TEST_OUTPUT_DIRECTORY is unset, _run_sub_test_lib_test_common sets
it to pwd, which is two levels under the pwd of the parent unit test,
and the parent can find the generated files just fine.
But when TEST_OUTPUT_DIRECTORY is set, it's stored in GIT-BUILD-OPTIONS,
and even though _run_sub_test_lib_test_common correctly overrides it,
when the child script is run, it sources GIT-BUILD-OPTIONS, and
TEST_OUTPUT_DIRECTORY is overridden.
Effectively both the parent and child scripts output to the same
directory.
make TEST_OUTPUT_DIRECTORY=/tmp/foobar GIT-BUILD-OPTIONS &&
make -C t t0000-basic.sh
I agree things are broken when TEST_OUTPUT_DIRECTORY is set. We pollute
/tmp/foobar in that case with trash directories, as well as its
test-results/ directory with subtest results (mostly "counts" files).
On the other hand we could follow the alternate path suggested in
6883047071 (t0000: set TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28):
pass the --root parameter to the child scripts.
The alternate solution works, so let's do that instead.
Unfortunately, this isn't a complete solution. Using --root fixes the
trash directories, but we still pollute test-results. No tests in t0000
rely on that, but it's still the wrong thing to be doing.
That's true before your patch, as well, though it does make things
slightly worse when TEST_OUTPUT_DIRECTORY isn't set (before in that case
everything worked perfectly, and now it pollutes test-results/, too).
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).
Alternatively, it would be nice if GIT-BUILD-OPTIONS didn't override the
environment. That would fix this problem, prevent similar ones in the
future, and I think would do the right thing if a caller decided to do
something like:
PERL_PATH=/some/other/perl ./t1234-foo.sh
We might be able to write GIT-BUILD-OPTIONS to use ${foo:=}
default-value expansion, but I'm not sure if there are any non-shell
readers of the file (I don't see any, but I feel like we've run afoul of
this before). Something like this might work:
@@ -54,7 +54,10 @@ thenecho>&2'error: GIT-BUILD-OPTIONS missing (has Git been built?).'exit1fi+# let our env take precedence over build options+current_env=$(set) ."$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS+eval"$current_env"exportPERL_PATHSHELL_PATH# Disallow the use of abbreviated options in the test suite by default
but I think more fixes are required for $TEST_OUTPUT_DIRECTORY, since we
set it so early in the script (before we read GIT-BUILD-OPTIONS, so at
that point we have no idea if it came from the environment or our
default settings).
-Peff
From: Felipe Contreras <hidden> Date: 2021-06-13 15:45:30
Jeff King wrote:
On Wed, Jun 09, 2021 at 12:05:20PM -0500, Felipe Contreras wrote:
quoted
The test_atexit unit test relies on the specific location of the
generated files.
When TEST_OUTPUT_DIRECTORY is unset, _run_sub_test_lib_test_common sets
it to pwd, which is two levels under the pwd of the parent unit test,
and the parent can find the generated files just fine.
But when TEST_OUTPUT_DIRECTORY is set, it's stored in GIT-BUILD-OPTIONS,
and even though _run_sub_test_lib_test_common correctly overrides it,
when the child script is run, it sources GIT-BUILD-OPTIONS, and
TEST_OUTPUT_DIRECTORY is overridden.
Effectively both the parent and child scripts output to the same
directory.
make TEST_OUTPUT_DIRECTORY=/tmp/foobar GIT-BUILD-OPTIONS &&
make -C t t0000-basic.sh
I agree things are broken when TEST_OUTPUT_DIRECTORY is set. We pollute
/tmp/foobar in that case with trash directories, as well as its
test-results/ directory with subtest results (mostly "counts" files).
quoted
On the other hand we could follow the alternate path suggested in
6883047071 (t0000: set TEST_OUTPUT_DIRECTORY for sub-tests, 2013-12-28):
pass the --root parameter to the child scripts.
The alternate solution works, so let's do that instead.
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.
Using --root fixes the
trash directories, but we still pollute test-results. No tests in t0000
rely on that, but it's still the wrong thing to be doing.
That's true before your patch, as well, though it does make things
slightly worse when TEST_OUTPUT_DIRECTORY isn't set (before in that case
everything worked perfectly, and now it pollutes test-results/, too).
True.
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.
I think whomever usses that script *and* TEST_OUTPUT_DIRECTORY, can
simply do `TEST_OUTPUT_DIRECTORY=foo valgrind/analyze.sh`.
So maybe:
@@ -1,8 +1,5 @@#!/bin/sh-# Get TEST_OUTPUT_DIRECTORY from GIT-BUILD-OPTIONS if it's there...-."$(dirname"$0")/../../GIT-BUILD-OPTIONS"-# ... otherwise set it to the default value. :${TEST_OUTPUT_DIRECTORY=$(dirname"$0")/..}output=
From: Jeff King <hidden> Date: 2021-06-14 07:44:02
On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:
quoted
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, or even making things worse (as
this patch did).
quoted
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.
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.
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.
-Peff
From: Felipe Contreras <hidden> Date: 2021-06-14 08:40:15
Jeff King wrote:
On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:
quoted
quoted
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.
quoted
quoted
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.
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.
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
On Sun, Jun 13, 2021 at 10:44:10AM -0500, Felipe Contreras wrote:
quoted
quoted
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)).
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".
quoted
quoted
quoted
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.
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?:)
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.
@@ -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_unsetHARNESS_ACTIVE&&++# These tests should emit no metrics or output that+# would normally go in the "test-results" directory.+TEST_NO_RESULTS_OUTPUT=1&&+exportTEST_NO_RESULTS_OUTPUT&&+cd"$name"&&write_script"$name.sh""$TEST_SHELL_PATH"<<-EOF&&test_description='$descr(runinsubtest-lib)
From: Jeff King <hidden> Date: 2021-06-14 14:25:45
On Mon, Jun 14, 2021 at 11:33:12AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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).
@@ -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_unsetHARNESS_ACTIVE&&++# These tests should emit no metrics or output that+# would normally go in the "test-results" directory.+TEST_NO_RESULTS_OUTPUT=1&&+exportTEST_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).
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...
quoted hunk
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
On Mon, Jun 14, 2021 at 11:33:12AM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
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).
@@ -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_unsetHARNESS_ACTIVE&&++# These tests should emit no metrics or output that+# would normally go in the "test-results" directory.+TEST_NO_RESULTS_OUTPUT=1&&+exportTEST_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).
Yeah I don't know and haven't dug into who wants all this combination of
GIT-BUILD-OPTIONS, passing things in the env, or passing things as
paramaters to make (sometimes under the same names).
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).
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...
quoted
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:
If we edit every single current callsite instead of setting it to
something you can't write to then we're setting ourselves up for subtle
bugss when someone uses $TEST_RESULTS_DIR for something else.
mkdir: cannot create directory ‘/dev/null’: Not a directory
or similar.
Yeah that error sucks, but nobody will see it unless they're hacking on
the guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.
In any case, I'll let Felipe decide what, if anything, to do with this
:)
From: Jeff King <hidden> Date: 2021-06-15 11:10:17
On Mon, Jun 14, 2021 at 06:55:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
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).
Odd.
I think it's just that the aggregation script was never meant to signal
to "make". In a regular "make test" (not using prove), each individual
test script is a dependency than can fail on its own. That means a
failure of any of them will signal "make" to fail the overall operation.
Interestingly it means we will not run the "aggregate-results" at all in
that case, so we would not give the nice output (you can run "make
aggregate-results" yourself, though; it doesn't depend on the tests
running itself, but assumes you've already run them).
So arguably we could do something like this:
but it makes "make aggregate-results" after the fact a little noisier. I
dunno. I don't really care that much about the output from this form of
the tests at all, since the "prove" output is _so_ much better, and I'd
highly recommend anybody use it instead.
The only thing preventing me from suggesting we get rid of the old
make-driven approach entirely is that there are probably platforms that
run the tests where "prove" is not available. And as long as it is not
generating wrong results (e.g., returning 0 when a test has failed), it
is doing that job OK.
quoted
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).
Yeah I don't know and haven't dug into who wants all this combination of
GIT-BUILD-OPTIONS, passing things in the env, or passing things as
paramaters to make (sometimes under the same names).
To be clear, I doubt it's all that important. It would occasionally be
less surprising when trying to override the environment while running a
test script manually (which is after all, basically the same thing
that's happening here, just driven by a script). But if it's hard to do,
I'm OK with an easier solution provided it doesn't regress any other
cases.
quoted
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).
Yeah, but only in the trivial wrapper function, you can still write the
test script and check the output yourself.
Sort of. You can avoid its setting of TEST_NO_RESULTS_OUTPUT, but we're
still left without a way to reliably override TEST_OUTPUT_DIRECTORY.
Again, I'm OK punting on that for now if there are no such tests.
quoted
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:
If we edit every single current callsite instead of setting it to
something you can't write to then we're setting ourselves up for subtle
bugss when someone uses $TEST_RESULTS_DIR for something else.
I was thinking we'd still set it to /dev/null as a belt-and-suspenders
(so the worst case is just an ugly error message). But...
quoted
mkdir: cannot create directory ‘/dev/null’: Not a directory
or similar.
Yeah that error sucks, but nobody will see it unless they're hacking on
the guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.
I think that's a good point. You're unlikely to stumble into
accidentally using TEST_NO_RESULTS_OUTPUT, so it might not be worth
caring about too much.
-Peff
I think it's just that the aggregation script was never meant to signal
to "make". In a regular "make test" (not using prove), each individual
test script is a dependency than can fail on its own. That means a
failure of any of them will signal "make" to fail the overall operation.
Only one failure can trigger FTBFS when make test, right?
--
An old man doll... just what I always wanted! - Clara
From: Jeff King <hidden> Date: 2021-06-15 11:23:56
On Tue, Jun 15, 2021 at 06:21:00PM +0700, Bagas Sanjaya wrote:
On 15/06/21 18.10, Jeff King wrote:
quoted
I think it's just that the aggregation script was never meant to signal
to "make". In a regular "make test" (not using prove), each individual
test script is a dependency than can fail on its own. That means a
failure of any of them will signal "make" to fail the overall operation.
Only one failure can trigger FTBFS when make test, right?
I'm not sure I understand what your question means. I know that "FTBFS"
means "failed to build from source", but that is not a term we use in
the Git project. So if you are asking whether "make test" will exit
non-zero if a single test fails, then yes.
If you're asking whether a Debian package would consider that an FTBFS,
then probably yes, but it depends on whether they run the tests.
-Peff
From: Felipe Contreras <hidden> Date: 2021-06-15 17:45:43
Æ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
From: Felipe Contreras <hidden> Date: 2021-06-15 18:09:54
Jeff King wrote:
On Mon, Jun 14, 2021 at 06:55:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
quoted
quoted
mkdir: cannot create directory ‘/dev/null’: Not a directory
or similar.
Yeah that error sucks, but nobody will see it unless they're hacking on
the guts of this $TEST_NO_RESULTS_OUTPUT, and I think it beats being fragile.
I think that's a good point. You're unlikely to stumble into
accidentally using TEST_NO_RESULTS_OUTPUT, so it might not be worth
caring about too much.
But check_sub_test_lib_test() cares:
test_must_be_empty err
'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
--
Felipe Contreras