Horton

Thread

Commits

  1. oauth: Correct test dependency on oauth_hook_client

  2. meson: Add missing dependencies for libpq tests

  3. meson: Add missing dependencies to libpq_pipeline test

  4. meson: Add pg_regress_ecpg to ecpg test dependencies

  5. meson: Add test dependencies for test_json_parser

  6. meson: Improve dependencies for tmp_install test target

  7. meson: Narrow dependencies for 'install-quiet' target

  1. meson missing test dependencies

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-02T10:10:56Z

    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?

  2. Re: meson missing test dependencies

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-02T10:50:57Z

    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

  3. Re: meson missing test dependencies

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-02T19:27:37Z

    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.

  4. Re: meson missing test dependencies

    Michael Paquier <michael@paquier.xyz> — 2024-12-03T01:05:42Z

    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

  5. Re: meson missing test dependencies

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-03T08:18:13Z

    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

  6. Re: meson missing test dependencies

    Nazir Bilal Yavuz <byavuz81@gmail.com> — 2024-12-03T08:52:28Z

    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

  7. Re: meson missing test dependencies

    Andres Freund <andres@anarazel.de> — 2024-12-03T16:01:27Z

    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',

  8. Re: meson missing test dependencies

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-05T19:08:24Z

    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).

  9. Re: meson missing test dependencies

    Andres Freund <andres@anarazel.de> — 2024-12-05T20:10:08Z

    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

  10. Re: meson missing test dependencies

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-06T17:54:25Z

    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.

  11. Re: meson missing test dependencies

    Andres Freund <andres@anarazel.de> — 2024-12-07T20:45:37Z

    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

  12. Re: meson missing test dependencies

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-09T15:06:18Z

    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?)

  13. Re: meson missing test dependencies

    Andres Freund <andres@anarazel.de> — 2024-12-09T15:42:16Z

    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

  14. Re: meson missing test dependencies

    Andres Freund <andres@anarazel.de> — 2025-02-05T00:04:00Z

    Hi,

I pushed these changes as part of
https://postgr.es/m/fucdlk6bgvrz6vb6ruscxa5xof5w2c3voxoqontl7oasf4idxl%40uyljndimefct

Greetings,

Andres Freund