Thread: Re: [PATCH] Add regression tests of ecpg command notice (error / warning)
On Wed, Feb 5, 2025 at 9:31 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> wrote: > > Hi hackers, > > When I wrote patch of ecpg command notice bug, I recognized needs of > regression tests for ecpg command notices and I say that I write the > tests. > > https://commitfest.postgresql.org/52/5497/ > https://www.postgresql.org/message-id/0efab1f6-5d8d-451f-a7dc-ef9c73ba9e02%40oss.nttdata.com > > This mail is about patch of the tests. > > Patch is attached to this mail : ecpg_cmd_notice_regress_test.patch > This patch passed CI. > https://cirrus-ci.com/build/4861827500212224 > > And corresponding diff is below. > https://github.com/ryogrid/postgres/compare/0ec3c295e7594ed3af86bca1a4b4be269c2f069d...72329311a75630594bcaa38248255360b7e8e525 > > I explain about implementation of this patch. > > What is this patch > - add regression tests which test ecpg command notices such as warning > and errors > - test notices implemented in ecpg.addons file > > Basic policy on implementation > - do in a way that matches the method using the existing pg_regress > command as much as possible > - avoid methods that increase the scope of influence > > Next, I list answers to points that are likely to be pointed out in > advance below :) > - shell scripts and bat files is used due to ... > avoid non zero exit code of ecpg command makes tests failure > avoid increasing C code for executing binary which cares cross platform > - python code is used because I couldn't write meson.build > appropriately describe dependency about materials which is used on > tests without it. please help me... > - as you said, kick this kind of tests by pg_regress accompanied with > needless PG server process execution. but pg_regress doesn't execute > test without it and making pg_regress require modification which has > not small scope of influence Sorry, I re-send patch because a patch already sent includes needless stderr output file. NEW PATCH FILENAME: ecpg_cmd_notice_regress_test2.patch In addition, diff between patches does not affect test behavior. --- Sincerely, Ryo Kanbayashi https://github.com/ryogrid
On Thu, Feb 13, 2025 at 10:49 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/02/06 8:57, Ryo Kanbayashi wrote: > > On Wed, Feb 5, 2025 at 9:31 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> wrote: > >> > >> Hi hackers, > >> > >> When I wrote patch of ecpg command notice bug, I recognized needs of > >> regression tests for ecpg command notices and I say that I write the > >> tests. > > Thanks for working on this! > > > >> I explain about implementation of this patch. > >> > >> What is this patch > >> - add regression tests which test ecpg command notices such as warning > >> and errors > >> - test notices implemented in ecpg.addons file > >> > >> Basic policy on implementation > >> - do in a way that matches the method using the existing pg_regress > >> command as much as possible > >> - avoid methods that increase the scope of influence > >> > >> Next, I list answers to points that are likely to be pointed out in > >> advance below :) > >> - shell scripts and bat files is used due to ... > >> avoid non zero exit code of ecpg command makes tests failure > >> avoid increasing C code for executing binary which cares cross platform > >> - python code is used because I couldn't write meson.build > >> appropriately describe dependency about materials which is used on > >> tests without it. please help me... > >> - as you said, kick this kind of tests by pg_regress accompanied with > >> needless PG server process execution. but pg_regress doesn't execute > >> test without it and making pg_regress require modification which has > >> not small scope of influence > > Wouldn't it be simpler to use the existing TAP test mechanism, > as shown in the attached patch? Please note that this patch is very WIP, > so there would be many places that need further implementation and refinement. Fujii San, Thank you for reviewing and indication of better implementation. I rewrite my patch based on your reference implementation :) --- Great regards, Ryo Kanbayashi NTT Open Source Software Center
On Tue, Feb 18, 2025 at 12:49 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> wrote: > > On Thu, Feb 13, 2025 at 10:49 PM Fujii Masao > <masao.fujii@oss.nttdata.com> wrote: > > > > > > > > On 2025/02/06 8:57, Ryo Kanbayashi wrote: > > > On Wed, Feb 5, 2025 at 9:31 PM Ryo Kanbayashi <kanbayashi.dev@gmail.com> wrote: > > >> > > >> Hi hackers, > > >> > > >> When I wrote patch of ecpg command notice bug, I recognized needs of > > >> regression tests for ecpg command notices and I say that I write the > > >> tests. > > > > Thanks for working on this! > > > > > > >> I explain about implementation of this patch. > > >> > > >> What is this patch > > >> - add regression tests which test ecpg command notices such as warning > > >> and errors > > >> - test notices implemented in ecpg.addons file > > >> > > >> Basic policy on implementation > > >> - do in a way that matches the method using the existing pg_regress > > >> command as much as possible > > >> - avoid methods that increase the scope of influence > > >> > > >> Next, I list answers to points that are likely to be pointed out in > > >> advance below :) > > >> - shell scripts and bat files is used due to ... > > >> avoid non zero exit code of ecpg command makes tests failure > > >> avoid increasing C code for executing binary which cares cross platform > > >> - python code is used because I couldn't write meson.build > > >> appropriately describe dependency about materials which is used on > > >> tests without it. please help me... > > >> - as you said, kick this kind of tests by pg_regress accompanied with > > >> needless PG server process execution. but pg_regress doesn't execute > > >> test without it and making pg_regress require modification which has > > >> not small scope of influence > > > > Wouldn't it be simpler to use the existing TAP test mechanism, > > as shown in the attached patch? Please note that this patch is very WIP, > > so there would be many places that need further implementation and refinement. > > Fujii San, > > Thank you for reviewing and indication of better implementation. > I rewrite my patch based on your reference implementation :) Fujii San and other hackers, I have rewrote my patch on TAP test sttyle :) File for build are also updated. Commitfest entry: https://commitfest.postgresql.org/patch/5543/ --- Great regards, Ryo Kanbayashi NTT Open Source Software Center
Attachment
On 2025/02/28 9:24, Ryo Kanbayashi wrote:
> I have rewrote my patch on TAP test sttyle :)
> File for build are also updated.
Thanks for updating the patch!
+ 'tests': [
+ 't/001_ecpg_notice.pl',
+ 't/002_ecpg_notice_informix.pl',
Since neither test emits "notice" messages, shouldn't the test file
names be revised to reflect this?
Also, I'm unsure if it's ideal to place input files directly under
the "t" directory. I looked for similar TAP tests with input files,
but I couldn't find any examples to guide this decision...
+program_help_ok('ecpg');
+program_version_ok('ecpg');
+program_options_handling_ok('ecpg');
+command_fails(['ecpg'], 'ecpg without arguments fails');
These checks seem unnecessary in 002 since they're already covered in 001.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, Feb 28, 2025 at 11:27 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2025/02/28 9:24, Ryo Kanbayashi wrote:
> > I have rewrote my patch on TAP test sttyle :)
> > File for build are also updated.
>
> Thanks for updating the patch!
Thanks for review:)
>
> + 'tests': [
> + 't/001_ecpg_notice.pl',
> + 't/002_ecpg_notice_informix.pl',
>
> Since neither test emits "notice" messages, shouldn't the test file
> names be revised to reflect this?
I replaced "notice" to "err_warn_msg"
> Also, I'm unsure if it's ideal to place input files directly under
> the "t" directory. I looked for similar TAP tests with input files,
> but I couldn't find any examples to guide this decision...
I couldn't too. So places are not changed.
> +program_help_ok('ecpg');
> +program_version_ok('ecpg');
> +program_options_handling_ok('ecpg');
> +command_fails(['ecpg'], 'ecpg without arguments fails');
>
> These checks seem unnecessary in 002 since they're already covered in 001.
I reflected above.
---
Great regards,
Ryo Kanbayashi
Attachment
On 2025/03/01 19:45, Ryo Kanbayashi wrote:
>> +program_help_ok('ecpg');
>> +program_version_ok('ecpg');
>> +program_options_handling_ok('ecpg');
>> +command_fails(['ecpg'], 'ecpg without arguments fails');
>>
>> These checks seem unnecessary in 002 since they're already covered in 001.
>
> I reflected above.
Thanks for updating the patch!
I've made some minor fixes and cosmetic adjustments.
The updated patch is attached.
Unless there are any objections, I'll commit it.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachment
>On Mon, Mar 3, 2025 at 12:23 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> On 2025/03/01 19:45, Ryo Kanbayashi wrote:
> >> +program_help_ok('ecpg');
> >> +program_version_ok('ecpg');
> >> +program_options_handling_ok('ecpg');
> >> +command_fails(['ecpg'], 'ecpg without arguments fails');
> >>
> >> These checks seem unnecessary in 002 since they're already covered in 001.
> >
> > I reflected above.
>
> Thanks for updating the patch!
>
> I've made some minor fixes and cosmetic adjustments.
> The updated patch is attached.
>
> Unless there are any objections, I'll commit it.
Thanks for reviewing and adustments.
There is no objections :)
---
Great regards,
Ryo Kanbayashi
NTT Open Source Software Center
On 2025/03/03 14:09, Ryo Kanbayashi wrote:
>> On Mon, Mar 3, 2025 at 12:23 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> On 2025/03/01 19:45, Ryo Kanbayashi wrote:
>>>> +program_help_ok('ecpg');
>>>> +program_version_ok('ecpg');
>>>> +program_options_handling_ok('ecpg');
>>>> +command_fails(['ecpg'], 'ecpg without arguments fails');
>>>>
>>>> These checks seem unnecessary in 002 since they're already covered in 001.
>>>
>>> I reflected above.
>>
>> Thanks for updating the patch!
>>
>> I've made some minor fixes and cosmetic adjustments.
>> The updated patch is attached.
>>
>> Unless there are any objections, I'll commit it.
>
> Thanks for reviewing and adustments.
> There is no objections :)
I've pushed the patch. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Mon, Mar 3, 2025 at 10:02 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> I've pushed the patch. Thanks!
Hi all,
> +tests += {
> + 'name': 'ecpg',
> + 'sd': meson.current_source_dir(),
> + 'bd': meson.current_build_dir(),
> + 'tap': {
> + 'tests': [
> + 't/001_ecpg_err_warn_msg.pl',
> + 't/002_ecpg_err_warn_msg_informix.pl',
> + ],
> + 'deps': ecpg_exe,
> + },
> +}
My version of Meson is complaining about this use of 'deps':
../meson.build:3603: WARNING: Project targets '>=0.54' but uses
feature introduced in '0.60.0': list.<plus>. The right hand operand
was not a list.
Adding test "ecpg/001_ecpg_err_warn_msg"
...
ecpg_exe should perhaps be wrapped in a list for now? I.e.
- 'deps': ecpg_exe,
+ 'deps': [ecpg_exe],
Thanks,
--Jacob
On 2025/03/05 7:26, Jacob Champion wrote:
> On Mon, Mar 3, 2025 at 10:02 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>> I've pushed the patch. Thanks!
>
> Hi all,
>
>> +tests += {
>> + 'name': 'ecpg',
>> + 'sd': meson.current_source_dir(),
>> + 'bd': meson.current_build_dir(),
>> + 'tap': {
>> + 'tests': [
>> + 't/001_ecpg_err_warn_msg.pl',
>> + 't/002_ecpg_err_warn_msg_informix.pl',
>> + ],
>> + 'deps': ecpg_exe,
>> + },
>> +}
>
> My version of Meson is complaining about this use of 'deps':
>
> ../meson.build:3603: WARNING: Project targets '>=0.54' but uses
> feature introduced in '0.60.0': list.<plus>. The right hand operand
> was not a list.
> Adding test "ecpg/001_ecpg_err_warn_msg"
> ...
>
> ecpg_exe should perhaps be wrapped in a list for now? I.e.
>
> - 'deps': ecpg_exe,
> + 'deps': [ecpg_exe],
Thanks for reporting this and suggesting a fix. I think you're right.
I confirmed that the compiler warning also appears in my environment,
and your fix resolves it. I’ve converted your fix into a patch, which is attached.
Unless there are any objections, I'm thinking to commit it.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Attachment
On 2025/03/05 9:32, Fujii Masao wrote:
>
>
> On 2025/03/05 7:26, Jacob Champion wrote:
>> On Mon, Mar 3, 2025 at 10:02 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>> I've pushed the patch. Thanks!
>>
>> Hi all,
>>
>>> +tests += {
>>> + 'name': 'ecpg',
>>> + 'sd': meson.current_source_dir(),
>>> + 'bd': meson.current_build_dir(),
>>> + 'tap': {
>>> + 'tests': [
>>> + 't/001_ecpg_err_warn_msg.pl',
>>> + 't/002_ecpg_err_warn_msg_informix.pl',
>>> + ],
>>> + 'deps': ecpg_exe,
>>> + },
>>> +}
>>
>> My version of Meson is complaining about this use of 'deps':
>>
>> ../meson.build:3603: WARNING: Project targets '>=0.54' but uses
>> feature introduced in '0.60.0': list.<plus>. The right hand operand
>> was not a list.
>> Adding test "ecpg/001_ecpg_err_warn_msg"
>> ...
>>
>> ecpg_exe should perhaps be wrapped in a list for now? I.e.
>>
>> - 'deps': ecpg_exe,
>> + 'deps': [ecpg_exe],
>
> Thanks for reporting this and suggesting a fix. I think you're right.
>
> I confirmed that the compiler warning also appears in my environment,
> and your fix resolves it. I’ve converted your fix into a patch, which is attached.
>
> Unless there are any objections, I'm thinking to commit it.
I've pushed the patch. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION