Re: Make more portable TAP tests of initdb

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