Re: Improving log capture of TAP tests with IPC::Run

On 07/09/2015 04:50 AM, Michael Paquier wrote:
> On Thu, Jul 9, 2015 at 12:49 AM, Heikki Linnakangas wrote:
>>>> Looking at the manual page of Test::More, it looks like you could change
>>>> where the perl script's STDOUT and STDERR point to, because Test::More
>>>> takes
>>>> a copy of them (when? at program startup I guess..). That would be much
>>>> more
>>>> convenient than decorating every run call with ">> logfile".
>>>
>>>
>>> Hm. There are two types of logs we want to capture:
>>> 1) stdout and stderr from the subprocesses kicked by IPC::Run::run
>>> 2) Status messages written in the log file by the process running the
>>> tests.
>>> Perhaps we could redirect the output of stdout and stderr but I think
>>> that this is going to need an fd open from the beginning of the test
>>> until the end, with something like that:
>>> open my $test_logfile_fd, '>>', $test_logfile;
>>> *STDOUT = $test_logfile_fd;
>>> *STDERR = $test_logfile_fd;
>>>
>>> While that would work on OSX and Linux for sure, I suspect that this
>>> will not on Windows where two concurrent processes cannot write to the
>>> same file.
>>
>> Hmm, as long as you make sure all the processes use the same filehandle,
>> rather than open the log file separately, I think it should work. But it's
>> Windows, so who knows..
>
> And actually your patch works even on Windows. Tests are running and
> log can be captured in the same shape as Linux and OSX!

Great!

>> I came up with the attached, which does that. It also plays some tricks with
>> perl "tie", to copy the "ok - ..." lines that go to the console, to the log.
>>
>> I tried to test that on my Windows system, but I don't have IPC::Run
>> installed. How did you get that on Windows? Can you test this?
>
> I simply downloaded them from CPAN and put them in PERL5LIB. And it
> worked. For Windows, you will also need some adjustments to make the
> tests able to run (see the other thread related to support TAP in MSVC
> http://www.postgresql.org/message-id/CAB7nPqTQwphkDfZP07w7yBnbFNDhW_JBAMyCFAkarE2VWg8irQ@mail.gmail.com)
> like using SSPI for connection, adjusting listen_addresses. The patch
> attached, which is a merge of your patch and my MSVC stuff, is able to
> do that. This is just intended for testing, so feel free to use it if
> you want to check by yourself how logs are captured. I'll repost a new
> version of it on the other thread depending on the outcome here.
>
> -    system_or_bail(
> -"pg_basebackup -D $test_standby_datadir -p $port_master -x >>$log_path 2>&1");
> +    system_or_bail('pg_basebackup', '-D', $test_standby_datadir,
> +                   '-p', $port_master, '-x';
> A parenthesis is missing here.
>
> -               my $result = run(
> -                       [   'pg_rewind',
> -                               "--source-server",
> -                               "port=$port_standby dbname=postgres",
> -                               "--target-pgdata=$test_master_datadir" ],
> -                       '>>',
> -                       $log_path,
> -                       '2>&1');
> -               ok($result, 'pg_rewind remote');
> +               command_ok(['pg_rewind',
> +                                       "--source-server",
> +                                       "port=$port_standby dbname=postgres",
> +                                       "--target-pgdata=$test_master_datadir"],
> +                                  'pg_rewind remote');
> As long as we are on it, I think that we should add --debug here to
> make the logs more useful.
>
> Except that this patch looks good to me. Thanks for the black magic on
> stdout/stderr handling.

Thanks, fixed the parenthesis and committed. The missing --debug is a 
separate issue.

- Heikki