| Message ID | patch-02.16-44223ae777e-20210412T110456Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: new test_commit args, simplification & fixes | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > There's no point in creating a repository or directory only to decide > right afterwards that we're skipping all the tests. > > So let's partially revert 06478dab4c (test-lib: retire $remove_trash > variable, 2017-04-23) and move the decision about whether to skip all > tests earlier. > > I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf > under --debug at the end of the test, 2017-04-24) for a bug we don't > want to re-introduce. > > While I'm at it let's move the HOME assignment to just before > test_create_repo, it could be lower, but it seems better to set it > before calling anything in test-lib-functions.sh > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- I am not clear what the reasoning behind this change is. Is it correct to say that the idea flows like the following? 0. If we do the $GIT_SKIP_TESTS check early, specifically, before we create the trash directory to run the tests in, we do not have to create and then remove it. 1. If we naïvely do the above, test_done called in the "ah, the entirety of test is skipped" case will try to see if the trash directory exists and complain, so we need to add more code in that test_done codepath to avoid complaints. 2. As a part of that "more code", we mark the fact that we created a new repository with $remove_trash variable. Assuming that the above flow of thought is what is behind this patch, I think the patch is mostly sensible. remove_trash needs to be cleared at the beginning, perhaps where store_arg_to and opt_required_arg are cleared, though, to protect us from a stray environment variable. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> There's no point in creating a repository or directory only to decide >> right afterwards that we're skipping all the tests. >> >> So let's partially revert 06478dab4c (test-lib: retire $remove_trash >> variable, 2017-04-23) and move the decision about whether to skip all >> tests earlier. >> >> I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf >> under --debug at the end of the test, 2017-04-24) for a bug we don't >> want to re-introduce. >> >> While I'm at it let's move the HOME assignment to just before >> test_create_repo, it could be lower, but it seems better to set it >> before calling anything in test-lib-functions.sh >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- > > I am not clear what the reasoning behind this change is. Is it > correct to say that the idea flows like the following? > > 0. If we do the $GIT_SKIP_TESTS check early, specifically, before > we create the trash directory to run the tests in, we do not > have to create and then remove it. > > 1. If we naïvely do the above, test_done called in the "ah, the > entirety of test is skipped" case will try to see if the trash > directory exists and complain, so we need to add more code in > that test_done codepath to avoid complaints. > > 2. As a part of that "more code", we mark the fact that we created > a new repository with $remove_trash variable. > > Assuming that the above flow of thought is what is behind this > patch, I think the patch is mostly sensible. By the way, I did wonder if the call to test_done is even needed, or if it is even ready to be called (in other words, the machineries it tries to finalize like junit and HARNESS_ACTIVE are even initialized yet). I verified that calling test_done this early is fine with respect to the call it makes to finalize_junit_xml, though. > remove_trash needs to be cleared at the beginning, perhaps where > store_arg_to and opt_required_arg are cleared, though, to protect > us from a stray environment variable. > > Thanks.
diff --git a/t/test-lib.sh b/t/test-lib.sh index d3f6af6a654..a8869eee58f 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -1167,7 +1167,7 @@ test_done () { esac fi - if test -z "$debug" + if test -z "$debug" && test -n "$remove_trash" then test -d "$TRASH_DIRECTORY" || error "Tests passed but trash directory already removed before test cleanup; aborting" @@ -1332,6 +1332,21 @@ then exit 1 fi +# Are we running this test at all? +this_test=${0##*/} +this_test=${this_test%%-*} +if match_pattern_list "$this_test" $GIT_SKIP_TESTS +then + say_color info >&3 "skipping test $this_test altogether" + skip_all="skip all tests in $this_test" + test_done +fi + +# Last-minute variable setup +HOME="$TRASH_DIRECTORY" +GNUPGHOME="$HOME/gnupg-home-not-used" +export HOME GNUPGHOME + # Test repository rm -fr "$TRASH_DIRECTORY" || { GIT_EXIT_OK=t @@ -1339,10 +1354,7 @@ rm -fr "$TRASH_DIRECTORY" || { exit 1 } -HOME="$TRASH_DIRECTORY" -GNUPGHOME="$HOME/gnupg-home-not-used" -export HOME GNUPGHOME - +remove_trash=t if test -z "$TEST_NO_CREATE_REPO" then test_create_repo "$TRASH_DIRECTORY" @@ -1354,15 +1366,6 @@ fi # in subprocesses like git equals our $PWD (for pathname comparisons). cd -P "$TRASH_DIRECTORY" || exit 1 -this_test=${0##*/} -this_test=${this_test%%-*} -if match_pattern_list "$this_test" $GIT_SKIP_TESTS -then - say_color info >&3 "skipping test $this_test altogether" - skip_all="skip all tests in $this_test" - test_done -fi - if test -n "$write_junit_xml" then junit_xml_dir="$TEST_OUTPUT_DIRECTORY/out"
There's no point in creating a repository or directory only to decide right afterwards that we're skipping all the tests. So let's partially revert 06478dab4c (test-lib: retire $remove_trash variable, 2017-04-23) and move the decision about whether to skip all tests earlier. I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf under --debug at the end of the test, 2017-04-24) for a bug we don't want to re-introduce. While I'm at it let's move the HOME assignment to just before test_create_repo, it could be lower, but it seems better to set it before calling anything in test-lib-functions.sh Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/test-lib.sh | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)