Thread: Re: meson missing test dependencies

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.

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