Обсуждение: pgsql: oauth: Add unit tests for multiplexer handling

oauth: Add unit tests for multiplexer handling

To better record the internal behaviors of oauth-curl.c, add a unit test
suite for the socket and timer handling code. This is all based on TAP
and driven by our existing Test::More infrastructure.

Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
Discussion: https://postgr.es/m/CAOYmi+nDZxJHaWj9_jRSyf8uMToCADAmOfJEggsKW-kY7aUwHA@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/1443b6c0eaa2b464affc0c3aacb3c3bf09efcd6d

Modified Files
--------------
src/interfaces/libpq-oauth/Makefile          |  14 +
src/interfaces/libpq-oauth/meson.build       |  35 ++
src/interfaces/libpq-oauth/t/001_oauth.pl    |  24 ++
src/interfaces/libpq-oauth/test-oauth-curl.c | 527 +++++++++++++++++++++++++++
4 files changed, 600 insertions(+)

On Fri, Aug 8, 2025 at 9:07 AM Jacob Champion <jchampion@postgresql.org> wrote:
>
> oauth: Add unit tests for multiplexer handling

Hmm, this has broken a couple of animals. Investigating.

--Jacob

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> Hmm, this has broken a couple of animals. Investigating.

At least on sifaka, oauth_tests should not be getting built at all,
because it doesn't use --with-libcurl nor have access to that library.
The link failure is unsurprising given that you're trying to build it
anyway.

            regards, tom lane

On Fri, Aug 8, 2025 at 9:42 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> At least on sifaka, oauth_tests should not be getting built at all,
> because it doesn't use --with-libcurl nor have access to that library.
> The link failure is unsurprising given that you're trying to build it
> anyway.

src/interfaces/libpq-oauth/Makefile is supposed to be skipped if
with_libcurl isn't there, though? Otherwise we'd fail to build the
`all` target, too.

Is the buildfarm client trying to build that directory explicitly?

--Jacob

On Fri, Aug 8, 2025 at 9:46 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Is the buildfarm client trying to build that directory explicitly?

Ah, yeah:

>     foreach my $testdir (
>        glob(
>            "$pgsql/src/test/modules/*
>             $pgsql/src/interfaces/*
>             $pgsql/src/tools/*"
>        )
>      )

Before this commit, there was no t/ directory to get caught by the
buildfarm. We've had to work around this in past, it looks like:

> next if $testname =~ /ssl/ && !$using_ssl;

Thinking...

--Jacob

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> Before this commit, there was no t/ directory to get caught by the
> buildfarm. We've had to work around this in past, it looks like:
>> next if $testname =~ /ssl/ && !$using_ssl;

Yeah, that's a horrid kluge.  The makefiles themselves ought to
short-circuit building the test program.  I think the issue is that
we apply that short-circuit at the next makefile level up --- can
we do it in src/interfaces/libpq-oauth/Makefile itself?

            regards, tom lane

On Fri, Aug 8, 2025 at 10:01 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, that's a horrid kluge.  The makefiles themselves ought to
> short-circuit building the test program.  I think the issue is that
> we apply that short-circuit at the next makefile level up --- can
> we do it in src/interfaces/libpq-oauth/Makefile itself?

I'm working on an ifeq test to do that, but some Meson animals are
also reporting issues:

/usr/bin/ld: test-oauth-curl.o: undefined reference to symbol
'floor@@GLIBC_2.2.5'
/usr/bin/ld: /lib/x86_64-linux-gnu/libm.so.6: error adding symbols:
DSO missing from command line
clang: error: linker command failed with exit code 1 (use -v to see invocation)

This is not fully baked enough. I'll revert the test commit for now;
today is not the day to make other people fight a red farm.

--Jacob

On Fri, Aug 8, 2025 at 10:05 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> I'll revert the test commit for now;
> today is not the day to make other people fight a red farm.

Reverted.

So when I go for a followup next week... I could
1) wrap just the installcheck and check targets in ifeq
2) wrap the `all` target in addition to the above
3) wrap everything after the Makefile.global inclusion

Any preferences?

--Jacob

Jacob Champion <jacob.champion@enterprisedb.com> writes:
> So when I go for a followup next week... I could
> 1) wrap just the installcheck and check targets in ifeq
> 2) wrap the `all` target in addition to the above
> 3) wrap everything after the Makefile.global inclusion
> Any preferences?

I think it'd be advisable to keep the "clean" target doing its thing.
But I agree with disabling 'all', 'check', 'installcheck'.

            regards, tom lane

On Fri, Aug 8, 2025 at 10:26 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think it'd be advisable to keep the "clean" target doing its thing.

Makes sense.

> But I agree with disabling 'all', 'check', 'installcheck'.

I'm also planning to add the `install` target to that protected list
(while keeping `uninstall` outside it, by the same logic as above).

--Jacob