Re: RFC: adding pytest as a supported test framework

On 2025-11-10 22:11:50 +0100, Jelte Fennema-Nio wrote:
> On Wed Oct 22, 2025 at 2:44 PM CEST, Jelte Fennema-Nio wrote:
> > So here's your patchset with an additional commit on top that does a
> > bunch of refactoring/renaming and adding features.
>
> Rebased to fix conflicts.

I assume this intentionally doesn't pass CI:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6045

> From f6823405eb994d457f8123df0d417ca2340e4c71 Mon Sep 17 00:00:00 2001
> From: Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>
> Date: Fri, 5 Sep 2025 16:39:08 -0700
> Subject: [PATCH v3 01/10] meson: Include TAP tests in the configuration
> summary
>
> ...to make it obvious when they've been enabled. prove is added to the
> executables list for good measure.
>
> TODO: does Autoconf need something similar?

I agree with adding tap to the configuration summary, but I don't understand
the prove part, that just seems like a waste of vertical space.

> From 5a27976496db53d8e9b88ab59e6c71f0f42dedcd Mon Sep 17 00:00:00 2001
> From: Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>
> Date: Wed, 13 Aug 2025 10:58:56 -0700
> Subject: [PATCH v3 02/10] Add support for pytest test suites
>
> Specify --enable-pytest/-Dpytest=enabled at configure time. This
> contains no Postgres test logic -- it is just a "vanilla" pytest
> skeleton.
>
> I've written a custom pgtap output plugin, used by the Meson mtest
> runner, to fully control what we see during CI test failures. The
> pytest-tap plugin would have been preferable, but it's now in
> maintenance mode, and it has problems with accidentally suppressing
> important collection failures.
>
> test_something.py is intended to show a sample failure in the CI.
>
> TODOs:
> - OpenBSD has an ANSI-related terminal bug, but I'm not sure if the bug
> is in Cirrus, the image, pytest, Python, or readline. The TERM envvar
> is unset to work around it. If this workaround is removed, a bad ANSI
> escape is inserted into the pgtap output and mtest is unable to parse
> it.
> - The Chocolatey CI setup is subpar. Need to find a way to bless the
> dependencies in use rather than pulling from pip... or maybe that will
> be done by the image baker.

Yes, that needs to be baked into the image. Chocolatey is catastrophically
slow and unreliable. It's also just bad form to hit any service with such
repeated downloads.

This is true for *all* of the platforms.

> +###############################################################
> +# Library: pytest
> +###############################################################
> +
> +pytest_enabled = false
> +pytest = not_found_dep
> +
> +pytestopt = get_option('pytest')
> +if not pytestopt.disabled()
> + pytest = find_program(get_option('PYTEST'), native: true, required: pytestopt)
> + if pytest.found()
> + pytest_check = run_command(pytest,
> + '-c', 'pytest.ini',
> + '--confcutdir=config',
> + '--capture=no',
> + 'config/check_pytest.py',
> + '--requirements', 'config/pytest-requirements.txt',
> + check: false)
> + if pytest_check.returncode() != 0
> + message(pytest_check.stderr())
> + if pytestopt.enabled()
> + error('Additional Python packages are required to run the pytest suites.')
> + else
> + warning('Additional Python packages are required to run the pytest suites.')
> + endif
> + else
> + pytest_enabled = true
> + endif
> + endif
> +endif

Why do we need pytest the program at all? Running the tests one-by-one with
pytest as a runner doesn't seem to make a whole lot of sense to me.

> diff --git a/src/test/Makefile b/src/test/Makefile
> index 511a72e6238..0be9771d71f 100644
> --- a/src/test/Makefile
> +++ b/src/test/Makefile
> @@ -12,7 +12,16 @@ subdir = src/test
> top_builddir = ../..
> include $(top_builddir)/src/Makefile.global
>
> -SUBDIRS = perl postmaster regress isolation modules authentication recovery subscription
> +SUBDIRS = \
> + authentication \
> + isolation \
> + modules \
> + perl \
> + postmaster \
> + pytest \
> + recovery \
> + regress \
> + subscription

I'm onboard with that, but we should do it separately and probably check for
other cases where we should do it at the same time.

I think it'd be a seriously bad idea to start with no central infrastructure,
we'd be force to duplicate that all over. Eventually we'll be forced to
introduce some central infrastructure, but we'll probably not go around and
carefully go through the existing tests for stuff that should now use the
common infrastructure.

Greetings,

Andres Freund