Thread: 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
Attachment
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], but it seems that change was never applied. Regards, [1] https://postgr.es/m/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
Attachment
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
Attachment
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