Re: Make more portable TAP tests of initdb

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