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. > > 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’... ------ Kind Regards, Peter Smith. Fujitsu Australia