On Wed, 10 May 2023 at 13:39, Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Mon, Apr 24, 2023 at 4:19 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > Hi,
> >
> > On Thu, Apr 13, 2023 at 03:26:56PM +1000, Peter Smith wrote:
> > >
> > > 1.
> > > All the comments look alike, so it is hard to know what is going on.
> > > If each of the main test parts could be highlighted then the test code
> > > would be easier to read IMO.
> > >
> > > Something like below:
> > > [...]
> >
> > I added a bit more comments about what's is being tested. I'm not sure that a
> > big TEST CASE prefix is necessary, as it's not really multiple separated test
> > cases and other stuff can be tested in between. Also AFAICT no other TAP test
> > current needs this kind of banner, even if they're testing more complex
> > scenario.
>
> Hmm, I think there are plenty of examples of subscription TAP tests
> having some kind of highlighted comments as suggested, for better
> readability.
>
> e.g. See src/test/subscription
> t/014_binary.pl
> t/015_stream.pl
> t/016_stream_subxact.pl
> t/018_stream_subxact_abort.pl
> t/021_twophase.pl
> t/022_twophase_cascade.pl
> t/023_twophase_stream.pl
> t/028_row_filter.pl
> t/030_origin.pl
> t/031_column_list.pl
> t/032_subscribe_use_index.pl
>
> A simple #################### to separate the main test parts is all
> that is needed.
Modified
>
> > > 4b.
> > > All these messages like "Table t1 should still have 2 rows on the new
> > > subscriber" don't seem very helpful. e.g. They are not saying anything
> > > about WHAT this is testing or WHY it should still have 2 rows.
> >
> > I don't think that those messages are supposed to say what or why something is
> > tested, just give a quick context / reference on the test in case it's broken.
> > The comments are there to explain in more details what is tested and/or why.
> >
>
> But, why can’t they do both? They can be a quick reference *and* at
> the same time give some more meaning to the error log. Otherwise,
> these messages might as well just say ‘ref1’, ‘ref2’, ‘ref3’...
Modified
These are handled as part of v7 posted at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm1ZrbHaWpJwwNhDTJocRKWd3rEkgJazuDdZ9Z-WdvonFg%40mail.gmail.com
Regards,
Vignesh
