Re: IPC::Run::time[r|out] vs our TAP tests

> On 28 Oct 2024, at 11:56, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> 
> On 09/04/2024 20:10, Daniel Gustafsson wrote:
>> Turning the timeout into a timer and returning undef along with logging a test
>> failure in case of expiration seems a bit saner (maybe Andrew can suggest an
>> API which has a better Perl feel to it).  Most callsites don't need any changes
>> to accommodate for this, the attached 0002 implements this timer change and
>> modify the few sites that need it, converting one to plain query() where the
>> added complexity of query_until isn't required.
> 
> +1. The patch looks good to me. I didn't comb through the tests to check for bugs of omission, i.e. cases where the caller would need adjustments because of this, I trust that you found them all.

Thanks for review!

>> =item $session->quit
>> Close the session and clean up resources. Each test run must be closed with
>> C<quit>.  Returns TRUE when the session was cleanly terminated, otherwise
>> FALSE.  A testfailure will be issued in case the session failed to finish.
> 
> What does "session failed to finish" mean? Does it mean the same as "not cleanly terminated", i.e. a test failure is issued whenever this returns FALSE?

It was very literally referring to the finish() method.  I've reworded the
comment to indicated that it throws a failure in case the process returns a
non-zero exit status to finish().

> typo: testfailure -> test failure

Fixed.

>> my $pid = $bgpsql->query('SELECT pg_backend_pid()');
>> +isnt($pid, undef, 'Get backend PID');
>>  # create the database, prevent drop database via lock held by a 2PC transaction
>> ok( $bgpsql->query_safe(
> 
> I'm not sure I understand these changes. These can only fail if the query() or query_until() function times out, right? In that case, the query() or query_until() would also report a test failure, so these additional checks after the call seem redundant. They don't do any harm either, but I wonder why have them in these particular call sites and not in other query() or query_until() calls.

Fair point, they were added to provide additional detail in case of failure,
but they are to some degree overzealous and definitely not required.

Attached is a v2 with the above changes and 0003 dropped due to already being
implemented.

--
Daniel Gustafsson