> On 28 Oct 2024, at 11:56, Heikki Linnakangas <hlinn...@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
v2-0001-Configure-interactive-instance-to-restart-timer.patch
Description: Binary data
v2-0002-Report-test-failure-rather-than-aborting-in-case-.patch
Description: Binary data
