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

On 2024-04-04 Th 17:24, Tom Lane wrote:
> TIL that IPC::Run::timer is not the same as IPC::Run::timeout.
> With a timer object you have to check $timer->is_expired to see
> if the timeout has elapsed, but with a timeout object you don't
> because it will throw a Perl exception upon timing out, probably
> killing your test program.
>
> It appears that a good chunk of our TAP codebase has not read this
> memo, because I see plenty of places that are checking is_expired
> in the naive belief that they'll still have control after a timeout
> has fired.
>

I started having a look at these.

Here are the cases I found:

./src/bin/psql/t/010_tab_completion.pl:    my $okay = ($out =~ $pattern
&& !$h->{timeout}->is_expired);
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm:      until
$self->{stdout} =~ /$banner/ || $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm:    die "psql startup
timed out" if $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm:    die "psql query
timed out" if $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm:    die "psql query
timed out" if $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/Cluster.pm:            # timeout, which
we'll handle by testing is_expired
./src/test/perl/PostgreSQL/Test/Cluster.pm:              unless
$timeout->is_expired;
./src/test/perl/PostgreSQL/Test/Cluster.pm:timeout is the
IPC::Run::Timeout object whose is_expired method can be tested
./src/test/perl/PostgreSQL/Test/Cluster.pm:            # timeout, which
we'll handle by testing is_expired
./src/test/perl/PostgreSQL/Test/Cluster.pm:              unless
$timeout->is_expired;
./src/test/perl/PostgreSQL/Test/Utils.pm:        if ($timeout->is_expired)
./src/test/recovery/t/021_row_visibility.pl:        if
($psql_timeout->is_expired)
./src/test/recovery/t/032_relfilenode_reuse.pl:        if
($psql_timeout->is_expired)

Those in Cluster.pm look correct - they are doing the run() in an eval
block and testing for the is_expired setting in an exception block. The
other cases look more suspect. I'll take a closer look.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com