| Message ID | 20201118190414.32616-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 53ff3b96a87f34c66ffe4a784841c31cf9feb262 |
| Headers | show |
| Series | [1/2] tests: make sure nested lazy prereqs work reliably | expand |
SZEDER Gábor <szeder.dev@gmail.com> writes: > This can be problematic, because lazy prereqs are evaluated in the > '$TRASH_DIRECTORY/prereq-test-dir' directory, which is the same for > every prereq, and which is automatically removed after the prereq has > been evaluated. So if the inner prereq (BAR above) is a lazy prereq > that hasn't been evaluated yet, then after its evaluation the > 'prereq-test-dir' shared with the outer prereq will be removed. OK. > Consequently, 'check-foo' will find itself in a non-existing > directory, and won't be able to create/access any files in its cwd, > which could result in an unfulfilled outer prereq. I have a feeling that this would be filesystem dependent what happens in an unlinked directory. But in any case, if an outer lazy prereq creates a file in the test directory, tries to evaluate another lazy prereq (which would use the same directory and then would remove everything in it and the directory itself when done), and then expects that the file it created before evaluating the inner lazy prereq is still there, it would not work, so I think this is a good change. I just think "won't be able to" is a bit too strong here ("may not be able to (depending on the filesystem)", I would buy). > +test_lazy_prereq NESTED_INNER ' > + >inner && > + rm -f outer > +' > +test_lazy_prereq NESTED_PREREQ ' > + >outer && > + test_have_prereq NESTED_INNER && > + echo "can create new file in cwd" >file && > + test -f outer && > + test ! -f inner > +' And the existence check for outer is exactly what I wrote above. That would portably break without separte directory. I do not know if "can create new file" step would fail portably. > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index 8d59b90348..94395b9807 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -474,15 +474,15 @@ test_lazy_prereq () { > > test_run_lazy_prereq_ () { > script=' > -mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" && > +mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" && > ( > - cd "$TRASH_DIRECTORY/prereq-test-dir" &&'"$2"' > + cd "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" &&'"$2"' > )' Qoting rules are a bit tricky here, but I think it does not matter too much, as $1 (name or prereq) won't have $IFS or "'" or anything funny in it. > say >&3 "checking prerequisite: $1" > say >&3 "$script" > test_eval_ "$script" > eval_ret=$? > - rm -rf "$TRASH_DIRECTORY/prereq-test-dir" > + rm -rf "$TRASH_DIRECTORY/prereq-test-dir-$1" And this is obviously safe no matter what is in $1 ;-) > if test "$eval_ret" = 0; then > say >&3 "prerequisite $1 ok" > else Looks good.
On Wed, Nov 18, 2020 at 08:04:13PM +0100, SZEDER Gábor wrote: > So to prevent nested prereqs from interfering with each other let's > evaluate each prereq in its own dedicated directory by appending the > prereq's name to the directory name, e.g. 'prereq-test-dir-SYMLINKS'. > In the test we check not only that the prereq test dir is still there, > but also that the inner prereq can't mess with the outer prereq's > files. That sounds reasonable. I do wonder, though, whether simply creating the prereq directory in the _current_ directory would be sufficient. Then you'd get prereq-test-dir/prereq-test-dir for a nested invocation. But the prereqs aren't supposed to care about which specific directory they're in. I.e., your test passes equally well with just: diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 59bbf75e83..f5dc6801d9 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -474,15 +474,15 @@ test_lazy_prereq () { test_run_lazy_prereq_ () { script=' -mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" && +mkdir -p "prereq-test-dir" && ( - cd "$TRASH_DIRECTORY/prereq-test-dir" &&'"$2"' + cd "prereq-test-dir" &&'"$2"' )' say >&3 "checking prerequisite: $1" say >&3 "$script" test_eval_ "$script" eval_ret=$? - rm -rf "$TRASH_DIRECTORY/prereq-test-dir" + rm -rf "prereq-test-dir" if test "$eval_ret" = 0; then say >&3 "prerequisite $1 ok" else though I guess it is not really much simpler (it avoids the funky quoting around $1 in the embedded script, but we already have that for $2). And perhaps debugging is easier with a more predictable and descriptive directory name. > +test_lazy_prereq NESTED_INNER ' > + >inner && > + rm -f outer > +' > +test_lazy_prereq NESTED_PREREQ ' > + >outer && > + test_have_prereq NESTED_INNER && > + echo "can create new file in cwd" >file && > + test -f outer && > + test ! -f inner > +' > +test_expect_success NESTED_PREREQ 'evaluating nested lazy prereqs dont interfere with each other' ' > + nestedworks=yes > +' > + > +if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" && test "$nestedworks" != yes > +then > + say 'bug in test framework: nested lazy prerequisites do not work' > + exit 1 > +fi I was surprised to see this bare exit, because I know we have some functions (run_sub_test_*) to help with testing the framework itself. It looks like the other prereq tests don't use it either, though. I wonder if there is a technical reason, or if they were simply added at a different time. (Either way, I am OK for your new test to match the surrounding ones like you have here). -Peff
On Thu, Nov 19, 2020 at 10:58:24AM -0500, Jeff King wrote: > > +if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" && test "$nestedworks" != yes > > +then > > + say 'bug in test framework: nested lazy prerequisites do not work' > > + exit 1 > > +fi > > I was surprised to see this bare exit, because I know we have some > functions (run_sub_test_*) to help with testing the framework itself. It > looks like the other prereq tests don't use it either, though. I wonder > if there is a technical reason, or if they were simply added at a > different time. (Either way, I am OK for your new test to match the > surrounding ones like you have here). I took a look at converting some of the existing tests. This seems to work. It's a bit longer to read, perhaps, but I kind of like that the expected outcome is all laid out. It also pollutes the test output less (e.g., if you wanted to count up skipped tests in the whole suite, you'd get a bunch of noise from t0000 for these uninteresting skips). Thoughts? I think this is something I'd do on top of your patch. diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index f4ba2e8c85..f369af76be 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -759,43 +759,50 @@ test_expect_success '--run invalid range end' " EOF_ERR " - -test_set_prereq HAVEIT -haveit=no -test_expect_success HAVEIT 'test runs if prerequisite is satisfied' ' - test_have_prereq HAVEIT && - haveit=yes -' -donthaveit=yes -test_expect_success DONTHAVEIT 'unmet prerequisite causes test to be skipped' ' - donthaveit=no -' -if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit != yesyes -then - say "bug in test framework: prerequisite tags do not work reliably" - exit 1 -fi - -test_set_prereq HAVETHIS -haveit=no -test_expect_success HAVETHIS,HAVEIT 'test runs if prerequisites are satisfied' ' - test_have_prereq HAVEIT && - test_have_prereq HAVETHIS && - haveit=yes -' -donthaveit=yes -test_expect_success HAVEIT,DONTHAVEIT 'unmet prerequisites causes test to be skipped' ' - donthaveit=no -' -donthaveiteither=yes -test_expect_success DONTHAVEIT,HAVEIT 'unmet prerequisites causes test to be skipped' ' - donthaveiteither=no +test_expect_success 'tests respect prerequisites' ' + run_sub_test_lib_test prereqs "tests respect prereqs" <<-\EOF && + + test_set_prereq HAVEIT + haveit=no + test_expect_success HAVEIT "prereq is satisfied" " + test_have_prereq HAVEIT && + haveit=yes + " + + donthaveit=yes + test_expect_success DONTHAVEIT "prereq not satisfied" " + donthaveit=no + " + + test_set_prereq HAVETHIS + haveit=no + test_expect_success HAVETHIS,HAVEIT "multiple prereqs" " + test_have_prereq HAVEIT && + test_have_prereq HAVETHIS && + haveit=yes + " + + donthaveit=yes + test_expect_success HAVEIT,DONTHAVEIT "mixed prereqs (yes,no)" " + donthaveit=no + " + + donthaveiteither=yes + test_expect_success DONTHAVEIT,HAVEIT "mixed prereqs (no,yes)" " + donthaveiteither=no + " + test_done + EOF + check_sub_test_lib_test prereqs <<-\EOF + ok 1 - prereq is satisfied + ok 2 # skip prereq not satisfied (missing DONTHAVEIT) + ok 3 - multiple prereqs + ok 4 # skip mixed prereqs (yes,no) (missing DONTHAVEIT of HAVEIT,DONTHAVEIT) + ok 5 # skip mixed prereqs (no,yes) (missing DONTHAVEIT of DONTHAVEIT,HAVEIT) + # passed all 5 test(s) + 1..5 + EOF ' -if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" -a $haveit$donthaveit$donthaveiteither != yesyesyes -then - say "bug in test framework: multiple prerequisite tags do not work reliably" - exit 1 -fi test_lazy_prereq LAZY_TRUE true havetrue=no
Jeff King <peff@peff.net> writes: > I took a look at converting some of the existing tests. This seems to > work. It's a bit longer to read, perhaps, but I kind of like that the > expected outcome is all laid out. It also pollutes the test output less > (e.g., if you wanted to count up skipped tests in the whole suite, you'd > get a bunch of noise from t0000 for these uninteresting skips). > > Thoughts? I think this is something I'd do on top of your patch. Yes, it looks nice as the expectation is expressed much clearly.
Jeff King <peff@peff.net> writes: > On Wed, Nov 18, 2020 at 08:04:13PM +0100, SZEDER Gábor wrote: > >> So to prevent nested prereqs from interfering with each other let's >> evaluate each prereq in its own dedicated directory by appending the >> prereq's name to the directory name, e.g. 'prereq-test-dir-SYMLINKS'. >> In the test we check not only that the prereq test dir is still there, >> but also that the inner prereq can't mess with the outer prereq's >> files. > > That sounds reasonable. I do wonder, though, whether simply creating the > prereq directory in the _current_ directory would be sufficient. Then > you'd get prereq-test-dir/prereq-test-dir for a nested invocation. But > the prereqs aren't supposed to care about which specific directory > they're in. True. That does sound conceptually simpler. As we've already seen the patch, I do not mind too deeply either way, though. Thanks.
On Thu, Nov 19, 2020 at 11:50:26AM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I took a look at converting some of the existing tests. This seems to > > work. It's a bit longer to read, perhaps, but I kind of like that the > > expected outcome is all laid out. It also pollutes the test output less > > (e.g., if you wanted to count up skipped tests in the whole suite, you'd > > get a bunch of noise from t0000 for these uninteresting skips). > > > > Thoughts? I think this is something I'd do on top of your patch. > > Yes, it looks nice as the expectation is expressed much clearly. OK, then here's the whole thing. I ended up with a few more cleanups, too. This is all on top of Gábor's patches. It's conceptually independent, but the textual wrangling was annoying enough it didn't make any sense to require you to do it again during merging. ;) Plus I do not think either topic is high-risk nor urgent enough to worry too much about one blocking the other. The diffstat is scary, but it's mostly the final patch, which is pretty mechanical. [1/4]: t0000: keep clean-up tests together [2/4]: t0000: run prereq tests inside sub-test [3/4]: t0000: run cleaning test inside sub-test [4/4]: t0000: consistently use single quotes for outer tests t/t0000-basic.sh | 570 +++++++++++++++++++++++------------------------ 1 file changed, 284 insertions(+), 286 deletions(-) -Peff
Jeff King <peff@peff.net> writes: > OK, then here's the whole thing. I ended up with a few more cleanups, > too. This is all on top of Gábor's patches. It's conceptually > independent, but the textual wrangling was annoying enough it didn't > make any sense to require you to do it again during merging. ;) Plus I > do not think either topic is high-risk nor urgent enough to worry too > much about one blocking the other. I'll ask you to do the last step maybe in a few weeks; the range notation tests seem to have changed since where I queued Gábor's patches and where [4/4] is based on (yours is based on newer codebase). > The diffstat is scary, but it's mostly the final patch, which is pretty > mechanical. Yup, and the result is much easier to read. Thanks. > [1/4]: t0000: keep clean-up tests together > [2/4]: t0000: run prereq tests inside sub-test > [3/4]: t0000: run cleaning test inside sub-test > [4/4]: t0000: consistently use single quotes for outer tests > > t/t0000-basic.sh | 570 +++++++++++++++++++++++------------------------ > 1 file changed, 284 insertions(+), 286 deletions(-) > > -Peff
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 923281af93..4031a0e3fc 100755 --- a/t/t0000-basic.sh +++ b/t/t0000-basic.sh @@ -831,6 +831,27 @@ then exit 1 fi +test_lazy_prereq NESTED_INNER ' + >inner && + rm -f outer +' +test_lazy_prereq NESTED_PREREQ ' + >outer && + test_have_prereq NESTED_INNER && + echo "can create new file in cwd" >file && + test -f outer && + test ! -f inner +' +test_expect_success NESTED_PREREQ 'evaluating nested lazy prereqs dont interfere with each other' ' + nestedworks=yes +' + +if test -z "$GIT_TEST_FAIL_PREREQS_INTERNAL" && test "$nestedworks" != yes +then + say 'bug in test framework: nested lazy prerequisites do not work' + exit 1 +fi + test_expect_success 'lazy prereqs do not turn off tracing' " run_sub_test_lib_test lazy-prereq-and-tracing \ 'lazy prereqs and -x' -v -x <<-\\EOF && diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 8d59b90348..94395b9807 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -474,15 +474,15 @@ test_lazy_prereq () { test_run_lazy_prereq_ () { script=' -mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" && +mkdir -p "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" && ( - cd "$TRASH_DIRECTORY/prereq-test-dir" &&'"$2"' + cd "$TRASH_DIRECTORY/prereq-test-dir-'"$1"'" &&'"$2"' )' say >&3 "checking prerequisite: $1" say >&3 "$script" test_eval_ "$script" eval_ret=$? - rm -rf "$TRASH_DIRECTORY/prereq-test-dir" + rm -rf "$TRASH_DIRECTORY/prereq-test-dir-$1" if test "$eval_ret" = 0; then say >&3 "prerequisite $1 ok" else
Some test prereqs depend on other prereqs, so in a couple of cases we have nested prereqs that look something like this: test_lazy_prereq FOO ' test_have_prereq BAR && check-foo ' This can be problematic, because lazy prereqs are evaluated in the '$TRASH_DIRECTORY/prereq-test-dir' directory, which is the same for every prereq, and which is automatically removed after the prereq has been evaluated. So if the inner prereq (BAR above) is a lazy prereq that hasn't been evaluated yet, then after its evaluation the 'prereq-test-dir' shared with the outer prereq will be removed. Consequently, 'check-foo' will find itself in a non-existing directory, and won't be able to create/access any files in its cwd, which could result in an unfulfilled outer prereq. Luckily, this doesn't affect any of our current nested prereqs, either because the inner prereq is not a lazy prereq (e.g. MINGW, CYGWIN or PERL), or because the outer prereq happens to be checked without touching any paths in its cwd (GPGSM and RFC1991 in 'lib-gpg.sh'). So to prevent nested prereqs from interfering with each other let's evaluate each prereq in its own dedicated directory by appending the prereq's name to the directory name, e.g. 'prereq-test-dir-SYMLINKS'. In the test we check not only that the prereq test dir is still there, but also that the inner prereq can't mess with the outer prereq's files. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- This came up while looking into: https://public-inbox.org/git/nycvar.QRO.7.76.6.2011152252520.18437@tvgsbejvaqbjf.bet/ because this did become an issue when I made the JGIT prereq depend on SHA1. t/t0000-basic.sh | 21 +++++++++++++++++++++ t/test-lib-functions.sh | 6 +++--- 2 files changed, 24 insertions(+), 3 deletions(-)