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

On 2024-10-31 Th 6:18 PM, Heikki Linnakangas wrote:
> On 31/10/2024 14:27, Daniel Gustafsson wrote:
>>> On 28 Oct 2024, at 11:56, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>>>
>>> On 09/04/2024 20:10, Daniel Gustafsson wrote:
>>>> =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().
>
> I see.
>
>> @@ -148,7 +148,9 @@ sub _wait_connect
>>  =item $session->quit
>>
>>  Close the session and clean up resources. Each test run must be
>> closed with
>> -C<quit>.
>> +C<quit>.  Returns TRUE when the session was cleanly terminated,
>> otherwise
>> +FALSE.  A test failure will be issued in case the session exited
>> with a non-
>> +zero exit status (the finish() method returns TRUE for 0 exit status).
>
> I still find that confusing. What finish() method? Yes, there's a
> finish() method in IPC::Run, but that's BackgroundPsql's internal
> affair, not exposed to the callers in any other way. And why do I care
> what that finish() returns for 0 exit status? That's not visible to
> the quit method's caller.
>
> Perhaps sommething like this:
>
> "Close the psql session and clean up resources. Each psql session must
> be closed with C<quit> before the end of the test.
> Returns TRUE if psql exited successfully (i.e. with zero exit code),
> otherwise returns FALSE and reports a test failure. "
>
> Would that be accurate?
>

I would be OK with Heikki's version.

The patches have bitrotted slightly.

Also this is wrong, I think:

    isnt($self->{timeout}->is_expired, 'psql query_until timed out');

I think it should be

    ok(! $self->{timeout}->is_expired, 'psql query_until did not time
out');

isnt() normally takes 3 arguments, and the message you have seems
backwards.

cheers

andrew

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