Re: Implement generalized sub routine find_in_log for tap test

On Fri, 26 May 2023 at 04:09, Michael Paquier <michael@paquier.xyz> wrote:

On Thu, May 25, 2023 at 06:34:20PM +0100, Dagfinn Ilmari Mannsåker wrote:

However, none of the other functions in ::Utils know anything about node
objects, which makes me think it should be a method on the node itself
(i.e. in PostgreSQL::Test::Cluster) instead.  Also, I think log_contains
would be a better name, since it just returns a boolean.  The name
find_in_log makes me think it would return the log lines matching the
pattern, or the position of the match in the file.

In that case, the slurp_file() call would have to be fully qualified,
since ::Cluster uses an empty import list to avoid polluting the method
namespace with imported functions.

Hmm.  connect_ok() and connect_fails() in Cluster.pm have a similar
log comparison logic, feeding from the offset of a log file.  Couldn't
you use the same code across the board for everything?  Note that this
stuff is parameterized so as it is possible to check if patterns match
or do not match, for multiple patterns.  It seems to me that we could
use the new log finding routine there as well, so how about extending
it a bit more?  You would need, at least:
- One parameter for log entries matching.
- One parameter for log entries not matching.

I felt adding these to log_contains was making the function slightly
complex with multiple checks. I was not able to make it simple with
the approach I tried. How about having a common function
check_connect_log_contents which has the common log contents check for
connect_ok and connect_fails function like the v2-0002 patch attached.

+    $offset = 0 unless defined $offset;

This is unnecessary, as slurp_file() handles it appropriately, and in fact doing this is slightly inefficient, as it will cause slurp_file to do a redundant seek.

FYI there's a simpler way to say it if we wanted to:

    $offset //= 0;

cheers

andrew

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