Re: meson and check-tests - Mailing list pgsql-hackers

On Wed, Sep 25, 2024 at 8:24 PM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
>
> Hi,
>
> Thanks for looking into this!
>
> On Wed, 25 Sept 2024 at 13:27, Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com> wrote:
> >
> > On Mon, Sep 23, 2024 at 2:16 PM Nazir Bilal Yavuz <byavuz81@gmail.com> wrote:
> > >
> > > On Sat, 21 Sept 2024 at 09:01, jian he <jian.universality@gmail.com> wrote:
> > > >
> > > > so
> > > > TESTS="copy jsonb stats_ext" meson test --suite regress
> > > > will fail.
> > > >
> > > > to make it work we need change it to
> > > > TESTS="test_setup copy jsonb stats_ext" meson test --suite regress
> > > >
> > > > Many tests depend on test_setup.sql, maybe we can implicitly prepend it.
> > > > Another dependency issue. alter_table depending on create_index.
> > > >
> > > > TESTS="test_setup alter_table" meson test --suite regress
> > > > will fail.
> > > > TESTS="test_setup create_index alter_table" meson test --suite regress
> > > > will work.
> > >
> > > Yes, I realized that but since that is how it is done in the make
> > > builds, I didn't want to change the behaviour. Also, I think it makes
> > > sense to leave it to the tester. It is more flexible in that way.
> >
> > Since meson has a setup suite already, it might have been a good idea
> > to do as Jian suggested. But a. setup is also another suite and not a
> > setup step per say. b. the dependencies between regression tests are
> > not documented well or rather we don't have a way to specify which
> > test depends upon which. So we can't infer the .sql files that need to
> > be run as a setup step. Somebody could add a dependency without meson
> > or make being aware of that and tests will fail again. So I think we
> > have to leave it as is. If we get to that point we should fix both
> > make as well as meson. But not as part of this exercise.
> >
> > It's a bit inconvenient that we don't see whether an individual test
> > failed or succeeded on the screen; we need to open testlog.txt for the
> > same. But that's true with the regress suite generally so no
> > complaints there.
>
> Thanks for sharing your thoughts.
>
> > Individual TAP tests are run using `meson test -C <build dir>
> > <suite>:<test>` syntax. If we can run individual SQL tests using same
> > syntax e.g. `meson test regress:partition_join` that would make it
> > consistent with the way TAP tests are run. But we need to make sure
> > that the test later in the syntax would see the objects left behind by
> > prior tests. E.g. `meson test regress:test_setup
> > regress:partition_join` should see both tests passing. partition_join
> > uses some tables created by test_setup, so those need to be run
> > sequentially. Is that doable?
>
> I think that makes sense, but it is not easily achievable right now.
> The difference between TAP tests and regress/regress tests is that TAP
> tests are registered individually, whereas regress/regress tests are
> registered as one (with the --schedule option). This means we need to
> register these tests one by one (instead of passing them with the
> --schedule option) to the Meson build system in order to run them as
> 'meson test <test_group>:<test>'.

I understand. Probably that's a shortcoming in the way our meson
support is designed. I don't see a way to fix it without changing a
lot. So I guess the current interface is good enough.

>
> Additionally, the patch I shared earlier was only for regress/regress
> tests. From what I understand from here [1], only regress/regress
> tests support 'make check-tests', so the patch seems correct. I
> experimented with how we can implement something similar for other
> types of tests, including other regression, isolation, and ECPG tests.
> The attached patch works for all types of tests but only for the Meson
> builds. For example you can run:
>
> $ meson test --suite setup
> $ TESTS='check check_btree' meson test amcheck/regress
> $ TESTS='ddl stream' meson test test_decoding/regress
> $ TESTS='oldest_xmin skip_snapshot_restore' meson test test_decoding/isolation
> $ TESTS='sql/prepareas compat_informix/dec_test' meson test ecpg/ecpg
>
> What do you think about that behaviour? It is different from 'make
> check-tests' but it looks useful to me.

I think that would be a good enhancement, if a particular regression
set takes longer e.g. the one in test_decoding takes a few seconds.
When we worked on PG_TEST_EXTRA, it was advised to keep feature parity
between meson and make. I guess, a similar advice applies here as well
and we will have to change make to support these options. But that
will be more work.

Let's split the patch into two 1. supporting TESTS in meson only for
regress/regress, 2. extending that support to other suites. The first
patch will bring meson inline with make as far as running a subset of
regression tests is concerned and can be committed separately. We will
seek opinions on the second patch and commit it separately if it takes
time. It will be good to see the support for running a subset of
regression in meson ASAP so that developers can save time running
entire regression suite when not needed. The second one will be an
additional feature that can wait if it takes more time to add it to
both meson and make.

--
Best Wishes,
Ashutosh Bapat