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.
> > 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