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

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.

> =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?

typo: testfailure -> test failure

> diff --git a/src/test/recovery/t/031_recovery_conflict.pl b/src/test/recovery/t/031_recovery_conflict.pl
> index d87efa823fd..62936f52d20 100644
> --- a/src/test/recovery/t/031_recovery_conflict.pl
> +++ b/src/test/recovery/t/031_recovery_conflict.pl
> @@ -253,9 +253,7 @@ $res = $psql_standby->query_until(
> -- wait for lock held by prepared transaction
> SELECT * FROM $table2;
> ]);
> -ok(1,
> - "$sect: cursor holding conflicting pin, also waiting for lock, established"
> -);
> +isnt($res, undef, "$sect: cursor holding conflicting pin, also waiting for lock, established");
>
> # just to make sure we're waiting for lock already
> ok( $node_standby->poll_query_until(
> diff --git a/src/test/recovery/t/037_invalid_database.pl b/src/test/recovery/t/037_invalid_database.pl
> index 6d1c7117964..c8c20077f85 100644
> --- a/src/test/recovery/t/037_invalid_database.pl
> +++ b/src/test/recovery/t/037_invalid_database.pl
> @@ -94,6 +94,7 @@ is($node->psql('postgres', 'DROP DATABASE regression_invalid'),
> my $cancel = $node->background_psql('postgres', on_error_stop => 1);
> my $bgpsql = $node->background_psql('postgres', on_error_stop => 0);
> 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.

> The tab completion test can use the API call for automatically restart the
> timer to reduce the complexity of check_completion a hair. Done in 0001 (but
> really not necessary).

+1

> Commit Af279ddd1c2 added this sequence to 040_standby_failover_slots_sync.pl in
> the recovery tests:
>
> $back_q->query_until(
> qr/logical_slot_get_changes/, q(
> \echo logical_slot_get_changes
> SELECT pg_logical_slot_get_changes('test_slot', NULL, NULL);
> ));
>
> ... <other tests> ...
>
> # Since there are no slots in standby_slot_names, the function
> # pg_logical_slot_get_changes should now return, and the session can be
> # stopped.
> $back_q->quit;
>
> There is no guarantee that pg_logical_slot_get_changes has returned when
> reaching this point. This might still work as intended, but the comment is
> slightly misleading IMO.

Agreed, it would be good to actually check that it returns.

> recovery/t/043_wal_replay_wait.pl calls pg_wal_replay_wait() since 06c418e163e
> in a background session which it then skips terminating. Calling ->quit is
> mandated by the API, in turn required by IPC::Run. Calling ->quit on the
> process makes the test fail from the process having already exited, but we can
> call ->finish directly like we do in test_misc/t/005_timeouts.pl. 0003 fixes
> this.

Alexander included this fix in commit 3c5db1d6b016 already.

--
Heikki Linnakangas
Neon (https://neon.tech)