Обсуждение: Mop-up from Test::More version change patch
[ moving thread to -hackers for a bit more visibility ] Attached are a couple of patches I propose in the wake of commit 405f32fc4 (Require version 0.98 of Test::More for TAP tests). 0001 responds to the failure we saw on buildfarm member wrasse [1] where, despite configure having carefully checked for Test::More being >= 0.98, actual tests failed with Test::More version 0.98 required--this is only version 0.92 at /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/test/perl/PostgreSQL/Test/Utils.pmline 63. The reason is that wrasse was choosing "prove" from a different Perl installation than "perl", as a result of its configuration having set PERL to a nondefault place but doing nothing about PROVE. We already installed a couple of mitigations for that: (a) as of c4fe3199a, configure checks "prove" not "perl" for appropriate module versions; (b) Noah has modified wrasse's configuration to set PROVE. But I'm of the opinion that (b) should not be necessary. If you set PERL then it's highly likely that you want to use "prove" from the same installation. So 0001 modifies configure to first check for an executable "prove" in the same directory as $PERL. If that's not what you want then you should override it by setting PROVE explicitly. Since this is mainly meant to prevent an easy-to-make error in setting up buildfarm configurations, we should back-patch it. 0002 is written to apply to v14 and earlier, and what it wants to do is back-patch the effects of 405f32fc4, so that the minimum Test::More version is 0.98 in all branches. The thought here is that (1) somebody is likely to want to back-patch a test involving Test::More::subtest before too long; (2) we have zero coverage for older Test::More versions anyway, now that all buildfarm members have been updated to work with HEAD. Any objections? regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=wrasse&dt=2021-11-20%2022%3A58%3A17 diff --git a/configure b/configure index 896b781473..a86a31fb42 100755 --- a/configure +++ b/configure @@ -19411,7 +19411,17 @@ fi # if test "$enable_tap_tests" = yes; then # Make sure we know where prove is. + # Prefer a prove located in the same directory as $PERL, + # otherwise search $PATH. if test -z "$PROVE"; then + if test -x "`dirname "$PERL"`/prove"; then + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for prove" >&5 +$as_echo_n "checking for prove... " >&6; } + PROVE="`dirname "$PERL"`/prove" + { $as_echo "$as_me:${as_lineno-$LINENO}: result: $PROVE" >&5 +$as_echo "$PROVE" >&6; } + else + if test -z "$PROVE"; then for ac_prog in prove do # Extract the first word of "$ac_prog", so it can be a program name with args. @@ -19465,8 +19475,10 @@ $as_echo_n "checking for PROVE... " >&6; } $as_echo "$PROVE" >&6; } fi - if test -z "$PROVE"; then - as_fn_error $? "prove not found" "$LINENO" 5 + if test -z "$PROVE"; then + as_fn_error $? "prove not found" "$LINENO" 5 + fi + fi fi # Check for necessary Perl modules. You might think we should use # AX_PROG_PERL_MODULES here, but prove might be part of a different Perl diff --git a/configure.ac b/configure.ac index b50130b323..98cd5ae12e 100644 --- a/configure.ac +++ b/configure.ac @@ -2378,9 +2378,19 @@ PGAC_PATH_PROGS(DBTOEPUB, dbtoepub) # if test "$enable_tap_tests" = yes; then # Make sure we know where prove is. - PGAC_PATH_PROGS(PROVE, prove) + # Prefer a prove located in the same directory as $PERL, + # otherwise search $PATH. if test -z "$PROVE"; then - AC_MSG_ERROR([prove not found]) + if test -x "`dirname "$PERL"`/prove"; then + AC_MSG_CHECKING(for prove) + PROVE="`dirname "$PERL"`/prove" + AC_MSG_RESULT([$PROVE]) + else + PGAC_PATH_PROGS(PROVE, prove) + if test -z "$PROVE"; then + AC_MSG_ERROR([prove not found]) + fi + fi fi # Check for necessary Perl modules. You might think we should use # AX_PROG_PERL_MODULES here, but prove might be part of a different Perl diff --git a/config/check_modules.pl b/config/check_modules.pl index 399ac5e3b6..cc0a7ab0e7 100644 --- a/config/check_modules.pl +++ b/config/check_modules.pl @@ -11,7 +11,7 @@ use IPC::Run 0.79; # Test::More and Time::HiRes are supposed to be part of core Perl, # but some distros omit them in a minimal installation. -use Test::More 0.87; +use Test::More 0.98; use Time::HiRes 1.52; # While here, we might as well report exactly what versions we found. diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm index 10bf31755e..f607ee3a4b 100644 --- a/src/test/perl/TestLib.pm +++ b/src/test/perl/TestLib.pm @@ -58,9 +58,8 @@ use File::Temp (); use IPC::Run; use SimpleTee; -# specify a recent enough version of Test::More to support the -# done_testing() function -use Test::More 0.87; +# We need a version of Test::More recent enough to support subtests +use Test::More 0.98; our @EXPORT = qw( generate_ascii_string
On 11/23/21 12:03, Tom Lane wrote: > [ moving thread to -hackers for a bit more visibility ] > > Attached are a couple of patches I propose in the wake of commit > 405f32fc4 (Require version 0.98 of Test::More for TAP tests). > > 0001 responds to the failure we saw on buildfarm member wrasse [1] > where, despite configure having carefully checked for Test::More > being >= 0.98, actual tests failed with > Test::More version 0.98 required--this is only version 0.92 at /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/test/perl/PostgreSQL/Test/Utils.pmline 63. > The reason is that wrasse was choosing "prove" from a different > Perl installation than "perl", as a result of its configuration > having set PERL to a nondefault place but doing nothing about PROVE. > > We already installed a couple of mitigations for that: > (a) as of c4fe3199a, configure checks "prove" not "perl" for > appropriate module versions; > (b) Noah has modified wrasse's configuration to set PROVE. > But I'm of the opinion that (b) should not be necessary. > If you set PERL then it's highly likely that you want to use > "prove" from the same installation. So 0001 modifies configure > to first check for an executable "prove" in the same directory > as $PERL. If that's not what you want then you should override > it by setting PROVE explicitly. > > Since this is mainly meant to prevent an easy-to-make error in > setting up buildfarm configurations, we should back-patch it. Do we really have much of an issue left to solve given c4fe3199a? It feels a bit like a solution in search of a problem. > > 0002 is written to apply to v14 and earlier, and what it wants > to do is back-patch the effects of 405f32fc4, so that the > minimum Test::More version is 0.98 in all branches. The thought > here is that (1) somebody is likely to want to back-patch a > test involving Test::More::subtest before too long; (2) we have > zero coverage for older Test::More versions anyway, now that > all buildfarm members have been updated to work with HEAD. > This one seems like a good idea. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Andrew Dunstan <andrew@dunslane.net> writes: > On 11/23/21 12:03, Tom Lane wrote: >> If you set PERL then it's highly likely that you want to use >> "prove" from the same installation. So 0001 modifies configure >> to first check for an executable "prove" in the same directory >> as $PERL. If that's not what you want then you should override >> it by setting PROVE explicitly. > Do we really have much of an issue left to solve given c4fe3199a? It > feels a bit like a solution in search of a problem. We don't absolutely have to do this, agreed. But I think the current behavior fails to satisfy the POLA. As I remarked in the other thread, I'm worried about somebody wasting time trying to identify why their TAP test isn't behaving the way it does when they invoke the code under the perl they think they're using. regards, tom lane
On Tue, Nov 23, 2021 at 12:03:05PM -0500, Tom Lane wrote: > Attached are a couple of patches I propose in the wake of commit > 405f32fc4 (Require version 0.98 of Test::More for TAP tests). > > 0001 responds to the failure we saw on buildfarm member wrasse [1] > where, despite configure having carefully checked for Test::More > being >= 0.98, actual tests failed with > Test::More version 0.98 required--this is only version 0.92 at /export/home/nm/farm/studio64v12_6/HEAD/pgsql.build/../pgsql/src/test/perl/PostgreSQL/Test/Utils.pmline 63. > The reason is that wrasse was choosing "prove" from a different > Perl installation than "perl", as a result of its configuration > having set PERL to a nondefault place but doing nothing about PROVE. > > We already installed a couple of mitigations for that: > (a) as of c4fe3199a, configure checks "prove" not "perl" for > appropriate module versions; > (b) Noah has modified wrasse's configuration to set PROVE. > But I'm of the opinion that (b) should not be necessary. > If you set PERL then it's highly likely that you want to use > "prove" from the same installation. My regular development system is a counterexample. It uses system Perl, but it has a newer "prove" from CPAN: $ grep -E '(PERL|PROVE)' config.status S["PROVE"]="/home/nm/sw/cpan/bin/prove" S["PERL"]="/usr/bin/perl" The patch sends it back to using the system "prove": S["PROVE"]="/usr/bin/prove" S["PERL"]="/usr/bin/perl" I could, of course, override that. But with so little evidence about systems helped by the proposed change, I'm now -1.0 on it. > 0002 is written to apply to v14 and earlier, and what it wants > to do is back-patch the effects of 405f32fc4, so that the > minimum Test::More version is 0.98 in all branches. The thought > here is that (1) somebody is likely to want to back-patch a > test involving Test::More::subtest before too long; (2) we have > zero coverage for older Test::More versions anyway, now that > all buildfarm members have been updated to work with HEAD. wrasse v10..v14 is testing older Test::More, so coverage persists. However, I am okay with this change.
On 23.11.21 18:03, Tom Lane wrote: > 0002 is written to apply to v14 and earlier, and what it wants > to do is back-patch the effects of 405f32fc4, so that the > minimum Test::More version is 0.98 in all branches. The thought > here is that (1) somebody is likely to want to back-patch a > test involving Test::More::subtest before too long; (2) we have > zero coverage for older Test::More versions anyway, now that > all buildfarm members have been updated to work with HEAD. The backpatching argument is a little off-target, I think. The purpose of subtests is so that wrappers like command_fails_like() count only as one test on the outside, instead of however many checks it runs internally. There is no use in using the subtest feature in a top-level test one might add, say in response to a bugfix. So the only reason this might become relevant is if someone were to backpatch new test infrastructure, which seems rare and in most cases would probably require additional portability and backpatching care. And even then, you still need to adjust the test count at the top of the file individually in each branch, because the total number of tests will probably be different. In my mind, the subtests feature is useful during new development, where you write a bunch of new tests and want to set the total test count by eyeballing what you just wrote instead of having to run test cycles and react to the complaints. But it won't be useful for patching tests into existing files. In summary, I would leave it alone.