On Fri Dec 19, 2025 at 3:18 PM CET, Xuneng Zhou wrote:
> Thanks for working on this. I’ve done an initial review of patch 4 and
> here are some comments below.
Attached is a version where I addressed all of those comemnts (the few
that I didn't or did in non-obvious ways are discussed at the end). I
also made a lot of improvements to the patchset:
1. Includes infrastructure to create multiple Postgres clusters in a
single test or module.
2. Basic documentation for the pytest testing infrastructure.
3. Configure pytest and dependencies in pyproject.toml instead of a
requirements file.
4. Set up the environment using "uv"[1] if no pytest binary can be
found and uv is found.
5. Use INITDB_TEMPLATE to speed up cluster creation.
6. Don't enable fsync during tests to speed them up.
7. I added a patch where I've rewritten the libpq load_balance_hosts
tests in python as a validation that the new infrastructure and APIs
work well. (the perl ones were also written by me)
8. Postgres logs are now included as a separate pytest "section" in the
output.
9. The pgtap output now includes all of the pytest sections. For an
example of what the failure output looks like, take a look here[2]
I also *removed* a few things that Jacob had initially added:
1. The SCRAM based windows auth. I think it's a good idea, but it
doesn't work with INITDB_TEMPLATE. I think that logic should be made
part of a separate patchset that stops use trust auth when creating
the INITDB_TEMPLATE. That way also the perl tests can benefit from it.
So seems good to do, but separate from the whole pytest work.
2. I removed the current_windows_user() function. This was dead code (as
also written in Jacob's comment) and python has built-in ways to get the current user.
3. I removed the fancy missing/incorrect dependency detection script. I
think (as Jacob also suggested in his code comments) that
importorskip is a better fit for this. Especially since we only have
pytest as a dependency for the core framework, and only the
cryptography package for the ssl tests.
Finally, I prepared a PR for our images to include the pytest
dependencies, so in a future version of the patchset we don't need to
ad-hoc install the required packages.
IMO patch 4 now serves as a good enough central infrastructure base for
people to develop tests on top of (which would possibly add some more
infrastructure as needed).
In regards to your second message, related to consensus on the
necessity of the project: Yes, based on the in-person conversations
about this people are either in favor of a pytest based test framework,
or neutral. There were no strong objections. We were now at the point
where "someone" now actually needs to do the work of getting some decent
infrastructure in place. Which is what Jacob and me have been trying to
do.
[1]: https://github.com/astral-sh/uv
[2]: https://cirrus-ci.com/task/4805772426608640?logs=test_world#L16
[3]: https://github.com/anarazel/pg-vm-images/pull/130
> 4) Type conversion: timestamp/timestamptz conversion could be wrong
> datetime.datetime.fromisoformat for both timestamp and timestamptz.
I now configured datestyle to ISO and UTC in postgresql.conf. If that
turns out not to be enough at some point (e.g. because a test sets a
different datestyle), we can revisit this.
> 6) Logging config: log_connections = all seems wrong
> print("log_connections = all", file=f)
>
> I don't see an option "all" for this parameter
> https://postgresqlco.nf/doc/en/param/log_connections/
That's a new value since PG18:
https://postgresqlco.nf/doc/en/param/log_connections/18/
> 7) UX: error message handling and query attachment
> raise_error() builds message with primary + optional Query: ....
>
> Should we include SQLSTATE and severity in the message string by
> default, because it helps when reading CI logs.
I don't think adding this additional info helps much anymore, now that
the postgres logs (item 8) are part of the output too. So I left it like
this, and even removed the query from the error message. By only
including the actual error message it's easier to match on it for errors
that a test expects. Matching on the SQLSTATE can be also done by
matching on the error type.
