Thread: Make more portable TAP tests of initdb
Hi all, I noticed that src/bin/initdb/t/001_initdb.pl uses directly rm via a system() call like that: system_or_bail "rm -rf '$tempdir'/*"; This way of doing is not portable, particularly on platforms that do not have rm like... Windows where the equivalent is del. And we could actually use remove_tree with its option keep_root to get the same effect in pure perl as mentioned here: http://perldoc.perl.org/File/Path.html With this formulation: remove_tree($tempdir, {keep_root => 1}); Attached is a patch doing that. Regards, -- Michael
Attachment
Michael Paquier wrote: > Hi all, > > I noticed that src/bin/initdb/t/001_initdb.pl uses directly rm via a > system() call like that: > system_or_bail "rm -rf '$tempdir'/*"; > > This way of doing is not portable, particularly on platforms that do > not have rm like... Windows where the equivalent is del. And we could > actually use remove_tree with its option keep_root to get the same > effect in pure perl as mentioned here: > http://perldoc.perl.org/File/Path.html > With this formulation: > remove_tree($tempdir, {keep_root => 1}); Does Perl 5.8 have this? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Apr 14, 2015, at 9:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> http://perldoc.perl.org/File/Path.html >> With this formulation: >> remove_tree($tempdir, {keep_root => 1}); > > Does Perl 5.8 have this? Yes, it does. http://cpansearch.perl.org/src/NWCLARK/perl-5.8.9/lib/File/Path.pm Best, David
On Tue, Apr 14, 2015 at 09:25:33AM -0700, David E. Wheeler wrote: > On Apr 14, 2015, at 9:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > >> http://perldoc.perl.org/File/Path.html > >> With this formulation: > >> remove_tree($tempdir, {keep_root => 1}); > > > > Does Perl 5.8 have this? > > Yes, it does. > > http://cpansearch.perl.org/src/NWCLARK/perl-5.8.9/lib/File/Path.pm One way to avoid a lot of these questions would be to use Test::MinimumVersion or similar in a git hook. If we're promising 5.8.mumble, we should be able to keep that promise always rather than search case by case, at least up to bugs in the version-checking software. http://search.cpan.org/~rjbs/Test-MinimumVersion-0.101081/lib/Test/MinimumVersion.pm What say? Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
David E. Wheeler wrote: > On Apr 14, 2015, at 9:05 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > >> http://perldoc.perl.org/File/Path.html > >> With this formulation: > >> remove_tree($tempdir, {keep_root => 1}); > > > > Does Perl 5.8 have this? > > Yes, it does. > > http://cpansearch.perl.org/src/NWCLARK/perl-5.8.9/lib/File/Path.pm Uh. 5.8.9 does, but 5.8.8 does not. Not sure if this is a problem. Ancient buildfarm member pademelon (Tom's HPUX stuff) has 5.8.9 -- but that was probably installed by hand. Brolga (really ancient Cygwin stuff) has 5.10.1. As far as I can tell (perldoc perldelta5101) it was upgraded to File::Path 2.07_03 which should be fine. Spoonbill has 5.12.2; 5.12.0 already included the fixes in 5.10.1 so it should be fine also. Friarbird has 5.12.4. Castoroides has 5.8.4. Oops. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Castoroides has 5.8.4. Oops. WUT.
David E. Wheeler wrote: > On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > Castoroides has 5.8.4. Oops. > > WUT. Yeah, eh? Anyway I don't think it matters much: just don't enable TAP tests on machines with obsolete Perl. I think this is fine since 5.8's latest release is supported. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Apr 15, 2015 at 5:29 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > David E. Wheeler wrote: >> On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> >> > Castoroides has 5.8.4. Oops. >> >> WUT. > > Yeah, eh? Anyway I don't think it matters much: just don't enable TAP > tests on machines with obsolete Perl. I think this is fine since 5.8's > latest release is supported. And castoroides is not using --enable-tap-tests in the buildfarm btw. -- Michael
On Tue, Apr 14, 2015 at 05:29:36PM -0300, Alvaro Herrera wrote: > David E. Wheeler wrote: > > On Apr 14, 2015, at 1:21 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > > Castoroides has 5.8.4. Oops. > > > > WUT. > > Yeah, eh? Anyway I don't think it matters much: just don't enable TAP > tests on machines with obsolete Perl. I think this is fine since 5.8's > latest release is supported. Solaris 10 ships Perl 5.8.4, and RHEL 5.11 ships Perl 5.8.8. Therefore, Perl installations lacking this File::Path feature will receive vendor support for years to come. Replacing the use of keep_root with rmtree+mkdir will add 2-10 lines of code, right? Let's do that and not raise the system requirements. Thanks, nm
On Wed, Apr 15, 2015 at 2:38 PM, Noah Misch wrote: > Solaris 10 ships Perl 5.8.4, and RHEL 5.11 ships Perl 5.8.8. Therefore, Perl > installations lacking this File::Path feature will receive vendor support for > years to come. Replacing the use of keep_root with rmtree+mkdir will add 2-10 > lines of code, right? Let's do that and not raise the system requirements. Good point. Let's use mkdir in combination then. Attached is an updated patch. -- Michael
Attachment
On Wed, Apr 15, 2015 at 02:59:55PM +0900, Michael Paquier wrote: > On Wed, Apr 15, 2015 at 2:38 PM, Noah Misch wrote: > > Solaris 10 ships Perl 5.8.4, and RHEL 5.11 ships Perl 5.8.8. Therefore, Perl > > installations lacking this File::Path feature will receive vendor support for > > years to come. Replacing the use of keep_root with rmtree+mkdir will add 2-10 > > lines of code, right? Let's do that and not raise the system requirements. > > Good point. Let's use mkdir in combination then. Attached is an updated patch. > --- a/src/bin/initdb/t/001_initdb.pl > +++ b/src/bin/initdb/t/001_initdb.pl > @@ -1,6 +1,7 @@ > use strict; > use warnings; > use TestLib; > +use File::Path qw(rmtree); > use Test::More tests => 19; > > my $tempdir = TestLib::tempdir; > @@ -18,27 +19,30 @@ command_fails([ 'initdb', '-S', "$tempdir/data3" ], > mkdir "$tempdir/data4" or BAIL_OUT($!); > command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory'); > > -system_or_bail "rm -rf '$tempdir'/*"; > - > +rmtree($tempdir); > +mkdir $tempdir; It's usually wrong to remove and recreate the very directory made by File::Temp. Doing so permits a race condition: an attacker can replace the directory between the rmdir() and the mkdir(). However, TestLib::tempdir returns a subdirectory of the build directory, and the build directory is presumed secure. That's good enough. > -system_or_bail "rm -rf '$tempdir'/*"; > +rmtree($tempdir); > command_ok([ 'initdb', '-T', 'german', "$tempdir/data" ], > 'select default dictionary'); You omitted the mkdir() on that last one. It works, since initdb does the equivalent of "mkdir -p", but it looks like an oversight. As I pondered this, I felt it would do better to solve a different problem. The "rm -rf" invocations presumably crept in to reduce peak disk usage. Considering the relatively-high duration of a successful initdb run, I doubt we get good value from so many positive tests. I squashed those positive tests into one, as attached. (I changed the order of negative tests but kept them all.) This removed the need for intra-script cleaning, and it accelerated script runtime from 22s to 9s. Does this seem good to you? Thanks, nm
Attachment
On Thu, Apr 30, 2015 at 12:17 PM, Noah Misch <noah@leadboat.com> wrote: > On Wed, Apr 15, 2015 at 02:59:55PM +0900, Michael Paquier wrote: >> On Wed, Apr 15, 2015 at 2:38 PM, Noah Misch wrote: >> > Solaris 10 ships Perl 5.8.4, and RHEL 5.11 ships Perl 5.8.8. Therefore, Perl >> > installations lacking this File::Path feature will receive vendor support for >> > years to come. Replacing the use of keep_root with rmtree+mkdir will add 2-10 >> > lines of code, right? Let's do that and not raise the system requirements. >> >> Good point. Let's use mkdir in combination then. Attached is an updated patch. > >> --- a/src/bin/initdb/t/001_initdb.pl >> +++ b/src/bin/initdb/t/001_initdb.pl >> @@ -1,6 +1,7 @@ >> use strict; >> use warnings; >> use TestLib; >> +use File::Path qw(rmtree); >> use Test::More tests => 19; >> >> my $tempdir = TestLib::tempdir; >> @@ -18,27 +19,30 @@ command_fails([ 'initdb', '-S', "$tempdir/data3" ], >> mkdir "$tempdir/data4" or BAIL_OUT($!); >> command_ok([ 'initdb', "$tempdir/data4" ], 'existing empty data directory'); >> >> -system_or_bail "rm -rf '$tempdir'/*"; >> - >> +rmtree($tempdir); >> +mkdir $tempdir; > > It's usually wrong to remove and recreate the very directory made by > File::Temp. Doing so permits a race condition: an attacker can replace the > directory between the rmdir() and the mkdir(). However, TestLib::tempdir > returns a subdirectory of the build directory, and the build directory is > presumed secure. That's good enough. OK, I haven't thought of that. >> -system_or_bail "rm -rf '$tempdir'/*"; >> +rmtree($tempdir); >> command_ok([ 'initdb', '-T', 'german', "$tempdir/data" ], >> 'select default dictionary'); > > You omitted the mkdir() on that last one. It works, since initdb does the > equivalent of "mkdir -p", but it looks like an oversight. > > > As I pondered this, I felt it would do better to solve a different problem. > The "rm -rf" invocations presumably crept in to reduce peak disk usage. > Considering the relatively-high duration of a successful initdb run, I doubt > we get good value from so many positive tests. I squashed those positive > tests into one, as attached. (I changed the order of negative tests but kept > them all.) This removed the need for intra-script cleaning, and it > accelerated script runtime from 22s to 9s. Does this seem good to you? That's a fight code simplicity vs. test performance. FWIW, I like the test suite with many positive tests because it is easy to read the test flow. And as this test suite is meant to grow up with new tests in the future, I am guessing that people may not follow the restriction this patch adds all the time. command_fails( - [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ], + [ 'initdb', $datadir, '-X', 'pgxlog' ], 'relative xlog directory not allowed'); $datadir should be the last argument. This would break on platforms where src/port/getopt_long.c is used, like Windows. +command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ], + 'successful creation'); This is grouping the test for nosync, existing nonempty xlog directory, and the one for selection of the default dictionary. Shouldn't this test name be updated otherwise then? Could you add a comment mentioning that tests are grouped to reduce I/O impact during a run? Regards, -- Michael
On Fri, May 01, 2015 at 03:23:44PM +0900, Michael Paquier wrote: > On Thu, Apr 30, 2015 at 12:17 PM, Noah Misch <noah@leadboat.com> wrote: > > As I pondered this, I felt it would do better to solve a different problem. > > The "rm -rf" invocations presumably crept in to reduce peak disk usage. > > Considering the relatively-high duration of a successful initdb run, I doubt > > we get good value from so many positive tests. I squashed those positive > > tests into one, as attached. (I changed the order of negative tests but kept > > them all.) This removed the need for intra-script cleaning, and it > > accelerated script runtime from 22s to 9s. Does this seem good to you? > > That's a fight code simplicity vs. test performance. FWIW, I like the > test suite with many positive tests because it is easy to read the > test flow. And as this test suite is meant to grow up with new tests > in the future, I am guessing that people may not follow the > restriction this patch adds all the time. True. > command_fails( > - [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ], > + [ 'initdb', $datadir, '-X', 'pgxlog' ], > 'relative xlog directory not allowed'); > $datadir should be the last argument. This would break on platforms > where src/port/getopt_long.c is used, like Windows. I committed that as a separate patch; thanks. > +command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ], > + 'successful creation'); > This is grouping the test for nosync, existing nonempty xlog > directory, and the one for selection of the default dictionary. > Shouldn't this test name be updated otherwise then? > > Could you add a comment mentioning that tests are grouped to reduce > I/O impact during a run? Committed with a comment added. The "successful creation" test name is generic because it's the designated place to add testing of new options.
On Sun, May 3, 2015 at 8:09 AM, Noah Misch <noah@leadboat.com> wrote: > On Fri, May 01, 2015 at 03:23:44PM +0900, Michael Paquier wrote: >> On Thu, Apr 30, 2015 at 12:17 PM, Noah Misch <noah@leadboat.com> wrote: >> > As I pondered this, I felt it would do better to solve a different problem. >> > The "rm -rf" invocations presumably crept in to reduce peak disk usage. >> > Considering the relatively-high duration of a successful initdb run, I doubt >> > we get good value from so many positive tests. I squashed those positive >> > tests into one, as attached. (I changed the order of negative tests but kept >> > them all.) This removed the need for intra-script cleaning, and it >> > accelerated script runtime from 22s to 9s. Does this seem good to you? >> >> That's a fight code simplicity vs. test performance. FWIW, I like the >> test suite with many positive tests because it is easy to read the >> test flow. And as this test suite is meant to grow up with new tests >> in the future, I am guessing that people may not follow the >> restriction this patch adds all the time. > > True. > >> command_fails( >> - [ 'initdb', "$tempdir/data", '-X', 'pgxlog' ], >> + [ 'initdb', $datadir, '-X', 'pgxlog' ], >> 'relative xlog directory not allowed'); >> $datadir should be the last argument. This would break on platforms >> where src/port/getopt_long.c is used, like Windows. > > I committed that as a separate patch; thanks. > >> +command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ], >> + 'successful creation'); >> This is grouping the test for nosync, existing nonempty xlog >> directory, and the one for selection of the default dictionary. >> Shouldn't this test name be updated otherwise then? >> >> Could you add a comment mentioning that tests are grouped to reduce >> I/O impact during a run? > > Committed with a comment added. The "successful creation" test name is > generic because it's the designated place to add testing of new options. Thanks. The overall result looks good to me. -- Michael