Re: [PATCH v2 2/6] t: extend test_lazy_prereq
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 13, 2025, 11:56 UTC
- Message-ID
- <xmqqsenhrvns.fsf@gitster.g>
- In-Reply-To
- <Z9ExMHf9CkcDwEt1@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 8 quoted lines
>> + elif test "$eval_ret" = 125; then >> + :; >> else >> say >&3 "prerequisite $1 not satisfied" >> fi > > The semicolon in ":;" threw me off a bit. Am I missing why we need it or > is it superfluous?
The latter, of course -).
Show 15 quoted lines
>> @@ -811,6 +813,9 @@ test_have_prereq () {
>> if test_run_lazy_prereq_ "$prerequisite" "$script"
>> then
>> test_set_prereq $prerequisite
>> + elif test $? = 125
>> + then
>> + BUG "Do not use $prerequisite"
>> fi
>> lazily_tested_prereq="$lazily_tested_prereq$prerequisite "
>> esac
>
> Hm, okay. It feels quite close to overthinking the whole deprecation
> cycle around prerequisites as it's nothing that we tend to do very
> often. But on the other hand the implementation is trivial enough, so I
> don't mind it much.I agree that this has nothing to do with breaking changes at the Git 3.0 boundary. We just did not have good documentation for lazy prerequisites, and we just did not have any good support for marking a prerequisite should no longer be used. [1/6] is for the former, and [2/6] is for the latter.
We can avoid the magic 125 by adding a new helper like test_removed_prereq and do this instead, which may be cleaner and simpler to reason about.
Another alternative that may make writing tests even be less error prone but a bit more verbose is to introduce test_unset_prereq and be explicit about unsatisfied prerequisites. The effect of the resulting system becomes larger to include detecting misspelled prerequisites, and removed prerequisites would be detected as a natural fallout from the same mechanism.
As we have >50 prerequisites defined with test_set_prereq and we'd need to add 50 calls to test_unset_prereq to mark them as "known but not satisified on this platform" to differentiate them from the ones that are misspelt or removed, if we go that route. I am not sure if that is worth it. Certainly not in the short term, but for a longer term, if people ever misspelt a prerequisite SYMLINKS as SYMLINK and wasted time wondering why their tests didn't trigger, it might be worth it. I dunno.
t/test-lib-functions.sh | 14 ++++++++++++++ t/test-lib.sh | 5 +++-- 2 files changed, 17 insertions(+), 2 deletions(-)
diff --git c/t/test-lib-functions.sh w/t/test-lib-functions.sh index 79377bc0fc..3903344fc1 100644 --- c/t/test-lib-functions.sh +++ w/t/test-lib-functions.sh @@ -751,7 +751,15 @@ test_set_prereq () { ;; esac } + satisfied_prereq=" " + +removed_prereq= +# Mark a prerequisite deprecated-and-then-removed +test_removed_prereq () { + removed_prereq="$removed_prereq$1 " +} + lazily_testable_prereq= lazily_tested_prereq= # Usage: test_lazy_prereq PREREQ 'script' @@ -801,6 +809,12 @@ test_have_prereq () { negative_prereq= esac + case " $removed_prereq " in + *" $prerequisite "*) + BUG "Do not use $prerequisite" + ;; + esac + case " $lazily_tested_prereq " in *" $prerequisite "*) ;; diff --git c/t/test-lib.sh w/t/test-lib.sh index 9001ed3a64..c2c96f5e7a 100644 --- c/t/test-lib.sh +++ w/t/test-lib.sh @@ -1862,8 +1862,9 @@ test_lazy_prereq CURL ' curl --version ' -test_lazy_prereq WITHOUT_BREAKING_CHANGES ' - test -z "$WITH_BREAKING_CHANGES" +test_removed_prereq WITHOUT_BREAKING_CHANGES +test_lazy_prereq WITH_BREAKING_CHANGES ' + test -n "$WITH_BREAKING_CHANGES" ' # SHA1 is a test if the hash algorithm in use is SHA-1. This is both for tests