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.