Feedback-ID: id4a34324:Fastmail
Date: Tue, 17 Jan 2023 11:56:42 -0800
From: Andres Freund <andres@anarazel.de>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	Thomas Munro <thomas.munro@gmail.com>,
	pgsql-hackers@lists.postgresql.org, Noah Misch <noah@leadboat.com>,
	Michael Paquier <michael@paquier.xyz>,
	Anastasia Lubennikova <lubennikovaav@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>,
	Melanie Plageman <melanieplageman@gmail.com>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	samay sharma <smilingsamay@gmail.com>
Subject: Re: CI and test improvements
Message-ID: <20230117195642.vkanrt4yjeanogqr@awork3.anarazel.de>
References: <20220828171029.GO2342@telsasoft.com>
 <20220828212802.r6eymfffrgr3lxxt@awork3.anarazel.de>
 <20220910200542.GX31833@telsasoft.com>
 <20221104235412.GE16921@telsasoft.com>
 <20221105015946.yrxijqb7h4rqhp6d@awork3.anarazel.de>
 <20221113235303.GA26337@telsasoft.com>
 <20221121224542.p2zapvyvb7objluw@alap3.anarazel.de>
 <20221122225744.GF11463@telsasoft.com>
 <20230104234424.GE3109@telsasoft.com>
 <20230117173509.GV9837@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230117173509.GV9837@telsasoft.com>
Archived-At: <https://www.postgresql.org/message-id/20230117195642.vkanrt4yjeanogqr%40awork3.anarazel.de>
Precedence: bulk

Hi,

On 2023-01-17 11:35:09 -0600, Justin Pryzby wrote:
> The autoconf system runs all tap tests in t/*.pl, but meson requires
> enumerating them in ./meson.build.

Yes. It was a mistake that we ever used t/*.pl for make. For one, it means
that make can't control concurrency meaningfully, due to the varying number of
tests run with one prove instance. It's also the only thing that tied us to
prove, which is one hell of a buggy mess.


> This checks for and finds no missing tests in the current tree:
>
> $ for pl in `find src contrib -path '*/t/*.pl'`; do base=${pl##*/}; dir=${pl%/*}; meson=${dir%/*}/meson.build; grep "$base" "$meson" >/dev/null || echo "$base is missing from $meson"; done

Likely because I do something similar locally.

# prep
m test --list > /tmp/tests.txt

# Check if all tap tests are known to meson
for f in $(git ls-files|grep -E '(t|test)/.*.pl$'|sort);do t=$(echo $f|sed -E -e 's/^.*\/([^/]*)\/(t|test)\/(.*)\.pl$/\1\/\3/');grep -q -L $t /tmp/tests.txt |\
| echo $f;done


# Check if all regression / isolation tests are known to meson
#
# Expected to find plpgsql due to extra 'src' directory level, src/test/mb
# because it's not run anywhere and sepgsql, because that's not tested yet
for d in $(find ~/src/postgresql -type d \( -name sql -or -name specs \) );do t=$(basename $(dirname $d)); grep -q -L $t /tmp/tests.txt || echo $d; done


> However, this finds two real problems and one false-positive with
> missing regress/isolation tests:

Which the above does *not* test for. Good catch.

I'll push the fix for those as soon as tests passed on my personal repo.


> $ for makefile in `find src contrib -name Makefile`; do for testname in `sed -r '/^(REGRESS|ISOLATION) =/!d; s///; :l; /\\\\$/{s///; N; b l}; s/\n//g' "$makefile"`; do meson=${makefile%/Makefile}/meson.build; grep -Fw "$testname" "$meson" >/dev/null || echo "$testname is missing from $meson"; done; done
> guc_privs is missing from src/test/modules/unsafe_tests/meson.build

Yep. That got added during the development of the meson port, so it's not too surprising.


> oldextversions is missing from contrib/pg_stat_statements/meson.build

This one however, is odd. Not sure how that happened.


> $(CF_PGP_TESTS) is missing from contrib/pgcrypto/meson.build

Assume that's the false positive?


> I also tried but failed to write something to warn if "meson test" was
> run with a list of tests but without tmp_install.  Help wanted.

That doesn't even catch the worst case - when there's tmp_install, but it's
too old.

The proper solution would be to make the creation of tmp_install a dependency
of the relevant tests. Unfortunately meson still auto-propages those to
dependencies of the 'all' target (for historical reasons), and creating the
temp install is too slow on some machines to make that tolerable. I think
there's an open PR to change that. Once that's in a released meson version
that's in somewhat widespread use, we should change that.

The other path forward is to allow running the tests without
tmp_install. There's not that much we'd need to allow running directly from
the source tree - the biggest thing is a way to load extensions from a list of
paths. This option is especially attractive because it'd allow to run
individual tests without a fully built sourcetree. No need to build other
binaries when you just want to test psql, or more extremely, pg_test_timing.


> I propose to put something like this into "SanityCheck".

Perhaps we instead could add it as a separate "meson-only" test? Then it'd
fail on developer's machines, instead of later in CI. We could pass the test
information from the 'tests' array, or it could look at the metadata in
meson-info/intro-tests.json

Greetings,

Andres Freund