Missing program_XXX calling in pgbench tests
Dear hackers,
While reviewing another thread I found #SUBJECT.
Most of client program executes program_help_ok, program_version_ok,
program_options_handling_ok() in their test, but pgbench does not do. Instead
pgbench has own tests for --help and --version options.
One concern is that program_help_ok() checks whether lines are too long or not.
For now, the rule seems to be kept but not sure in future.
I feel that we can replace tests with common function, like attached. How do you
think?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
0001-pgbench-Run-program_XXX-tests-instead-of-its-own-tes.patchapplication/octet-stream; name=0001-pgbench-Run-program_XXX-tests-instead-of-its-own-tes.patchDownload
From 39b09415ed136e3223d42c2f112f61bd4be2c2ff Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 5 Jun 2025 10:14:48 +0900
Subject: [PATCH] pgbench: Run program_XXX tests instead of its own tests
---
src/bin/pgbench/t/002_pgbench_no_server.pl | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index f975c73dd75..2cc59cc8140 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -233,21 +233,9 @@ for my $o (@options)
'pgbench option error: ' . $name);
}
-# Help
-pgbench(
- '--help', 0,
- [
- qr{benchmarking tool for PostgreSQL},
- qr{Usage},
- qr{Initialization options:},
- qr{Common options:},
- qr{Report bugs to}
- ],
- [qr{^$}],
- 'pgbench help');
-
-# Version
-pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version');
+program_help_ok('pgbench');
+program_version_ok('pgbench');
+program_options_handling_ok('pgbench');
# list of builtins
pgbench(
--
2.47.1
On 2025/06/05 10:18, Hayato Kuroda (Fujitsu) wrote:
Dear hackers,
While reviewing another thread I found #SUBJECT.
Most of client program executes program_help_ok, program_version_ok,
program_options_handling_ok() in their test, but pgbench does not do. Instead
pgbench has own tests for --help and --version options.One concern is that program_help_ok() checks whether lines are too long or not.
For now, the rule seems to be kept but not sure in future.I feel that we can replace tests with common function, like attached. How do you
think?
+1
A bit similar discussion came up before regarding pgbench and program_xxx_ok
in [1]/messages/by-id/20180723033518.GG2854@paquier.xyz, but it seems that change was never applied.
Regards,
[1]: /messages/by-id/20180723033518.GG2854@paquier.xyz
--
Fujii Masao
NTT DATA Japan Corporation
Dear Fujii-san,
A bit similar discussion came up before regarding pgbench and program_xxx_ok
in [1], but it seems that change was never applied.
I didn't know that, thanks for sharing. ISTM, it tried to extend function to test the
shorter options.
While verifying the idea, I found that pg_config and pg_bsd_indent have not been
supported "-V" option yet. They must address the option ro improve test functions.
Attached patch set implemented the idea. 0001 is my original point and can be
backported. 0002-0004 needs API changes so they aim to be applied for HEAD.
If possible, I want to fork another thread to discuss 0002-0004 and want to
concentrate 0001 here.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
v2-0001-pgbench-Run-program_XXX-tests-instead-of-its-own-.patchapplication/octet-stream; name=v2-0001-pgbench-Run-program_XXX-tests-instead-of-its-own-.patchDownload
From 39b09415ed136e3223d42c2f112f61bd4be2c2ff Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 5 Jun 2025 10:14:48 +0900
Subject: [PATCH v2 1/4] pgbench: Run program_XXX tests instead of its own
tests
---
src/bin/pgbench/t/002_pgbench_no_server.pl | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index f975c73dd75..2cc59cc8140 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -233,21 +233,9 @@ for my $o (@options)
'pgbench option error: ' . $name);
}
-# Help
-pgbench(
- '--help', 0,
- [
- qr{benchmarking tool for PostgreSQL},
- qr{Usage},
- qr{Initialization options:},
- qr{Common options:},
- qr{Report bugs to}
- ],
- [qr{^$}],
- 'pgbench help');
-
-# Version
-pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version');
+program_help_ok('pgbench');
+program_version_ok('pgbench');
+program_options_handling_ok('pgbench');
# list of builtins
pgbench(
--
2.47.1
v2-0002-pg_config-add-V-option.patchapplication/octet-stream; name=v2-0002-pg_config-add-V-option.patchDownload
From 037cc33319d7c23ea6d03fbda41115c5c1da96e3 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 5 Jun 2025 11:52:12 +0900
Subject: [PATCH v2 2/4] pg_config: add -V option
---
src/bin/pg_config/pg_config.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/bin/pg_config/pg_config.c b/src/bin/pg_config/pg_config.c
index 28db024263d..7c2ad204200 100644
--- a/src/bin/pg_config/pg_config.c
+++ b/src/bin/pg_config/pg_config.c
@@ -63,6 +63,7 @@ static const InfoItem info_items[] = {
{"--ldflags_sl", "LDFLAGS_SL"},
{"--libs", "LIBS"},
{"--version", "VERSION"},
+ {"-V", "VERSION"},
{NULL, NULL}
};
--
2.47.1
v2-0003-pg_bsd_indent-add-V-option.patchapplication/octet-stream; name=v2-0003-pg_bsd_indent-add-V-option.patchDownload
From 465a19c46709f92e7d4d288e3bec5146bd969e55 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 5 Jun 2025 11:52:27 +0900
Subject: [PATCH v2 3/4] pg_bsd_indent: add -V option
---
src/tools/pg_bsd_indent/args.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/tools/pg_bsd_indent/args.c b/src/tools/pg_bsd_indent/args.c
index 5fa7e6b038c..4dec315e0a2 100644
--- a/src/tools/pg_bsd_indent/args.c
+++ b/src/tools/pg_bsd_indent/args.c
@@ -93,6 +93,7 @@ struct pro {
{"T", PRO_SPECIAL, 0, KEY, 0},
{"U", PRO_SPECIAL, 0, KEY_FILE, 0},
+ {"V", PRO_SPECIAL, 0, VERSION, 0},
{"-version", PRO_SPECIAL, 0, VERSION, 0},
{"P", PRO_SPECIAL, 0, IGN, 0},
{"bacc", PRO_BOOL, false, ON, &blanklines_around_conditional_compilation},
--
2.47.1
v2-0004-Allow-program_XXX_ok-functions-to-test-short-opti.patchapplication/octet-stream; name=v2-0004-Allow-program_XXX_ok-functions-to-test-short-opti.patchDownload
From 64429da9ec4ebd607a7475ce5a16be0f397cd621 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 5 Jun 2025 11:51:11 +0900
Subject: [PATCH v2 4/4] Allow program_XXX_ok functions to test short option
---
src/test/perl/PostgreSQL/Test/Utils.pm | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 7d7ca83495f..bfa0d0fb0b2 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -918,7 +918,7 @@ sub command_exit_is
=item program_help_ok(cmd)
-Check that the command supports the C<--help> option.
+Check that the command supports the C<-?> and C<--help> option.
=cut
@@ -927,8 +927,17 @@ sub program_help_ok
local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
my ($stdout, $stderr);
+
+ print("# Running: $cmd -?\n");
+ my $result = IPC::Run::run [ $cmd, '-?' ],
+ '>' => \$stdout,
+ '2>' => \$stderr;
+ ok($result, "$cmd -? exit code 0");
+ isnt($stdout, '', "$cmd -? goes to stdout");
+ is($stderr, '', "$cmd -? nothing to stderr");
+
print("# Running: $cmd --help\n");
- my $result = IPC::Run::run [ $cmd, '--help' ],
+ $result = IPC::Run::run [ $cmd, '--help' ],
'>' => \$stdout,
'2>' => \$stderr;
ok($result, "$cmd --help exit code 0");
@@ -950,7 +959,7 @@ sub program_help_ok
=item program_version_ok(cmd)
-Check that the command supports the C<--version> option.
+Check that the command supports the C<-V> and C<--version> option.
=cut
@@ -959,8 +968,17 @@ sub program_version_ok
local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
my ($stdout, $stderr);
+
+ print("# Running: $cmd -V\n");
+ my $result = IPC::Run::run [ $cmd, '-V' ],
+ '>' => \$stdout,
+ '2>' => \$stderr;
+ ok($result, "$cmd -V exit code 0");
+ isnt($stdout, '', "$cmd -V goes to stdout");
+ is($stderr, '', "$cmd -V nothing to stderr");
+
print("# Running: $cmd --version\n");
- my $result = IPC::Run::run [ $cmd, '--version' ],
+ $result = IPC::Run::run [ $cmd, '--version' ],
'>' => \$stdout,
'2>' => \$stderr;
ok($result, "$cmd --version exit code 0");
--
2.47.1
On 05.06.25 05:00, Hayato Kuroda (Fujitsu) wrote:
Dear Fujii-san,
A bit similar discussion came up before regarding pgbench and program_xxx_ok
in [1], but it seems that change was never applied.I didn't know that, thanks for sharing. ISTM, it tried to extend function to test the
shorter options.While verifying the idea, I found that pg_config and pg_bsd_indent have not been
supported "-V" option yet. They must address the option ro improve test functions.Attached patch set implemented the idea. 0001 is my original point and can be
backported. 0002-0004 needs API changes so they aim to be applied for HEAD.If possible, I want to fork another thread to discuss 0002-0004 and want to
concentrate 0001 here.
Patch 0001 looks very sensible.
I don't think we need to bother we the other ones. pg_config works
differently than the other programs anyway, because --version does not
exit the program. And pg_bsd_indent is an externally maintained
program. So I think it is ok if these two are intentionally different.
Dear Peter,
Thanks for the comment.
Patch 0001 looks very sensible.
I don't think we need to bother we the other ones. pg_config works
differently than the other programs anyway, because --version does not
exit the program. And pg_bsd_indent is an externally maintained
program. So I think it is ok if these two are intentionally different.
You meant that 0002-0004 are not needed, right?
So let's put on out-of-scope...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On 2025/06/05 16:44, Hayato Kuroda (Fujitsu) wrote:
Dear Peter,
Thanks for the comment.
Patch 0001 looks very sensible.
I don't think we need to bother we the other ones. pg_config works
differently than the other programs anyway, because --version does not
exit the program. And pg_bsd_indent is an externally maintained
program. So I think it is ok if these two are intentionally different.You meant that 0002-0004 are not needed, right?
So let's put on out-of-scope...
I agree with Peter. I don't think patches 0002 and 0003 are necessary.
As for 0004, it adds tests for the short options -? and -V, which
duplicate the existing tests for the long options --help and --version.
I'm not sure it's worth adding tests just to confirm that the short
and long options behave the same.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
Dear Fujii-san,
I agree with Peter. I don't think patches 0002 and 0003 are necessary.
As for 0004, it adds tests for the short options -? and -V, which
duplicate the existing tests for the long options --help and --version.
I'm not sure it's worth adding tests just to confirm that the short
and long options behave the same.
Only adding 0004 is not allowed because it can fail due to other commands.
So let's drop them.
I verified 0001 can be applied cleanly for all supported branches. To clarify,
let me attach the 0001 patch again. Please focus on it...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
v3-PG13-HEAD-0001-pgbench-Run-program_XXX-tests-instead-of-its.patchapplication/octet-stream; name=v3-PG13-HEAD-0001-pgbench-Run-program_XXX-tests-instead-of-its.patchDownload
From f9d53f5b1aab86b0e47024f54207df8f4347a70d Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Thu, 5 Jun 2025 10:14:48 +0900
Subject: [PATCH v3-HEAD] pgbench: Run program_XXX tests instead of its own
tests
---
src/bin/pgbench/t/002_pgbench_no_server.pl | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index f975c73dd75..2cc59cc8140 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -233,21 +233,9 @@ for my $o (@options)
'pgbench option error: ' . $name);
}
-# Help
-pgbench(
- '--help', 0,
- [
- qr{benchmarking tool for PostgreSQL},
- qr{Usage},
- qr{Initialization options:},
- qr{Common options:},
- qr{Report bugs to}
- ],
- [qr{^$}],
- 'pgbench help');
-
-# Version
-pgbench('-V', 0, [qr{^pgbench .PostgreSQL. }], [qr{^$}], 'pgbench version');
+program_help_ok('pgbench');
+program_version_ok('pgbench');
+program_options_handling_ok('pgbench');
# list of builtins
pgbench(
--
2.47.1
On 2025/06/09 13:48, Hayato Kuroda (Fujitsu) wrote:
Dear Fujii-san,
I agree with Peter. I don't think patches 0002 and 0003 are necessary.
As for 0004, it adds tests for the short options -? and -V, which
duplicate the existing tests for the long options --help and --version.
I'm not sure it's worth adding tests just to confirm that the short
and long options behave the same.Only adding 0004 is not allowed because it can fail due to other commands.
So let's drop them.I verified 0001 can be applied cleanly for all supported branches. To clarify,
let me attach the 0001 patch again. Please focus on it...
+1 to focusing on the 0001 patch.
Since this isn't a bug fix, I'm not sure back-patching is strictly necessary.
That said, it does improve consistency and test coverage, e.g., by adding checks
like help text length, so I'd be fine with back-patching if others see value in it.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
Dear Fujii-san,
+1 to focusing on the 0001 patch.
Since this isn't a bug fix, I'm not sure back-patching is strictly necessary.
That said, it does improve consistency and test coverage, e.g., by adding checks
like help text length, so I'd be fine with back-patching if others see value in it.
Initially I thought this was helpful even for back branches, but it is not
100% needed.
No objections even if it is only applied to master - it can check new features in
future.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
On 12.06.25 05:23, Hayato Kuroda (Fujitsu) wrote:
+1 to focusing on the 0001 patch.
Since this isn't a bug fix, I'm not sure back-patching is strictly necessary.
That said, it does improve consistency and test coverage, e.g., by adding checks
like help text length, so I'd be fine with back-patching if others see value in it.Initially I thought this was helpful even for back branches, but it is not
100% needed.
No objections even if it is only applied to master - it can check new features in
future.
committed