| Message ID | 20211117090410.8013-3-fs@gigacodes.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib: improve missing prereq handling | expand |
Fabian Stelzer <fs@gigacodes.de> writes: > + # Abort if this prereq was marked as required > + if test -n $GIT_TEST_REQUIRE_PREREQ If GIT_TEST_REQUIRE_PREREQ is an empty string, this will ask test -n and "test" will say "yes" (because "-n" is not an empty string). Let's surround it with a pair of double-quotes. > + then > + case ",$GIT_TEST_REQUIRE_PREREQ," in > + *,$prerequisite,*) > + error "required prereq $prerequisite failed" > + ;; > + esac > + fi > + > if test -z "$missing_prereq" > then > missing_prereq=$prerequisite
On 18.11.2021 15:42, Junio C Hamano wrote: >Fabian Stelzer <fs@gigacodes.de> writes: > >> + # Abort if this prereq was marked as required >> + if test -n $GIT_TEST_REQUIRE_PREREQ > >If GIT_TEST_REQUIRE_PREREQ is an empty string, this will ask > > test -n > >and "test" will say "yes" (because "-n" is not an empty string). > >Let's surround it with a pair of double-quotes. Will do. Thanks > >> + then >> + case ",$GIT_TEST_REQUIRE_PREREQ," in >> + *,$prerequisite,*) >> + error "required prereq $prerequisite failed" >> + ;; >> + esac >> + fi >> + >> if test -z "$missing_prereq" >> then >> missing_prereq=$prerequisite
On Wed, Nov 17 2021, Fabian Stelzer wrote: > In certain environments or for specific test scenarios we might expect a > specific prerequisite check to succeed. Therefore we would like to > trigger an error when running our tests if this is not the case. trigger an error but... > To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ > which can be set to a comma separated list of prereqs. If one of these > prereq tests fail then the whole test run will abort. ..here it's "abort the whole test run". If that's what you want use BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!" syntax on bad SANITIZE=leak use, 2021-10-14) > +GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of > +prereqs that are required to succeed. If a prereq in this list is triggered by > +a test and then fails then the whole test run will abort. This can help to make > +sure the expected tests are executed and not silently skipped when their > +dependency breaks or is simply not present in a new environment. > + > Naming Tests > ------------ For other things we specify via lists such as GIT_SKIP_TESTS that's space-separated, but here it's comma-separated, isn't that just a leaky abstraction in this case? I.e. this is exposing a previously internal-only implementation detail of the prereq code. It's less painful in shellscript if anything like this supports space-separated parameters, as you can interpolate them more easily in any wrapper script without using "tr" or the like...
On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote: > >On Wed, Nov 17 2021, Fabian Stelzer wrote: > >> In certain environments or for specific test scenarios we might expect a >> specific prerequisite check to succeed. Therefore we would like to >> trigger an error when running our tests if this is not the case. > >trigger an error but... > >> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ >> which can be set to a comma separated list of prereqs. If one of these >> prereq tests fail then the whole test run will abort. > >..here it's "abort the whole test run". If that's what you want use >BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!" >syntax on bad SANITIZE=leak use, 2021-10-14) > ok, thanks. BAIL_OUT seems better. i grepped through the tests and didn't find anything like it, so i used error. >> +GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of >> +prereqs that are required to succeed. If a prereq in this list is triggered by >> +a test and then fails then the whole test run will abort. This can help to make >> +sure the expected tests are executed and not silently skipped when their >> +dependency breaks or is simply not present in a new environment. >> + >> Naming Tests >> ------------ > >For other things we specify via lists such as GIT_SKIP_TESTS that's >space-separated, but here it's comma-separated, isn't that just a leaky >abstraction in this case? I.e. this is exposing a previously >internal-only implementation detail of the prereq code. > >It's less painful in shellscript if anything like this supports >space-separated parameters, as you can interpolate them more easily in >any wrapper script without using "tr" or the like... Ok. easy enough to change. Should the listing of missing prereq at the end of a test run be space separated as well? (maybe helps with word wrapping)
On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote: > >On Wed, Nov 17 2021, Fabian Stelzer wrote: > >> In certain environments or for specific test scenarios we might expect a >> specific prerequisite check to succeed. Therefore we would like to >> trigger an error when running our tests if this is not the case. > >trigger an error but... > >> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ >> which can be set to a comma separated list of prereqs. If one of these >> prereq tests fail then the whole test run will abort. > >..here it's "abort the whole test run". If that's what you want use >BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!" >syntax on bad SANITIZE=leak use, 2021-10-14) > Hm, while testing this change i noticed another problem that i really have no idea how to fix. When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed when run with '-v'. This is not the case when the prereq is specified in the test header. The test run will abort, but no error will be printed which can be quite confusing :/ I guess this has something to do with how tests are run in subshells and their outputs only printed with -v. Maybe there should be some kind of override for BAIL_OUT at least? Not sure if/how this could be done.
On Fri, Nov 19 2021, Fabian Stelzer wrote: > On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote: >> >>On Wed, Nov 17 2021, Fabian Stelzer wrote: >> >>> In certain environments or for specific test scenarios we might expect a >>> specific prerequisite check to succeed. Therefore we would like to >>> trigger an error when running our tests if this is not the case. >> >>trigger an error but... >> >>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ >>> which can be set to a comma separated list of prereqs. If one of these >>> prereq tests fail then the whole test run will abort. >> >>..here it's "abort the whole test run". If that's what you want use >>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!" >>syntax on bad SANITIZE=leak use, 2021-10-14) >> > > Hm, while testing this change i noticed another problem that i really > have no idea how to fix. > When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed > when run with '-v'. This is not the case when the prereq is specified > in the test header. The test run will abort, but no error will be > printed which can be quite confusing :/ > I guess this has something to do with how tests are run in subshells and > their outputs only printed with -v. Maybe there should be some kind of > override for BAIL_OUT at least? Not sure if/how this could be done. It has to do with how we juggle file descriptors around, see test_eval_ in test-lib.sh. So the "real" stdout is fd 5, not 1 when you're in a prereq. Just: BAIL_OUT "bad" >&5 Will work, maybe it's a good idea to have: BAIL_OUT_PREREQ () { BAIL_OUT $@ >&5 } Sorry, I forgot about that caveat when suggesting it.
On 19.11.2021 15:26, Ævar Arnfjörð Bjarmason wrote: > >On Fri, Nov 19 2021, Fabian Stelzer wrote: > >> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote: >>> >>>On Wed, Nov 17 2021, Fabian Stelzer wrote: >>> >>>> In certain environments or for specific test scenarios we might expect a >>>> specific prerequisite check to succeed. Therefore we would like to >>>> trigger an error when running our tests if this is not the case. >>> >>>trigger an error but... >>> >>>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ >>>> which can be set to a comma separated list of prereqs. If one of these >>>> prereq tests fail then the whole test run will abort. >>> >>>..here it's "abort the whole test run". If that's what you want use >>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!" >>>syntax on bad SANITIZE=leak use, 2021-10-14) >>> >> >> Hm, while testing this change i noticed another problem that i really >> have no idea how to fix. >> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed >> when run with '-v'. This is not the case when the prereq is specified >> in the test header. The test run will abort, but no error will be >> printed which can be quite confusing :/ >> I guess this has something to do with how tests are run in subshells and >> their outputs only printed with -v. Maybe there should be some kind of >> override for BAIL_OUT at least? Not sure if/how this could be done. > >It has to do with how we juggle file descriptors around, see test_eval_ >in test-lib.sh. > >So the "real" stdout is fd 5, not 1 when you're in a prereq. > >Just: > > BAIL_OUT "bad" >&5 > >Will work, maybe it's a good idea to have: > > BAIL_OUT_PREREQ () { > BAIL_OUT $@ >&5 > } > >Sorry, I forgot about that caveat when suggesting it. Hm. Any reason to not do this in BAIL_OUT itself? As far as i can see the setup of the additional fd's would only need to move up a few lines.
On Fri, Nov 19 2021, Fabian Stelzer wrote: > On 19.11.2021 15:26, Ævar Arnfjörð Bjarmason wrote: >> >>On Fri, Nov 19 2021, Fabian Stelzer wrote: >> >>> On 19.11.2021 12:13, Ævar Arnfjörð Bjarmason wrote: >>>> >>>>On Wed, Nov 17 2021, Fabian Stelzer wrote: >>>> >>>>> In certain environments or for specific test scenarios we might expect a >>>>> specific prerequisite check to succeed. Therefore we would like to >>>>> trigger an error when running our tests if this is not the case. >>>> >>>>trigger an error but... >>>> >>>>> To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ >>>>> which can be set to a comma separated list of prereqs. If one of these >>>>> prereq tests fail then the whole test run will abort. >>>> >>>>..here it's "abort the whole test run". If that's what you want use >>>>BAIL_OUT, not error. See: 234383cd401 (test-lib.sh: use "Bail out!" >>>>syntax on bad SANITIZE=leak use, 2021-10-14) >>>> >>> >>> Hm, while testing this change i noticed another problem that i really >>> have no idea how to fix. >>> When a test uses test_have_prereq then the error/BAIL_OUT message will only be printed >>> when run with '-v'. This is not the case when the prereq is specified >>> in the test header. The test run will abort, but no error will be >>> printed which can be quite confusing :/ >>> I guess this has something to do with how tests are run in subshells and >>> their outputs only printed with -v. Maybe there should be some kind of >>> override for BAIL_OUT at least? Not sure if/how this could be done. >> >>It has to do with how we juggle file descriptors around, see test_eval_ >>in test-lib.sh. >> >>So the "real" stdout is fd 5, not 1 when you're in a prereq. >> >>Just: >> >> BAIL_OUT "bad" >&5 >> >>Will work, maybe it's a good idea to have: >> >> BAIL_OUT_PREREQ () { >> BAIL_OUT $@ >&5 >> } >> >>Sorry, I forgot about that caveat when suggesting it. > > Hm. Any reason to not do this in BAIL_OUT itself? As far as i can see > the setup of the additional fd's would only need to move up a few lines. That does look like a better solution, I've tried it just now locally & it works for me. Perhaps there's some subtlety I'm missing, but that should Just Work. This is by far not the first time I've poked at something in test-lib.sh only to discover that its pattern of doing setup A, setup C, setup B etc. caused a problem solved by moving B & C around :( It could really do with a change to move everything it's now doing to functions, which we'd then call, so what setup we do in what order would fit on a single screen, but that's a much larger change...
diff --git a/t/README b/t/README index 29f72354bf..18ce75976e 100644 --- a/t/README +++ b/t/README @@ -466,6 +466,12 @@ explicitly providing repositories when accessing submodule objects is complete or needs to be abandoned for whatever reason (in which case the migrated codepaths still retain their performance benefits). +GIT_TEST_REQUIRE_PREREQ=<list> allows specifying a comma speparated list of +prereqs that are required to succeed. If a prereq in this list is triggered by +a test and then fails then the whole test run will abort. This can help to make +sure the expected tests are executed and not silently skipped when their +dependency breaks or is simply not present in a new environment. + Naming Tests ------------ diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index eef2262a36..2c8abf3420 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -680,6 +680,17 @@ test_have_prereq () { # Keep a list of missing prerequisites; restore # the negative marker if necessary. prerequisite=${negative_prereq:+!}$prerequisite + + # Abort if this prereq was marked as required + if test -n $GIT_TEST_REQUIRE_PREREQ + then + case ",$GIT_TEST_REQUIRE_PREREQ," in + *,$prerequisite,*) + error "required prereq $prerequisite failed" + ;; + esac + fi + if test -z "$missing_prereq" then missing_prereq=$prerequisite
In certain environments or for specific test scenarios we might expect a specific prerequisite check to succeed. Therefore we would like to trigger an error when running our tests if this is not the case. To remedy this we add the environment variable GIT_TEST_REQUIRE_PREREQ which can be set to a comma separated list of prereqs. If one of these prereq tests fail then the whole test run will abort. Signed-off-by: Fabian Stelzer <fs@gigacodes.de> --- t/README | 6 ++++++ t/test-lib-functions.sh | 11 +++++++++++ 2 files changed, 17 insertions(+)