| Message ID | 20220920201619.40972-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] t/Makefile: remove 'test-results' on 'make clean' | expand |
On Tue, Sep 20 2022, SZEDER Gábor wrote: > The 't/test-results' directory and its contents are by-products of the > test process, so 'make clean' should remove them, but, alas, this has > been broken since fee65b194d (t/Makefile: don't remove test-results in > "clean-except-prove-cache", 2022-07-28). > > The 'clean' target in 't/Makefile' was not directly responsible for > removing the 'test-results' directory, but relied on its dependency > 'clean-except-prove-cache' to do that [1]. ee65b194d broke this, > because it only removed the 'rm -r test-results' command from the > 'clean-except-prove-cache' target instead of moving it to the 'clean' > target, resulting in stray 't/test-results' directories. > > Add that missing cleanup command to 't/Makefile', and to all > sub-Makefiles touched by that commit as well. > > [1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs, > 2012-05-02) > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- Thanks, and sorry about the breakage. I've looked this over carefully & it fixes the edge-case you noted without making anything else that I could spot worse. In case it helps: Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> It's still a bit odd to have a "clean" that cleans up a thing it *might have* generated, i.e. sometimes we create & use these via a Makefile target, and sometimes by manually invoking test scripts. But any such issues far pre-date this fix (or my commit).
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Tue, Sep 20 2022, SZEDER Gábor wrote: > >> The 't/test-results' directory and its contents are by-products of the >> test process, so 'make clean' should remove them, but, alas, this has >> been broken since fee65b194d (t/Makefile: don't remove test-results in >> "clean-except-prove-cache", 2022-07-28). >> >> The 'clean' target in 't/Makefile' was not directly responsible for >> removing the 'test-results' directory, but relied on its dependency >> 'clean-except-prove-cache' to do that [1]. ee65b194d broke this, >> because it only removed the 'rm -r test-results' command from the >> 'clean-except-prove-cache' target instead of moving it to the 'clean' >> target, resulting in stray 't/test-results' directories. >> >> Add that missing cleanup command to 't/Makefile', and to all >> sub-Makefiles touched by that commit as well. >> >> [1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs, >> 2012-05-02) >> >> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> >> --- > > Thanks, and sorry about the breakage. I've looked this over carefully & > it fixes the edge-case you noted without making anything else that I > could spot worse. > > In case it helps: > > Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > It's still a bit odd to have a "clean" that cleans up a thing it *might > have* generated, i.e. sometimes we create & use these via a Makefile > target, and sometimes by manually invoking test scripts. I think this is perfectly fine, but we could also use "distclean" for cleaning up something that might have been created. Thanks, both. Will queue.
SZEDER Gábor <szeder.dev@gmail.com> writes: > The 't/test-results' directory and its contents are by-products of the > test process, so 'make clean' should remove them, but, alas, this has > been broken since fee65b194d (t/Makefile: don't remove test-results in > "clean-except-prove-cache", 2022-07-28). What I find more disturbing is this is part of "leak check" topic, and I see no reason why we need to futz with "make clean" rule in the makefile while extending SANITIZE=leak support. We really should yell louder when we see topics that tries to do too much "while at it" and reject them to minimize the risk of introducing this kind of breakage. Will queue.
On Wed, Sep 21 2022, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > >> The 't/test-results' directory and its contents are by-products of the >> test process, so 'make clean' should remove them, but, alas, this has >> been broken since fee65b194d (t/Makefile: don't remove test-results in >> "clean-except-prove-cache", 2022-07-28). > > What I find more disturbing is this is part of "leak check" topic, > and I see no reason why we need to futz with "make clean" rule in > the makefile while extending SANITIZE=leak support. We really > should yell louder when we see topics that tries to do too much > "while at it" and reject them to minimize the risk of introducing > this kind of breakage. FWIW this wasn't a "while at it", it was critical to making another part of that topic work at all. It adds a "check" mode, which I use to check that the TEST_PASSES_SANITIZE_LEAK=true markings are correct. Intrinsic to how that works (and [1] has more details) is that we "pass" all tests, when some of them really failed under SANITIZE=leak. I.e. a failure of a test not opted in to TEST_PASSES_SANITIZE_LEAK=true is considered OK. Without fee65b194d that combined with GIT_TEST_SANITIZE_LEAK_LOG=true added in the same topic would make post-test leak analysis I do useless, because before fee65b194d the Makefile made the assumption that "test-results" directories were only need in case the tests failed. 1. e92684e1a26 (test-lib: add a GIT_TEST_PASSING_SANITIZE_LEAK=check mode, 2022-07-28)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > FWIW this wasn't a "while at it", it was critical to making another part > of that topic work at all. Perhaps it was critical within the approach of the topic took, and if a different approach was taken or the scope of topic was reduced, you would have avoided it. I would still consider such an unnecessary change an unwated "whilte at it". The moral of the story is that the topic took too big a bite to be done without causing unnecessary mistakes like this.
diff --git a/contrib/scalar/t/Makefile b/contrib/scalar/t/Makefile index 1ed174a8cf..e0bf2e32cb 100644 --- a/contrib/scalar/t/Makefile +++ b/contrib/scalar/t/Makefile @@ -46,6 +46,7 @@ clean-except-prove-cache: $(RM) -r valgrind/bin clean: clean-except-prove-cache + $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' $(RM) .prove test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax diff --git a/contrib/subtree/t/Makefile b/contrib/subtree/t/Makefile index 3d278bb0ed..4655e0987b 100644 --- a/contrib/subtree/t/Makefile +++ b/contrib/subtree/t/Makefile @@ -51,6 +51,7 @@ clean-except-prove-cache: $(RM) -r valgrind/bin clean: clean-except-prove-cache + $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' $(RM) .prove test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax diff --git a/t/Makefile b/t/Makefile index 1c80c0c79a..cb04481114 100644 --- a/t/Makefile +++ b/t/Makefile @@ -66,6 +66,7 @@ clean-except-prove-cache: clean-chainlint $(RM) -r valgrind/bin clean: clean-except-prove-cache + $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' $(RM) .prove clean-chainlint:
The 't/test-results' directory and its contents are by-products of the test process, so 'make clean' should remove them, but, alas, this has been broken since fee65b194d (t/Makefile: don't remove test-results in "clean-except-prove-cache", 2022-07-28). The 'clean' target in 't/Makefile' was not directly responsible for removing the 'test-results' directory, but relied on its dependency 'clean-except-prove-cache' to do that [1]. ee65b194d broke this, because it only removed the 'rm -r test-results' command from the 'clean-except-prove-cache' target instead of moving it to the 'clean' target, resulting in stray 't/test-results' directories. Add that missing cleanup command to 't/Makefile', and to all sub-Makefiles touched by that commit as well. [1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs, 2012-05-02) Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- Range-diff: 1: 36a3a71ea5 ! 1: ccb28d7ce7 t/Makefile: remove 'test-results' on 'make clean' @@ Commit message The 't/test-results' directory and its contents are by-products of the test process, so 'make clean' should remove them, but, alas, this has - been broken since ee65b194d (t/Makefile: don't remove test-results in + been broken since fee65b194d (t/Makefile: don't remove test-results in "clean-except-prove-cache", 2022-07-28). The 'clean' target in 't/Makefile' was not directly responsible for @@ Commit message 'clean-except-prove-cache' target instead of moving it to the 'clean' target, resulting in stray 't/test-results' directories. - Add that missing cleanup command to 't/Makefile', and all sub-Makefiles - touched by ee65b194d as well. + Add that missing cleanup command to 't/Makefile', and to all + sub-Makefiles touched by that commit as well. [1] 60f26f6348 (t/Makefile: retain cache t/.prove across prove runs, 2012-05-02) contrib/scalar/t/Makefile | 1 + contrib/subtree/t/Makefile | 1 + t/Makefile | 1 + 3 files changed, 3 insertions(+)