meson missing test dependencies

Hi,

On Mon, 2 Dec 2024 at 13:11, Peter Eisentraut <peter@eisentraut.org> wrote:

I have noticed that under meson many tests don't have dependencies on
the build artifacts that they are testing. As an example among many, if
you make a source code change in contrib/cube/cube.c (see patch 0001 for
a demo) and then run

make -C contrib/cube check

the test run will reflect the changed code, because the "check" targets
typically depend on the "all" targets. But if you do this under meson with

meson test -C build --suite setup --suite cube

the code will not be rebuilt first, and the test run will not reflect
the changed code.

This seems straightforward to fix, see patch 0002. The meson test setup
has support for this, but it seems not widely used.

Patch 0003 is another example, this time for a TAP-style test.

Is there any reason this was not done yet?

This looks useful. I am not sure why this was not done before.

I applied your patches and the cube example worked. But when I edited
'test_json_parser_perf.c' and 'test_json_parser_incremental.c', meson
did not rebuild. I used the 'meson test -C build --suite setup --suite
test_json_parser' command to test test_json_parser. Did I do something
wrong?

--
Regards,
Nazir Bilal Yavuz
Microsoft

On 02.12.24 11:50, Nazir Bilal Yavuz wrote:

I applied your patches and the cube example worked. But when I edited
'test_json_parser_perf.c' and 'test_json_parser_incremental.c', meson
did not rebuild. I used the 'meson test -C build --suite setup --suite
test_json_parser' command to test test_json_parser. Did I do something
wrong?

Seems to work for me. I edited test_json_parser_incremental.c and then ran

$ meson test -C build --suite setup --suite test_json_parser -v
ninja: Entering directory `/Users/peter/devel/postgresql/postgresql/build'
[2/2] Linking target
src/test/modules/test_json_parser/test_json_parser_incremental
1/7 postgresql:setup / tmp_install
RUNNING
...

Without my patch, you don't get the "Linking target ..." output.

On Mon, Dec 02, 2024 at 01:50:57PM +0300, Nazir Bilal Yavuz wrote:

On Mon, 2 Dec 2024 at 13:11, Peter Eisentraut <peter@eisentraut.org> wrote:

Is there any reason this was not done yet?

This looks useful. I am not sure why this was not done before.

Do you think that it would be possible to automate the addition of the
dependency link between the module and the tests in some way? The
most common case where the only dependency needed by the test is the
module itself would cover a lot of ground if this could be enforced in
some fashion.
--
Michael

Hi,

On Mon, 2 Dec 2024 at 22:27, Peter Eisentraut <peter@eisentraut.org> wrote:

On 02.12.24 11:50, Nazir Bilal Yavuz wrote:

I applied your patches and the cube example worked. But when I edited
'test_json_parser_perf.c' and 'test_json_parser_incremental.c', meson
did not rebuild. I used the 'meson test -C build --suite setup --suite
test_json_parser' command to test test_json_parser. Did I do something
wrong?

Seems to work for me. I edited test_json_parser_incremental.c and then ran

$ meson test -C build --suite setup --suite test_json_parser -v
ninja: Entering directory `/Users/peter/devel/postgresql/postgresql/build'
[2/2] Linking target
src/test/modules/test_json_parser/test_json_parser_incremental
1/7 postgresql:setup / tmp_install
RUNNING
...

Without my patch, you don't get the "Linking target ..." output.

It is working fine now, I must have done something wrong before. Sorry
for the noise.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On Tue, 3 Dec 2024 at 04:05, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Dec 02, 2024 at 01:50:57PM +0300, Nazir Bilal Yavuz wrote:

On Mon, 2 Dec 2024 at 13:11, Peter Eisentraut <peter@eisentraut.org> wrote:

Is there any reason this was not done yet?

This looks useful. I am not sure why this was not done before.

Do you think that it would be possible to automate the addition of the
dependency link between the module and the tests in some way? The
most common case where the only dependency needed by the test is the
module itself would cover a lot of ground if this could be enforced in
some fashion.

I tried something like:

diff --git a/meson.build b/meson.build
index 451c3f6d851..ddf6045d472 100644
--- a/meson.build
+++ b/meson.build
@@ -3366,6 +3366,13 @@ foreach test_dir : tests

t = test_dir[kind]

+    t_dep = [get_variable(test_dir['name'], '')]
+    message('Adding test = @0@ and dep = @1@'.format(test_dir['name'], t_dep))
+    if t_dep != ['']
+      t += {'deps': t_dep}
+    endif
+

The code above creates a dependency between the module (*whose name is
same with the test*) and the test. This errors out for the 'libpq,
ssl, ldap and icu' because the type of these variables is dependency;
but test.depends can be one of the '[BuildTarget | CustomTarget |
CustomTargetIndex]' types, it can not be a dependency type.

And this can not create a link for the 'scripts, regress, isolation,
authentication, postmaster, recovery, subscription, brin, commit_ts,
gin, test_extensions, test_json_parser, test_misc, test_pg_dump,
typcache, unsafe_tests, worker_spi, kerberos and ecpg' tests. I think
only 'regress, isolation, test_json_parser, worker_spi and ecpg' are
wrong in this list as their modules names are not the same with their
tests.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Hi,

On 2024-12-02 11:10:56 +0100, Peter Eisentraut wrote:

I have noticed that under meson many tests don't have dependencies on the
build artifacts that they are testing. As an example among many, if you
make a source code change in contrib/cube/cube.c (see patch 0001 for a demo)
and then run

make -C contrib/cube check

the test run will reflect the changed code, because the "check" targets
typically depend on the "all" targets. But if you do this under meson with

meson test -C build --suite setup --suite cube

the code will not be rebuilt first, and the test run will not reflect the
changed code.

That's unfortunately a very partial fix - because we insist on tests being run
against a temporary install, we don't just need to rebuild the code, we also
need to re-install it. Without that you don't, e.g., see server changes.

Currently the install is done as part of the setup/tmp_install test.

It'd be *much* nicer to create the temporary installation as a build step, but
unfortunately meson currently builds all test dependencies when you build the
default target. Which unfortunately would mean that we'd reinstall the temp
installation whenever 'ninja' is issued. Hence the weird setup with the test
doing the install. There's recent progress towards improving this in meson,
luckily.

However, it looks like the tmp_install test *does* miss dependencies too and I
see no reason to not fix that.

Medium term I think we should work on being able to run our tests from the
source tree. That'd substantially improve the time to run individual tests, I
think.

Greetings,

Andres Freund

diff --git i/meson.build w/meson.build
index ff3848b1d85..55b751a0c6b 100644
--- i/meson.build
+++ w/meson.build
@@ -3263,6 +3263,7 @@ test('tmp_install',
     priority: setup_tests_priority,
     timeout: 300,
     is_parallel: false,
+    depends: all_built,
     suite: ['setup'])

test('install_test_files',

On 03.12.24 17:01, Andres Freund wrote:

Hi,

On 2024-12-02 11:10:56 +0100, Peter Eisentraut wrote:

I have noticed that under meson many tests don't have dependencies on the
build artifacts that they are testing. As an example among many, if you
make a source code change in contrib/cube/cube.c (see patch 0001 for a demo)
and then run

make -C contrib/cube check

the test run will reflect the changed code, because the "check" targets
typically depend on the "all" targets. But if you do this under meson with

meson test -C build --suite setup --suite cube

the code will not be rebuilt first, and the test run will not reflect the
changed code.

That's unfortunately a very partial fix - because we insist on tests being run
against a temporary install, we don't just need to rebuild the code, we also
need to re-install it. Without that you don't, e.g., see server changes.

Right, I was willing to accept that as a compromise, but see below.

However, it looks like the tmp_install test *does* miss dependencies too and I
see no reason to not fix that.

diff --git i/meson.build w/meson.build
index ff3848b1d85..55b751a0c6b 100644
--- i/meson.build
+++ w/meson.build
@@ -3263,6 +3263,7 @@ test('tmp_install',
priority: setup_tests_priority,
timeout: 300,
is_parallel: false,
+    depends: all_built,
suite: ['setup'])

test('install_test_files',

Yes, that addresses my cube example.

But it doesn't address the test_json_parser example, because in that
case the test executables are not installed. So in that case we still
need to manually add the dependencies. But those are only a few cases
(maybe just test_json_parser and libpq_pipeline).

Hi,

On 2024-12-05 20:08:24 +0100, Peter Eisentraut wrote:

On 03.12.24 17:01, Andres Freund wrote:

On 2024-12-02 11:10:56 +0100, Peter Eisentraut wrote:
That's unfortunately a very partial fix - because we insist on tests being run
against a temporary install, we don't just need to rebuild the code, we also
need to re-install it. Without that you don't, e.g., see server changes.

Right, I was willing to accept that as a compromise, but see below.

However, it looks like the tmp_install test *does* miss dependencies too and I
see no reason to not fix that.

diff --git i/meson.build w/meson.build
index ff3848b1d85..55b751a0c6b 100644
--- i/meson.build
+++ w/meson.build
@@ -3263,6 +3263,7 @@ test('tmp_install',
priority: setup_tests_priority,
timeout: 300,
is_parallel: false,
+    depends: all_built,
suite: ['setup'])

test('install_test_files',

Yes, that addresses my cube example.

Let's do that. I'm inclined to only do so on master, but I can also see
backpatching it. Arguments?

But it doesn't address the test_json_parser example, because in that case
the test executables are not installed. So in that case we still need to
manually add the dependencies. But those are only a few cases (maybe just
test_json_parser and libpq_pipeline).

Agreed!

Greetings,

Andres Freund

On 05.12.24 21:10, Andres Freund wrote:

Hi,

On 2024-12-05 20:08:24 +0100, Peter Eisentraut wrote:

On 03.12.24 17:01, Andres Freund wrote:

On 2024-12-02 11:10:56 +0100, Peter Eisentraut wrote:
That's unfortunately a very partial fix - because we insist on tests being run
against a temporary install, we don't just need to rebuild the code, we also
need to re-install it. Without that you don't, e.g., see server changes.

Right, I was willing to accept that as a compromise, but see below.

However, it looks like the tmp_install test *does* miss dependencies too and I
see no reason to not fix that.

diff --git i/meson.build w/meson.build
index ff3848b1d85..55b751a0c6b 100644
--- i/meson.build
+++ w/meson.build
@@ -3263,6 +3263,7 @@ test('tmp_install',
priority: setup_tests_priority,
timeout: 300,
is_parallel: false,
+    depends: all_built,
suite: ['setup'])

test('install_test_files',

Yes, that addresses my cube example.

Let's do that. I'm inclined to only do so on master, but I can also see
backpatching it. Arguments?

master is enough for me.

Hi,

On 2024-12-06 18:54:25 +0100, Peter Eisentraut wrote:

On 05.12.24 21:10, Andres Freund wrote:

On 2024-12-05 20:08:24 +0100, Peter Eisentraut wrote:

On 03.12.24 17:01, Andres Freund wrote:

On 2024-12-02 11:10:56 +0100, Peter Eisentraut wrote:
That's unfortunately a very partial fix - because we insist on tests being run
against a temporary install, we don't just need to rebuild the code, we also
need to re-install it. Without that you don't, e.g., see server changes.

Right, I was willing to accept that as a compromise, but see below.

However, it looks like the tmp_install test *does* miss dependencies too and I
see no reason to not fix that.

diff --git i/meson.build w/meson.build
index ff3848b1d85..55b751a0c6b 100644
--- i/meson.build
+++ w/meson.build
@@ -3263,6 +3263,7 @@ test('tmp_install',
priority: setup_tests_priority,
timeout: 300,
is_parallel: false,
+    depends: all_built,
suite: ['setup'])

test('install_test_files',

Yes, that addresses my cube example.

Let's do that. I'm inclined to only do so on master, but I can also see
backpatching it. Arguments?

master is enough for me.

This doesn't actually make that much difference. But only really kinda by
accident, so I'm still planning to apply a patch improving this.

If a test target does not have ay dependency 'meson test' treats it has having
a dependency on the project default test. Which in turn currently includes
everything that will be installed, i.e. everything that tmp_install might
need. But that's not something I think we should rely on going forward.

Attached is a series that:

- Reduces the dependencies of the 'install-quiet' target, to not build
e.g. ecpg tests

- Fixes the dependencies of tmp_install test more precise

- Fixes dependencies for ecpg's tests

- Fixes dependencies for test_json_parser's (slightly evolved version of
yours)

- Fixes dependencies for libpq_pipeline

- Fixes dependencies for libpq's tests

The last case was a bit less fun to fix.

To have correct dependencies, the libpq tests need a dependency on
libpq_uri_regress and libpq_testclient. But until now that was hard to do,
because src/interfaces/libpq/test is entered from the top-level:

# test/ is entered via top-level meson.build, that way it can use the default
# args for executables (which depend on libpq).

At the top-level we have:
subdir('src/interfaces/libpq')
# fe_utils depends on libpq
subdir('src/fe_utils')

# for frontend binaries
frontend_code = declare_dependency(
include_directories: [postgres_inc],
link_with: [fe_utils, common_static, pgport_static],
sources: generated_headers,
dependencies: [os_deps, libintl],
)

and the test binaries currently use frontend_code.

The least bad fix that I could see was to introduce, at the top-level,
frontend_no_fe_utils_code, which can be defined before we get to libpq/. That
in turn allows us to recurse into libpq/test from within libpq/meson.build,
which allows us to define the tests with proper dependencies.

With all of these applied
ninja clean && ninja meson-test-prereq && m test --no-rebuild

passes, which doesn't guarantee that *all* dependencies are correctly
declared, but certainly is better than where we were before.

Greetings,

Andres Freund

On 07.12.24 21:45, Andres Freund wrote:

If a test target does not have ay dependency 'meson test' treats it has having
a dependency on the project default test. Which in turn currently includes
everything that will be installed, i.e. everything that tmp_install might
need. But that's not something I think we should rely on going forward.

I don't understand this. What is the project default test? I don't
find any documentation about this default dependency. (Not everything
like that tends to be documented, but do you have a source code
reference or a GH issue?)

Hi,

On 2024-12-09 16:06:18 +0100, Peter Eisentraut wrote:

On 07.12.24 21:45, Andres Freund wrote:

If a test target does not have ay dependency 'meson test' treats it has having
a dependency on the project default test. Which in turn currently includes
everything that will be installed, i.e. everything that tmp_install might
need. But that's not something I think we should rely on going forward.

I don't understand this. What is the project default test?

Oops, default target, not test.

I don't find any documentation about this default dependency. (Not
everything like that tends to be documented, but do you have a source code
reference or a GH issue?)

https://github.com/mesonbuild/meson/blob/83253cdbaa8afd268286ca06520ca1cf2095dd28/mesonbuild/mtest.py#L2164C1-L2175C74

Greetings,

Andres Freund

Hi,

I pushed these changes as part of
/messages/by-id/fucdlk6bgvrz6vb6ruscxa5xof5w2c3voxoqontl7oasf4idxl@uyljndimefct

Greetings,

Andres Freund