On Wed, 10 May 2023 at 13:39, Peter Smith <smithpb2...@gmail.com> wrote: > > On Mon, Apr 24, 2023 at 4:19 PM Julien Rouhaud <rjuju...@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